null_blk: fix init/exit races and memleaks#6
Open
blktests-ci-block-trial[bot] wants to merge 9 commits into
Open
null_blk: fix init/exit races and memleaks#6blktests-ci-block-trial[bot] wants to merge 9 commits into
blktests-ci-block-trial[bot] wants to merge 9 commits into
Conversation
In null_init(), mutex_init(&lock) currently happens after configfs_register_subsystem(), which exposes the nullb subsystem to userspace. A racing mkdir() into /sys/kernel/config/nullb/ can reach null_find_dev_by_name() -> mutex_lock(&lock) before the mutex is initialized, trigger warning: [ 123.137788] DEBUG_LOCKS_WARN_ON(lock->magic != lock) [ 123.137796] WARNING: kernel/locking/mutex.c:159 at mutex_lock+0x171/0x1c0, CPU#13: mkdir/1301 [ 123.140090] Modules linked in: null_blk(+) nft_fib_inet nft_fib_ipv4 ...... [ 123.154926] Call Trace: [ 123.155172] <TASK> [ 123.155419] ? __pfx_mutex_lock+0x10/0x10 [ 123.156181] ? __pfx__raw_spin_lock+0x10/0x10 [ 123.156571] nullb_group_make_group+0x20/0x100 [null_blk] [ 123.157011] configfs_mkdir+0x47b/0xc70 [ 123.157337] ? __pfx_configfs_mkdir+0x10/0x10 [ 123.157719] ? may_create_dentry+0x242/0x2e0 [ 123.158061] vfs_mkdir+0x2a9/0x6c0 [ 123.158352] filename_mkdirat+0x3dc/0x500 [ 123.158710] ? __pfx_filename_mkdirat+0x10/0x10 [ 123.159070] ? strncpy_from_user+0x3a/0x1d0 [ 123.159413] __x64_sys_mkdir+0x6b/0x90 [ 123.159760] do_syscall_64+0xea/0x600 Replace the runtime mutex_init(&lock) with a static DEFINE_MUTEX(lock) declaration to fix this issue. Fixes: 49c3b92 ("block: null_blk: Improve device creation with configfs") Suggested-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Zizhi Wo <wozizhi@huawei.com> Reviewed-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
In null_init(), configfs_register_subsystem() currently runs before register_blkdev(), so when null_blk is built as a module, a racing mkdir() + poweron from userspace can reach null_add_dev() while null_major is still 0. __add_disk() then hits WARN_ON(disk->minors) (major=0 with minors!=0) and fails: [root@fedora ~]# [ 2366.521436] WARNING: block/genhd.c:476 at __add_disk+0x8a7/0xde0, [ 2366.523552] Modules linked in: null_blk(+) nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib [ 2366.529081] CPU: 26 UID: 0 PID: 1600 Comm: sh Not tainted 7.2.0-rc1+ #66 PREEMPT(full) ...... [ 2366.547251] Call Trace: [ 2366.547575] <TASK> [ 2366.547831] ? _raw_spin_lock+0x84/0xe0 [ 2366.548260] add_disk_fwnode+0x114/0x560 [ 2366.548739] null_add_dev+0x102d/0x1b80 [null_blk] [ 2366.549310] ? __pfx_null_add_dev+0x10/0x10 [null_blk] [ 2366.549906] ? mutex_lock+0xde/0x1c0 [ 2366.550361] ? __pfx_mutex_lock+0x10/0x10 [ 2366.550827] nullb_device_power_store+0x1e7/0x280 [null_blk] [ 2366.551499] ? __pfx_nullb_device_power_store+0x10/0x10 [null_blk] [ 2366.552177] ? __kmalloc_cache_noprof+0x1f5/0x470 [ 2366.552748] ? configfs_write_iter+0x35c/0x4e0 [ 2366.553242] configfs_write_iter+0x286/0x4e0 [ 2366.553787] vfs_write+0x52d/0xd00 [ 2366.554169] ? __pfx_vfs_write+0x10/0x10 [ 2366.554679] ? __pfx___css_rstat_updated+0x10/0x10 [ 2366.555196] ? fdget_pos+0x1cf/0x4c0 [ 2366.555649] ksys_write+0xfc/0x1d0 ...... Additionally, the err_dev path destroys all devices on nullb_list while configfs is still registered. If a racing mkdir() + poweron puts a user device on the list, null_destroy_dev()->null_free_dev() kfrees the user device's nullb_device but /sys/kernel/config/nullb/<name> is still reachable. Any userspace access to the item will trigger a UAF. For simplicity, move configfs_register_subsystem() to the end to solve the problems above. Fixes: 3bf2bd2 ("nullb: add configfs interface") Signed-off-by: Zizhi Wo <wozizhi@huawei.com> Reviewed-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
In null_exit(), unregister_blkdev() is called before the null_blk instances are destroyed, which is inconsistent with the cleanup order in null_init(). Move it after null_destroy_dev() so that teardown happens in the reverse order of initialization. No functional change intended. Suggested-by: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Zizhi Wo <wozizhi@huaweicloud.com> Reviewed-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
If shared_tags is enabled, null_setup_tagset() allocates the global tag_set via null_init_global_tag_set(). If device creation later fails, err_dev destroys the default devices and calls unregister_blkdev(), but never frees the global tag_set. Since module init failed, null_exit() is never invoked, so the global tag_set's tags and maps are permanently leaked. Free the global tag_set in err_dev, matching null_exit() which does if (tag_set.ops) blk_mq_free_tag_set(&tag_set). Fixes: 82f402f ("null_blk: add support for shared tags") Signed-off-by: Zizhi Wo <wozizhi@huawei.com> Reviewed-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
null_init_zoned_dev() allocates dev->zones when a zoned device is powered on, but null_del_dev() never frees it on power-off; dev->zones is only freed later in null_free_dev(), when the configfs directory is removed. If the device is powered off and then on again, null_init_zoned_dev() allocates a new array and overwrites the dev->zones pointer, leaking the previous allocation each power cycle. Free dev->zones in null_del_dev() via null_free_zoned_dev() to solve it. And calling null_del_dev() in null_free_dev() is no longer necessary because every caller already invokes null_del_dev() first: via nullb_group_drop_item() before nullb_device_release(), in the null_add_dev() error path of null_create_dev(), and in null_destroy_dev(). Remove the redundant call. Fixes: ca4b2a0 ("null_blk: add zone support") Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
Replace remaining nullb->dev dereferences with the already-cached local dev variable. No functional change. Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
When shared_tags is enabled, null_setup_tagset() makes the device use the global tag_set, whose driver_data stays NULL. null_map_queues() therefore falls back to the module-wide g_submit_queues/g_poll_queues instead of any per-device value. Resizing submit_queues or poll_queues via configfs on such a device calls blk_mq_update_nr_hw_queues() on the shared set, shrinking set->nr_hw_queues. __blk_mq_realloc_hw_ctxs() only grows the q->queue_hw_ctx[] allocation, so on shrink it merely exits and NULLs the now-excess hctx slots. null_map_queues(), however, keeps mapping CPUs with the unchanged g_submit_queues/g_poll_queues, so mq_map[] ends up pointing at those NULLed hctx slots. blk_mq_map_swqueue() then dereferences the NULL hctx (hctx->cpumask), crashing the kernel: [ 460.218374] KASAN: null-ptr-deref in range [0x0000000000000098-0x000000000000009f] [ 460.219003] CPU: 24 UID: 0 PID: 1492 Comm: sh Not tainted 7.2.0-rc2+ #67 PREEMPT(full) [ 460.219792] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.17.0-4.fc41 04/01/2014 [ 460.220452] RIP: 0010:blk_mq_map_swqueue+0x4db/0x1430 ...... [ 460.228977] Call Trace: [ 460.229175] <TASK> [ 460.229354] blk_mq_update_nr_hw_queues+0xd49/0x11c0 [ 460.229779] ? __pfx_blk_mq_update_nr_hw_queues+0x10/0x10 [ 460.230200] nullb_update_nr_hw_queues+0x1a9/0x370 [null_blk] [ 460.230694] nullb_device_submit_queues_store+0xd9/0x170 [null_blk] [ 460.231190] ? __pfx_nullb_device_submit_queues_store+0x10/0x10 [null_blk] [ 460.231776] ? configfs_write_iter+0x35c/0x4e0 [ 460.232122] configfs_write_iter+0x286/0x4e0 [ 460.232460] vfs_write+0x52d/0xd00 [ 460.232779] ? __x64_sys_openat+0x108/0x1d0 [ 460.233106] ? __pfx_vfs_write+0x10/0x10 [ 460.233413] ? fdget_pos+0x1cf/0x4c0 [ 460.233745] ? fput_close+0x133/0x190 [ 460.234038] ? __pfx_expand_files+0x10/0x10 [ 460.234368] ksys_write+0xfc/0x1d0 Reproducer: modprobe null_blk shared_tags=1 submit_queues=64 poll_queues=1 mkdir /sys/kernel/config/nullb/dev echo 1 > /sys/kernel/config/nullb/dev/power echo 1 > /sys/kernel/config/nullb/dev/submit_queues A per-device resize of a shared tag set is meaningless anyway, so reject it with -EINVAL in nullb_update_nr_hw_queues() when the device is bound to the global tag_set. Fixes: 45919fb ("null_blk: Enable modifying 'submit_queues' after an instance has been configured") Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
The NULLB_DEVICE_ATTR _store takes no lock: apply_fn attributes (submit_queues, poll_queues) get dev->NAME written again after apply_fn returns, outside its lock; APPLY=NULL attributes are entirely lockless. configfs only serializes stores per-open-file, so concurrent stores on separate fds race. For apply_fn attributes, once one store's apply_fn has reconfigured the hardware, a second (losing) store can still overwrite dev->NAME afterwards. This leaves dev->submit_queues out of sync with the live queue count, which is later caught by the WARN_ON_ONCE() in null_map_queues(). For !apply_fn attributes, power_store()'s null_add_dev() validates and builds the device under "lock" but only sets CONFIGURED afterwards. A store slipping in during this window can change a field mid-setup -- for example, zone_nr_conv can be pushed above nr_zones after it has already been clamped, leading to an out-of-bounds dev->zones[] access. Take "lock" in the macro around the apply_fn call, the CONFIGURED test and the field write, and move it out of nullb_apply_submit_queues()/ nullb_apply_poll_queues() so both paths are covered once. This serializes stores with power_store's setup and with each other. Also reset ret to 0 after the input parsing so that within the locked section ret is purely a status code, rather than carrying the byte count returned by nullb_device_##TYPE##_attr_store(). The field is then written only on success via if (!ret), giving a single consistent rule for both the apply_fn and the APPLY=NULL paths. Fixes: 45919fb ("null_blk: Enable modifying 'submit_queues' after an instance has been configured") Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
The _show callback in the NULLB_DEVICE_ATTR macro reads dev->NAME and the _store path writes it. configfs does not serialize accesses across separate open file descriptions (buffer->mutex is per-fd), and _show takes no lock, so a concurrent read and write on the same attribute is a data race. The _show readers also race against writes to these fields that run after the configfs item becomes visible, e.g. in nullb_update_nr_hw_queues(). All of those writers now run under the file-scope lock: _store takes it unconditionally, and the setup-side writers run under power_store() which holds the same lock. The only remaining unsynchronized accesses are the plain reads in _show. Rather than annotating every field with READ_ONCE()/WRITE_ONCE() across files, simply take the file-scope lock in _show (and in power_show) as well. This closes the remaining _show-vs-write data races with a single lock and keeps the writers as plain assignments. configfs attribute access is not on the I/O hot path, so taking the mutex in _show is acceptable from a performance standpoint. The dev fields written in null_alloc_dev() and dev->power in nullb_group_drop_item() need no locking: the former runs from .make_group before the item is published, and the latter is serialized by configfs frag_sem/frag_dead against attribute show/store. Suggested-by: Nilay Shroff <nilay@linux.ibm.com> Signed-off-by: Zizhi Wo <wozizhi@huawei.com>
Author
|
Upstream branch: a635d67 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request for series with
subject: null_blk: fix init/exit races and memleaks
version: 4
url: http://redsun45:8000/project/linux-block/list/?series=275