If the 'hctx' isn't removed from cpuhp callback list, we can't reuse it,
otherwise use-after-free may be triggered.
Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202412172217.b906db7c-lkp@intel.com
Tested-by: kernel test robot <oliver.sang@intel.com>
Fixes: 22465bbac53c ("blk-mq: move cpuhp callback registering out of q->sysfs_lock")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Link: https://lore.kernel.org/r/20241218101617.3275704-3-ming.lei@redhat.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
This reverts commit be26ba96421ab0a8fa2055ccf7db7832a13c44d2.
Commit be26ba96421a ("block: Fix potential deadlock while freezing queue and
acquiring sysfs_loc") actually reverts commit 22465bbac53c ("blk-mq: move cpuhp
callback registering out of q->sysfs_lock"), and causes the original resctrl
lockdep warning.
So revert it and we need to fix the issue in another way.
Cc: Nilay Shroff <nilay@linux.ibm.com>
Fixes: be26ba96421a ("block: Fix potential deadlock while freezing queue and acquiring sysfs_loc")
Signed-off-by: Ming Lei <ming.lei@redhat.com>
Link: https://lore.kernel.org/r/20241218101617.3275704-2-ming.lei@redhat.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
For storing a value to a queue attribute, the queue_attr_store function
first freezes the queue (->q_usage_counter(io)) and then acquire
->sysfs_lock. This seems not correct as the usual ordering should be to
acquire ->sysfs_lock before freezing the queue. This incorrect ordering
causes the following lockdep splat which we are able to reproduce always
simply by accessing /sys/kernel/debug file using ls command:
[ 57.597146] WARNING: possible circular locking dependency detected
[ 57.597154] 6.12.0-10553-gb86545e02e8c #20 Tainted: G W
[ 57.597162] ------------------------------------------------------
[ 57.597168] ls/4605 is trying to acquire lock:
[ 57.597176] c00000003eb56710 (&mm->mmap_lock){++++}-{4:4}, at: __might_fault+0x58/0xc0
[ 57.597200]
but task is already holding lock:
[ 57.597207] c0000018e27c6810 (&sb->s_type->i_mutex_key#3){++++}-{4:4}, at: iterate_dir+0x94/0x1d4
[ 57.597226]
which lock already depends on the new lock.
[ 57.597233]
the existing dependency chain (in reverse order) is:
[ 57.597241]
-> #5 (&sb->s_type->i_mutex_key#3){++++}-{4:4}:
[ 57.597255] down_write+0x6c/0x18c
[ 57.597264] start_creating+0xb4/0x24c
[ 57.597274] debugfs_create_dir+0x2c/0x1e8
[ 57.597283] blk_register_queue+0xec/0x294
[ 57.597292] add_disk_fwnode+0x2e4/0x548
[ 57.597302] brd_alloc+0x2c8/0x338
[ 57.597309] brd_init+0x100/0x178
[ 57.597317] do_one_initcall+0x88/0x3e4
[ 57.597326] kernel_init_freeable+0x3cc/0x6e0
[ 57.597334] kernel_init+0x34/0x1cc
[ 57.597342] ret_from_kernel_user_thread+0x14/0x1c
[ 57.597350]
-> #4 (&q->debugfs_mutex){+.+.}-{4:4}:
[ 57.597362] __mutex_lock+0xfc/0x12a0
[ 57.597370] blk_register_queue+0xd4/0x294
[ 57.597379] add_disk_fwnode+0x2e4/0x548
[ 57.597388] brd_alloc+0x2c8/0x338
[ 57.597395] brd_init+0x100/0x178
[ 57.597402] do_one_initcall+0x88/0x3e4
[ 57.597410] kernel_init_freeable+0x3cc/0x6e0
[ 57.597418] kernel_init+0x34/0x1cc
[ 57.597426] ret_from_kernel_user_thread+0x14/0x1c
[ 57.597434]
-> #3 (&q->sysfs_lock){+.+.}-{4:4}:
[ 57.597446] __mutex_lock+0xfc/0x12a0
[ 57.597454] queue_attr_store+0x9c/0x110
[ 57.597462] sysfs_kf_write+0x70/0xb0
[ 57.597471] kernfs_fop_write_iter+0x1b0/0x2ac
[ 57.597480] vfs_write+0x3dc/0x6e8
[ 57.597488] ksys_write+0x84/0x140
[ 57.597495] system_call_exception+0x130/0x360
[ 57.597504] system_call_common+0x160/0x2c4
[ 57.597516]
-> #2 (&q->q_usage_counter(io)#21){++++}-{0:0}:
[ 57.597530] __submit_bio+0x5ec/0x828
[ 57.597538] submit_bio_noacct_nocheck+0x1e4/0x4f0
[ 57.597547] iomap_readahead+0x2a0/0x448
[ 57.597556] xfs_vm_readahead+0x28/0x3c
[ 57.597564] read_pages+0x88/0x41c
[ 57.597571] page_cache_ra_unbounded+0x1ac/0x2d8
[ 57.597580] filemap_get_pages+0x188/0x984
[ 57.597588] filemap_read+0x13c/0x4bc
[ 57.597596] xfs_file_buffered_read+0x88/0x17c
[ 57.597605] xfs_file_read_iter+0xac/0x158
[ 57.597614] vfs_read+0x2d4/0x3b4
[ 57.597622] ksys_read+0x84/0x144
[ 57.597629] system_call_exception+0x130/0x360
[ 57.597637] system_call_common+0x160/0x2c4
[ 57.597647]
-> #1 (mapping.invalidate_lock#2){++++}-{4:4}:
[ 57.597661] down_read+0x6c/0x220
[ 57.597669] filemap_fault+0x870/0x100c
[ 57.597677] xfs_filemap_fault+0xc4/0x18c
[ 57.597684] __do_fault+0x64/0x164
[ 57.597693] __handle_mm_fault+0x1274/0x1dac
[ 57.597702] handle_mm_fault+0x248/0x484
[ 57.597711] ___do_page_fault+0x428/0xc0c
[ 57.597719] hash__do_page_fault+0x30/0x68
[ 57.597727] do_hash_fault+0x90/0x35c
[ 57.597736] data_access_common_virt+0x210/0x220
[ 57.597745] _copy_from_user+0xf8/0x19c
[ 57.597754] sel_write_load+0x178/0xd54
[ 57.597762] vfs_write+0x108/0x6e8
[ 57.597769] ksys_write+0x84/0x140
[ 57.597777] system_call_exception+0x130/0x360
[ 57.597785] system_call_common+0x160/0x2c4
[ 57.597794]
-> #0 (&mm->mmap_lock){++++}-{4:4}:
[ 57.597806] __lock_acquire+0x17cc/0x2330
[ 57.597814] lock_acquire+0x138/0x400
[ 57.597822] __might_fault+0x7c/0xc0
[ 57.597830] filldir64+0xe8/0x390
[ 57.597839] dcache_readdir+0x80/0x2d4
[ 57.597846] iterate_dir+0xd8/0x1d4
[ 57.597855] sys_getdents64+0x88/0x2d4
[ 57.597864] system_call_exception+0x130/0x360
[ 57.597872] system_call_common+0x160/0x2c4
[ 57.597881]
other info that might help us debug this:
[ 57.597888] Chain exists of:
&mm->mmap_lock --> &q->debugfs_mutex --> &sb->s_type->i_mutex_key#3
[ 57.597905] Possible unsafe locking scenario:
[ 57.597911] CPU0 CPU1
[ 57.597917] ---- ----
[ 57.597922] rlock(&sb->s_type->i_mutex_key#3);
[ 57.597932] lock(&q->debugfs_mutex);
[ 57.597940] lock(&sb->s_type->i_mutex_key#3);
[ 57.597950] rlock(&mm->mmap_lock);
[ 57.597958]
*** DEADLOCK ***
[ 57.597965] 2 locks held by ls/4605:
[ 57.597971] #0: c0000000137c12f8 (&f->f_pos_lock){+.+.}-{4:4}, at: fdget_pos+0xcc/0x154
[ 57.597989] #1: c0000018e27c6810 (&sb->s_type->i_mutex_key#3){++++}-{4:4}, at: iterate_dir+0x94/0x1d4
Prevent the above lockdep warning by acquiring ->sysfs_lock before
freezing the queue while storing a queue attribute in queue_attr_store
function. Later, we also found[1] another function __blk_mq_update_nr_
hw_queues where we first freeze queue and then acquire the ->sysfs_lock.
So we've also updated lock ordering in __blk_mq_update_nr_hw_queues
function and ensured that in all code paths we follow the correct lock
ordering i.e. acquire ->sysfs_lock before freezing the queue.
[1] https://lore.kernel.org/all/CAFj5m9Ke8+EHKQBs_Nk6hqd=LGXtk4mUxZUN5==ZcCjnZSBwHw@mail.gmail.com/
Reported-by: kjain@linux.ibm.com
Fixes: af2814149883 ("block: freeze the queue in queue_attr_store")
Tested-by: kjain@linux.ibm.com
Cc: hch@lst.de
Cc: axboe@kernel.dk
Cc: ritesh.list@gmail.com
Cc: ming.lei@redhat.com
Cc: gjoyce@linux.ibm.com
Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
Reviewed-by: Ming Lei <ming.lei@redhat.com>
Link: https://lore.kernel.org/r/20241210144222.1066229-1-nilay@linux.ibm.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
Move a statement that occurs in both branches of an if-statement in front of the if-statement. Fix a typo in a source code comment. No functionality has been changed. Reviewed-by: Damien Le Moal <dlemoal@kernel.org> Cc: Christoph Hellwig <hch@lst.de> Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Christoph Hellwig <hch@lst.de> Reviewed-by: Nitesh Shetty <nj.shetty@samsung.com> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Link: https://lore.kernel.org/r/20241212212941.1268662-3-bvanassche@acm.org Signed-off-by: Jens Axboe <axboe@kernel.dk>
Registering and unregistering cpuhp callback requires global cpu hotplug lock, which is used everywhere. Meantime q->sysfs_lock is used in block layer almost everywhere. It is easy to trigger lockdep warning[1] by connecting the two locks. Fix the warning by moving blk-mq's cpuhp callback registering out of q->sysfs_lock. Add one dedicated global lock for covering registering & unregistering hctx's cpuhp, and it is safe to do so because hctx is guaranteed to be live if our request_queue is live. [1] https://lore.kernel.org/lkml/Z04pz3AlvI4o0Mr8@agluck-desk3/ Cc: Reinette Chatre <reinette.chatre@intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: Peter Newman <peternewman@google.com> Cc: Babu Moger <babu.moger@amd.com> Reported-by: Luck Tony <tony.luck@intel.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> Tested-by: Tony Luck <tony.luck@intel.com> Link: https://lore.kernel.org/r/20241206111611.978870-3-ming.lei@redhat.com Signed-off-by: Jens Axboe <axboe@kernel.dk>
We need to retrieve 'hctx' from xarray table in the cpuhp callback, so the callback should be registered after this 'hctx' is added to xarray table. Cc: Reinette Chatre <reinette.chatre@intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: Peter Newman <peternewman@google.com> Cc: Babu Moger <babu.moger@amd.com> Cc: Luck Tony <tony.luck@intel.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> Tested-by: Tony Luck <tony.luck@intel.com> Link: https://lore.kernel.org/r/20241206111611.978870-2-ming.lei@redhat.com Signed-off-by: Jens Axboe <axboe@kernel.dk>
The rq->part pointer is already NULLified in the memset() call, so - like for other pointers in rq - don't re-NULLify. Signed-off-by: John Garry <john.g.garry@oracle.com> Link: https://lore.kernel.org/r/20241125100258.4172774-1-john.g.garry@oracle.com Signed-off-by: Jens Axboe <axboe@kernel.dk>
Fix an issue detected by the `smatch` tool: block/blk-mq.c:3314 blk_rq_prep_clone() error: uninitialized symbol 'bio'. This patch refactors `blk_rq_prep_clone()` to improve code readability and ensure safety by addressing potential misuse of the `bio` variable: - Move the bio_put(bio); call to the bio_ctr error handling block, which is the only place where it can be triggered. - Move the bio variable into the __rq_for_each_bio loop scope. This change removes the need to set bio to NULL at the loop's end. discussion on why bio remains uninitialized: https://lore.kernel.org/lkml/20241004141037.43277-1-surajsonawane0215@gmail.com Summary of above discussion: - I pointed out that `bio` can remain uninitialized if the allocation with `bio_alloc_clone` fails. - Keith Busch explained that `bio` is initialized to `NULL` when `bio_alloc_clone()` fails, preventing uninitialized usage. - John Garry questioned whether `rq_src->bio` being `NULL` could leave `bio` uninitialized. Keith clarified that in such cases, `bio` is not referenced, so it does not need initialization. - Christoph Hellwig recommended code improvements: - move the bio_put to the bio_ctr error handling, which is the only case where it can happen - move the bio variable into the __rq_for_each_bio scope, which also removed the need to zero it at the end of the loop These changes enhance code clarity, address static analysis tool warnings, and make the function more maintainable. thread of previous version patch discussion: https://lore.kernel.org/lkml/20241004100842.9052-1-surajsonawane0215@gmail.com Signed-off-by: Suraj Sonawane <surajsonawane0215@gmail.com> Reviewed-by: Christoph Hellwig <hch@lst.de> Link: https://lore.kernel.org/r/20241119164412.37609-1-surajsonawane0215@gmail.com Signed-off-by: Jens Axboe <axboe@kernel.dk>
Add requests to the tail of the list instead of the front so that they are queued up in submission order. Remove the re-reordering in blk_mq_dispatch_plug_list, virtio_queue_rqs and nvme_queue_rqs now that the list is ordered as expected. Signed-off-by: Christoph Hellwig <hch@lst.de> Link: https://lore.kernel.org/r/20241113152050.157179-6-hch@lst.de Signed-off-by: Jens Axboe <axboe@kernel.dk>
Replace the semi-open coded request list helpers with a proper rq_list type that mirrors the bio_list and has head and tail pointers. Besides better type safety this actually allows to insert at the tail of the list, which will be useful soon. Signed-off-by: Christoph Hellwig <hch@lst.de> Link: https://lore.kernel.org/r/20241113152050.157179-5-hch@lst.de Signed-off-by: Jens Axboe <axboe@kernel.dk>