Niova ublk 7.1 fix backports - #3
Open
bsbernd wants to merge 91 commits into
Open
Conversation
This reverts commit 0ad6835.
This reverts commit cd624cf.
Move ublk_req_build_flags() above its callers so it doesn't need to be forward-declared. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260520203654.1413640-2-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit eee9224)
The code for initializing struct ublksrv_io_desc on I/O dispatch is largely duplicated in 3 places. Commit 4d4a512 ("ublk: add PFN- based buffer matching in I/O path") added support to ublk_setup_iod() for matching request buffers against registered UBLK_F_SHMEM_ZC buffers, but missed adding it to ublk_setup_iod_zoned() for zoned requests. Move the duplicated logic to a new helper ublk_init_iod(). This way, zone appends can also benefit from avoiding the data copy. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260520203654.1413640-3-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 23130b3) The cherry pick to 7.1-niova differs in respect to blk_rq_has_data(): that inline came with 999722b ("blk-mq: introduce blk_rq_has_data()"), which 7.1 does not have. ublk_rq_has_data() stays and moves above ublk_init_iod(), so the call site still sees a declaration.
ublk_start_cancel() previously bailed out early when ublk_get_disk() returned NULL, treating it as "our disk has been dead". That is correct for the post-teardown case, but it also wrongly covers the pre-start case: ublk_ctrl_start_dev() has not assigned ub->ub_disk yet, while io_uring is already tearing down the daemon's uring_cmds via ublk_uring_cmd_cancel_fn(). In that window, the cancel path skips ublk_set_canceling(), so ubq->canceling stays false, even though ublk_cancel_cmd() goes on to NULL out every io->cmd. ublk_ctrl_start_dev() then proceeds to set ub->ub_disk, call add_disk(), and schedule partition_scan_work. When ublk_partition_scan_work() runs bdev_disk_changed() and the resulting read reaches ublk_queue_rq() -> ublk_queue_cmd(), the ubq->canceling check passes and the code dereferences the NULL io->cmd: BUG: kernel NULL pointer dereference, address: 0000000000000018 RIP: ublk_queue_cmd drivers/block/ublk_drv.c [inline] RIP: ublk_queue_rq+0x73/0x100 Call Trace: blk_mq_dispatch_rq_list+0x1c5/0xca0 ... bdev_disk_changed+0x3d4/0x5e0 ublk_partition_scan_work+0x89/0xe0 process_one_work+0x344/0x8a0 Fix it by always setting ub->canceling / ubq->canceling under cancel_mutex. When the disk is allocated, keep the existing quiesce/unquiesce dance so the flag is observed across the ublk_queue_rq() barrier. When the disk is not yet allocated, there is no request_queue and ublk_queue_rq() cannot be running concurrently, so simply flipping the flag is sufficient: any subsequent I/O - including the partition scan started by ublk_ctrl_start_dev() - will see canceling set and be aborted via __ublk_queue_rq_common(). Fixes: 7fc4da6 ("ublk: scan partition in async way") Signed-off-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260527144042.2095194-1-tom.leiming@gmail.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 1133b93)
The batch prepare path rereads its userspace element array when rolling back a partially prepared batch. Userspace can change an already processed tag before the second read, causing rollback to reject the replacement tag and leave earlier I/O slots prepared. The WARN_ON_ONCE() in the rollback path then fires. Copy the bounded batch into kernel memory before changing any I/O state and use the same snapshot for preparation and rollback. Commit and fetch batches retain the existing chunked userspace walk. Fixes: b256795 ("ublk: handle UBLK_U_IO_PREP_IO_CMDS") Reported-by: syzbot+1a67ee1aa79484801ec6@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=1a67ee1aa79484801ec6 Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260630211827.50475-1-alhouseenyousef@gmail.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit f01f5275feb77bac9fefbbf7cc584fe0b3850a92)
ub->completion is only re-armed by a successful START_USER_RECOVERY. If the ublk server sends END_USER_RECOVERY without one - e.g. its START failed with -EBUSY and the error was ignored - the wait is satisfied by the stale completion of the previous recovery cycle, and the device is marked LIVE and the requeue list kicked while the FETCH stream is still running and ubq->canceling is still set. The kick redispatches a previously requeued request, __ublk_queue_rq_common() sees ->canceling and parks it again via __ublk_abort_rq(), and after the last FETCH clears ->canceling nothing ever kicks the requeue list again: the request is stranded there while holding its tag. If it is the flush machinery's flush_rq, every subsequent fsync piles up in uninterruptible sleep and teardown hangs on tag draining. This matches a report of a lost PREFLUSH with ext4 on top of ublk after daemon crash recovery. ub->completion is an edge-triggered latch used as a proxy for the level condition "every queue has fetched all I/O commands", which can regress (F_BATCH's UNPREP, daemon death) and whose re-arm can be skipped. Drop it and wait on the real condition instead: the new helper ublk_wait_dev_ready_and_lock() waits on ublk_dev_ready() via wait_var_event_interruptible(), woken from ublk_mark_io_ready(), then re-checks it under ub->mutex, waiting again on regression, and returns with the mutex held and readiness guaranteed. Readiness becomes true in the same ub->mutex critical section that clears the last queue's ->canceling, so END_USER_RECOVERY marks the device LIVE and kicks the requeue list strictly after ->canceling clears. The wait stays interruptible, so a server whose daemon died can still be signalled out. For ublk_ctrl_start_dev() this replaces the fail-fast -EINVAL on an F_BATCH ready->UNPREP regression with waiting until the device is ready again. Reported-by: George Salisbury <gsalisbury@apnic.net> Fixes: 728cbac ("ublk: move device reset into ublk_ch_release()") Cc: stable@vger.kernel.org Signed-off-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260719134540.120269-1-tom.leiming@gmail.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 432a9b2780c0a01caf547bd1fc2fcf28aeb8d173)
ublk_ctrl_add_dev() memcpy()s the userspace ublksrv_ctrl_dev_info into ub->dev_info and then fixes up the fields the driver owns, but misses ->state and ->ublksrv_pid. A device added with ->state = UBLK_S_DEV_LIVE passes the "->state != UBLK_S_DEV_DEAD" test that ublk_stop_dev_unlocked() uses as its proxy for "a disk is attached", while ->ub_disk is still NULL, so DEL_DEV right after ADD_DEV oopses in del_gendisk(). UBLK_S_DEV_QUIESCED plus UBLK_F_USER_RECOVERY dies one step earlier, in ublk_force_abort_dev(). A poisoned ->state also gets START_USER_RECOVERY and the char device read/write path onto a device that was never started, and wedges START_DEV at -EEXIST. A poisoned ->ublksrv_pid just makes GET_DEV_INFO report an unrelated task as the ublk server. Reset both after the memcpy(), as ublk_detach_disk() does. Userspace only ever reads these back, so correcting them silently breaks nothing. ADD_DEV has copied ->state in unsanitized since ublk was merged, but back then it was harmless: the gendisk was allocated during ADD_DEV, and both teardown and the START_DEV -EEXIST check keyed off disk_live() rather than ->state. The oops became reachable once the disk allocation moved to START_DEV and those checks switched to ->state. Fixes: 6d9e6df ("ublk: defer disk allocation") Cc: stable@vger.kernel.org Signed-off-by: Ming Lei <tom.leiming@gmail.com> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260726145025.1507383-1-tom.leiming@gmail.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit e65848e4ce352bac9e3465099354c8b8f845391f)
import_ubuf() can fail if the address range (provided by the userspace ublk server) is outside the allowed user address space. Return that 0 bytes were copied if import_ubuf() fails rather than passing an uninitialized struct iov_iter to ublk_copy_user_pages(). Fixes: 981f95a ("ublk: cleanup ublk_copy_user_pages") Reported-by: Ming Lei <tom.leiming@gmail.com> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260729171041.45061-2-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 3831568792af75b6523fa93bb91560e29189cf55)
If the userspace ublk server passes an unmapped address as the data buffer for a completed ublk read, ublk_unmap_io() will return 0 indicating no bytes could be copied. Currently, this will result in calling blk_update_request() with nr_bytes=0, which doesn't seem supported. Fail the I/O with BLK_STS_IOERR in this case instead. Fixes: 71f28f3 ("ublk_drv: add io_uring based userspace block driver") Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260729171041.45061-3-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 24fd3706178f1ae5501fd1ff9036e170ed0665ba)
io->res is set from struct ublksrv_io_cmd's result field, which is controlled by the ublk server process, without any validation. It's thus possible for userspace to trigger the io->res > rq_bytes warning. ublk_copy_user_pages() already limits the copy length to the request data length, so drop the warning. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260729171041.45061-4-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 15c1339ef44054cc79c49b40b24a26aa80a2253e)
The u16 nr_hw_queues and queue_depth fields of the ublk UAPI struct ublksrv_ctrl_dev_info constrain the number of queues and queue depth of each ublk device. However, the ublk driver is a bit inconsistent with the type it uses to represent these values, mixing u16 with int and unsigned int. Change all queue number, queue depth, q_id, and tag variables/fields to u16 to save some space. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260803211441.2538144-2-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit f510198855b6ddac0ffe62684dc407c56d3e0f24)
struct ublk_zoned_report_desc's operation field is only ever set to UBLK_IO_OP_REPORT_ZONES, so remove it. Replace its one load with the constant. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260803211441.2538144-3-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 3a00b782a7b63b1876261ab883e1d8e1aa0e09fb)
In preparation for moving the struct ublksrv_io_desc initialization from
the thread submitting ublk requests to the daemon thread receiving them,
split the fallible part of ublk_setup_iod{,_zoned}() into new helper
ublk_validate_req{,_zoned}(). Only ublk_setup_iod{,_zoned}() accesses
the io_desc and cannot error out.
Return a bool value from ublk_validate_req{,_zoned}() as the existing
error code ublk_setup_iod{,_zoned}() returns is only checked against
BLK_STS_OK.
Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
Link: https://patch.msgid.link/20260803211441.2538144-4-csander@purestorage.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(cherry picked from commit 8c76625ff9360cbe9ce0c47a624877aea14b3499)
ublk_setup_iod() is currently called to populate struct ublksrv_io_desc on the thread submitting I/O to a ublk device. However, only the ublk server threads read the io_descs. This basically guarantees a cache miss on both threads for each ublk I/O. There's really no need to initialize the io_descs on the submitting thread. Move the ublk_setup_iod() call to ublk_dispatch_req() (for non-UBLK_F_BATCH_IO) and __ublk_batch_prep_dispatch() (for UBLK_F_BATCH_IO), which runs on the ublk server daemon thread before dispatching the I/O to userspace. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260803211441.2538144-5-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 735409f58b3da45094f2dfd7c18fb9e1937431f1)
ublk passes the parameters of incoming I/O in memory shared between the kernel ublk driver and userspace ublk server in struct ublksrv_io_desc. The size of this struct is currently fixed to 24 bytes, which has been an obstacle to extending it with additional fields [1]. Additionally, with multiple ublk server threads handling I/Os from the same ublk queue (possible with UBLK_F_PER_IO_DAEMON or UBLK_F_BATCH_IO), false sharing results from adjacent io_descs sharing the same cache line. Add a ublk feature UBLK_F_IO_DESC_SIZE to allow a ublk server to override the size of each io_desc. The size must be at least 24 and a multiple of 8 to store a properly-aligned struct ublksrv_io_desc. It's also limited to a maximum of 256, though this bound could be lifted in the future. The struct ublksrv_io_desc is located at the beginning of each io_desc and the remainder is padding. The mmap() performed for each queue must have a length of queue_depth * io_desc_size rounded up to the page size. The mmap() offset must be q_id * UBLK_MAX_QUEUE_DEPTH * io_desc_size, also rounded up to the page size. [1]: https://lore.kernel.org/linux-block/aV8QfvaNO5P6vOs6@fedora/ Suggested-by: Ming Lei <ming.lei@redhat.com> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260803211441.2538144-6-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 5c0958d80190822c4614ca00478d9acd9671bba6)
Add an optional --io_desc_size argument to the kublk add/recover commands to enable UBLK_F_IO_DESC on the ublk device. The mmap() arguments and ublk_get_iod() computation are adjusted accordingly. Display the configured io_desc_size in the kublk list output for ublk devices with UBLK_F_IO_DESC. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260803211441.2538144-7-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit fc01b96d74b3a9eec2b558d49ec9f5176473766a)
Add test loop_08, which creates a ublk device with UBLK_F_IO_DESC_SIZE enabled and io_desc_size set to 64. The test issues verified I/O to the device using fio. Signed-off-by: Caleb Sander Mateos <csander@purestorage.com> Link: https://patch.msgid.link/20260803211441.2538144-8-csander@purestorage.com Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit d61d0f95e686be015cfaf193c0ae15156d1a0cc4)
ublk_map_io() and ublk_unmap_io() are no-ops for ublk devices that
enable user copy or zero copy, as well as for requests without data to
copy in the given direction. However, the implementation is a bit
convoluted, returning the full request data length and relying on the
caller to check the return value against the request length.
UBLK_F_SHMEM_ZC recently added branches to skip the ublk_{,un}map_io()
call for I/Os using a shared-memory buffer. This is a more logical place
for the device need_map and the ublk_need_{,un}map_req() checks, so move
them there from ublk_{,un}map_io().
Checking these conditions early also skips the expensive pointer-chasing
for the ublk_iod_is_shmem_zc() check in __ublk_complete_rq() for the
common case of a ublk device using user copy or zero copy.
Drop the req_op() filter in __ublk_complete_rq(), as it's redundant with
the ublk_need_unmap_req() check.
Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
Link: https://patch.msgid.link/20260803211441.2538144-9-csander@purestorage.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(cherry picked from commit a8a79eba22dc4c11f2877bcf9e8557f6d95541ac)
With UBLK_F_AUTO_BUF_REG, invalid sqe->addr can fail after ublk_fill_io_cmd() has set UBLK_IO_FLAG_ACTIVE. The uring_cmd is completed while the tag stays active, which can hang teardown. Split validation from buffer apply so the check has no side effects, then take the uring_cmd and store the already-validated buffer. Apply the same order in FETCH so io->buf is not written before __ublk_fetch() state checks. Fixes: 52460dd ("ublk: move auto buffer register handling into one dedicated helper") Suggested-by: Caleb Sander Mateos <csander@purestorage.com> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn> Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit ca5a01eee34c7cbe0f531a613b0292a3ad1a419b)
Batch AUTO_BUF_REG COMMIT must unregister the old auto_buf index before storing the next one. Fixed per-tag indexing (A == B) masks bugs that clear after overwriting io->buf. Add kublk --rotate_auto_buf so each tag alternates between two sparse buffer indices, and test_batch_04.sh to exercise that path. Without the driver fix, the request ref stays stuck and I/O hangs; the test uses a short timeout and kills the ublk daemon to recover. With the fix, a small write completes quickly. Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn> Reviewed-by: Ming Lei <tom.leiming@gmail.com> Link: https://patch.msgid.link/20260730024050.1062354-1-yangxiuwei@kylinos.cn Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit d507d3cb19e71f10336e41deeac517cdc21c34ea)
ublk_batch_commit_io() stored the new auto_buf into io->buf before calling ublk_clear_auto_buf_reg(). Clear takes the unregister index from io->buf.auto_reg, so it could drop the new slot and leave the old registered buffer behind. Fixes: 1e500e1 ("ublk: handle UBLK_U_IO_COMMIT_IO_CMDS") Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn> Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit af0955c8f26aa0f02e534084bc9948a69c9e7ce0)
UBLK_F_ZONED uses params.basic.chunk_sectors as zone size. ublk uses ilog2(chunk_sectors) to get number of zones, so the value must be power of 2. If chunk_sectors is 96 and dev_sectors is 96 * 16, userspace asks for 16 zones. But the shift calculation gets 24 zones. Block layer rejects such zone size when the disk is started. But SET_PARAMS has already returned success, which is confusing for userspace. Reject it in SET_PARAMS with other zoned parameter checks. Fixes: 29802d7 ("ublk: enable zoned storage support") Signed-off-by: Yao Sang <sangyao@kylinos.cn> Link: https://patch.msgid.link/20260814023226.354288-2-sangyao@kylinos.cn Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 2707acf1856da266139986c9398ad722fd4f48c0)
The normal kublk add command goes through device startup. It does not tell the shell test whether a bad parameter is rejected by SET_PARAMS or later by START_DEV. Add a set_params command. It creates a temporary ublk device, sends SET_PARAMS with the command line parameters, returns the ioctl result, and deletes the device before START_DEV. Signed-off-by: Yao Sang <sangyao@kylinos.cn> Link: https://patch.msgid.link/20260814023226.354288-3-sangyao@kylinos.cn Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 176f02a86c0384df4ba175e319321b07089a8016)
Add test_params_01.sh for SET_PARAMS. The test checks valid basic parameters and several invalid parameter cases. Also cover zoned parameters, including a non-power-of-2 zone size. This case must fail in SET_PARAMS instead of being accepted and rejected later when the device is started. Signed-off-by: Yao Sang <sangyao@kylinos.cn> Link: https://patch.msgid.link/20260814023226.354288-4-sangyao@kylinos.cn Signed-off-by: Jens Axboe <axboe@kernel.dk> (cherry picked from commit 007af5e5cc822aed3a0909534ffb810f7816ed1f)
__ublk_shmem_remove_ranges() removes matching maple tree ranges in
batches, but first stores each range into a temporary xarray so that the
pages can be unpinned after dropping the maple tree lock.
That temporary xarray is filled under the maple tree lock with
xa_store(..., GFP_ATOMIC). If the store fails before mas_erase(), the
current range is left in the tree and the helper returns false. The
outer ublk_shmem_remove_ranges() loop then immediately retries the same
range. While the atomic allocation keeps failing, the teardown path has
no forward progress.
The issue can be reproduced with radix_tree_node failslab injection after
a SHMEM_ZC buffer has already been registered:
# Kernel config:
# CONFIG_BLK_DEV_UBLK=y
# CONFIG_DEBUG_FS=y
# CONFIG_FAULT_INJECTION=y
# CONFIG_FAULT_INJECTION_DEBUG_FS=y
# CONFIG_FAILSLAB=y
echo 10 > /proc/sys/vm/nr_hugepages
mkdir -p /tmp/htlb
mount -t hugetlbfs none /tmp/htlb
fallocate -l 4M /tmp/htlb/ublk_buf
dev_id=$(kublk add -t null --shmem_zc \
--htlb /tmp/htlb/ublk_buf |
awk -F '[ :]' '/dev id/ {print $3}')
echo 1 > /sys/kernel/slab/radix_tree_node/failslab
echo Y > /sys/kernel/debug/failslab/cache-filter
echo Y > /sys/kernel/debug/failslab/ignore-gfp-wait
echo 1 > /sys/kernel/debug/failslab/interval
echo -1 > /sys/kernel/debug/failslab/times
echo 100 > /sys/kernel/debug/failslab/probability
kublk del -n "$dev_id"
On the unfixed kernel the delete command was still running after 3
seconds. Disabling failslab made it return. The fault-injection stack
showed:
should_failslab
kmem_cache_alloc_lru_noprof
__xas_nomem
__xa_store
xa_store
__ublk_shmem_remove_ranges
ublk_cdev_rel
ublk_ctrl_del_dev
Remove the allocation from the teardown loop. Keep the existing batch
limit, but collect {base_pfn, nr_pages} pairs in a fixed-size stack array.
Once a matching range is found, the range is erased from the maple tree
before dropping the lock, so each successful scan makes progress without
depending on any GFP_ATOMIC allocation.
With the same failslab settings, the fixed kernel completed
"kublk del -n $dev_id" successfully in about 45 ms.
Fixes: 309e02d ("ublk: avoid unpinning pages under maple tree spinlock")
Signed-off-by: Yao Sang <sangyao@kylinos.cn>
Reviewed-by: Ming Lei <tom.leiming@gmail.com>
Link: https://patch.msgid.link/20260804125736.2011774-1-sangyao@kylinos.cn
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(cherry picked from commit 4fd66a7f829f3f38f92a79081f0f2688aed644f0)
UBLK_CMD_GET_FEATURES returns UBLK_F_ALL, which lists UBLK_F_ZONED unconditionally. Built with CONFIG_BLK_DEV_ZONED=n the driver then refuses the device it just advertised: ublk_ctrl_add_dev() returns -EINVAL for a zoned request, and ublk_dev_param_zoned_validate() returns -EOPNOTSUPP. A ublk server has no other way to ask, so it takes the feature bit at its word and fails at ADD_DEV. test_params_01.sh does exactly that: it gates its zoned cases on the reported feature, so on such a kernel the first case fails and the remaining ones pass only because creating the device fails before any parameter is validated. Clear the bit in the reported feature set. UBLK_F_ALL itself is left alone: ublk_ctrl_add_dev() masks dev_info.flags with it before testing for a zoned device, so removing the bit from the mask would drop the request instead of refusing it, and hand back a non-zoned device. Fixes: 29802d7 ("ublk: enable zoned storage support") Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
The driver's own state cannot be read from userspace. The control
device reports what was configured, not what the driver is doing with
it, and a device that is being torn down is unreachable through both
/dev/ublkcN and the control device while its state is exactly what is
wanted.
Add a generic inspection point under ublk/<dev_id>/:
dev_state device state and flags, the per queue flags, and the
device reference count
tags one line per tag in use - io->flags, the parked command,
the request handed to the ublk server, the reference
counts, the serving task, and whether the request is
started
Tag state is copied under io->lock and printed outside it.
A device outlives its deletion whenever something still holds it, and
that is the state worth reading, so ublk_cdev_rel() removes the
directory at the final put rather than ublk_remove() at deletion. That
removal runs before the queues and the tag set the files report on are
freed, and debugfs_remove_recursive() waits out readers already inside
a file operation and refuses any that start later, so no reader can
reach a device that is going away.
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
ublk/<dev_id>/dev_state and ublk/<dev_id>/tags are the only view of per-tag state from userspace, and the teardown cases added later take their verdict from them. Cover the files themselves first: that they report the geometry the device was created with, that tags stays readable while ublk_debugfs_tags_show() walks state the IO path is changing underneath, and that the directory goes away with the device. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
A device that will not go away says nothing about how far its teardown got. The end state is all that can be read, and it does not answer whether the character device was ever released, whether the queued task work ran, or whether a parked command was ever offered to cancellation. Record it. ublk/<dev_id>/teardown lists the lifecycle steps a device reached, and counts the events that only make sense compared against each other: task work queued against task work run, and cancellation calls against what they did. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
ublk_batch_cancel_queue() sets ubq->force_abort so that no further batch
fetch command attaches while the queue is being cancelled. Recovery resets
the queue through ublk_queue_reset_io_flags(), which clears ->canceling and
->fail_io but leaves ->force_abort set.
A UBLK_F_BATCH_IO device that went through UBLK_CMD_QUIESCE_DEV therefore
cannot be recovered. END_USER_RECOVERY reports success and the device goes
LIVE, but the recovered server's UBLK_U_IO_FETCH_IO_CMDS is refused by
ublk_batch_attach(), which tests ->force_abort together with ->canceling.
Its IO threads exit at once, the server dies, and a UBLK_F_USER_RECOVERY
device falls straight back to QUIESCED:
state: QUIESCED
ublksrv_tgid: -1
queue 0: depth 64 canceling 1 force_abort 1 fail_io 0 nr_io_ready 0
Clear it with the rest. The classic cancel path never sets ->force_abort per
queue, which is why only batch devices are affected; the other setters are
teardown paths where the device is going away.
Fixes: a4d8837 ("ublk: add UBLK_U_IO_FETCH_IO_CMDS for batch I/O processing")
Cc: stable@kernel.org
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
ublk_wait_for_idle_io() returns immediately for UBLK_F_BATCH_IO, so QUIESCE_DEV runs its single cancel pass against whatever the queues hold at that moment, and the server learns about the quiesce only if an fcmd was cancellable there. Every quiesce case so far creates a non-batch device, so neither that early return nor ublk_batch_cancel_queue() behind it has been covered. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
del_gendisk() waits for requests that were started, and teardown looked for them by scanning io->flags for UBLK_IO_FLAG_OWNED_BY_SRV. That scan has to enumerate every state a tag can hold, and it reads the request out of the cmd/req union, which is only valid for the state it just tested. Walk blk_mq_tagset_busy_iter() instead. Only started requests reach the callback, so the set teardown ends is the set del_gendisk() waits for, the request comes from the tag rather than the union, and a state that holds no request needs no arm in the walk. A tag being dispatched is owned by the dispatcher for that whole interval and is skipped: it ends its own request once it observes ->canceling. Anything the walk finds started but unowned is a bug rather than work to do, and stalls the freeze that follows. The iterator covers the whole tag set, so the per-queue ublk_abort_queue() gives way to ublk_abort_dev(), which drains the batch event fifos and then walks once. The scan being removed here is the one "ublk: serialize tag state and command claims" takes io->lock across earlier in this series. Both are needed on their own: that commit closes the fill-versus-cancel race on every tag and applies without this one, while this commit is about which requests del_gendisk() is left waiting for. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io> Fixes: 71f28f3 ("ublk_drv: add io_uring based userspace block driver") Cc: stable@kernel.org
io_uring_prep_timeout() records only the pointer to the timespec, and ublk_fault_inject_queue_io() puts it on its own stack. The kernel reads it when the event loop submits, after that frame is gone. The stale bytes parse as an expired timeout, so --delay_us never delays and test_generic_06.sh fails: dd completes before the daemon is killed. A delay above 999999us could not work either, being stored in tv_nsec alone. Fixes: 8158665 ("selftests: ublk: add generic_06 for covering fault inject") Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
blk_mq_tagset_busy_iter() must end exactly the set del_gendisk() waits for, and a request the walk finds started but unowned stalls the freeze that follows. Every existing delete case runs a null device, which completes at once and rarely has a request with the ublk server when the delete lands. 1M writes to a backing file keep requests there, and the batch and auto_zc arms go through the same walk, which covers the whole tag set rather than a queue at a time. A backing file only makes the state likely. The fault_inject target makes it certain: it answers each IO with a timeout, so every fetched tag stays OWNED_BY_SRV for as long as the delay lasts and the delete meets a full queue of them. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
io_uring runs pending task work with io_tw_token_t.cancel set when the ring goes away, so the driver can finish what it queued. ublk_batch_tw_cb() ignored the token and went on to dispatch. A fetch command is ubq->active_fcmd while its task work is outstanding, and ublk_batch_cancel_cmd() deliberately skips the active one, so a command left parked in that run is stranded: it keeps referencing the ublk char device and UBLK_U_CMD_DEL_DEV waits on ublk_idr_wq forever. Give back the queued tags and unwind the fetch command through ublk_batch_deinit_fetch_buf(), which is already the "nothing can move on" path. Report UBLK_IO_RES_ABORT, as ublk_batch_cancel_cmd() does for the same command: the ublk server decides whether to fetch again by comparing against it. Fixes: a4d8837 ("ublk: add UBLK_U_IO_FETCH_IO_CMDS for batch I/O processing") Cc: stable@kernel.org Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
A ublk server that goes away leaves the device live with no STOP_DEV ever sent, and io_uring then runs the task work it queued with cancel set. For UBLK_F_BATCH_IO that run is the only chance the fetch command gets: it is ubq->active_fcmd, which ublk_batch_cancel_cmd() skips, so a command left parked there keeps referencing /dev/ublkcN and UBLK_U_CMD_DEL_DEV waits on ublk_idr_wq for ever. kublk had no shutdown of its own, so nothing could reach that ordering. The teardown modes added here run on a sigwait() thread, because ublk_ctrl_stop_dev() submits on the control ring and is not async-signal-safe; later cases take the same route. Whether task work is outstanding at the instant the ring dies is not something the test can arrange, so it kills at a different point each cycle and takes many cycles rather than one. --die_during_fetch is not an alternative: the fault_inject target raises SIGKILL only for tag 1, and the batch path calls pre_fetch_io once per queue with tag 0, so a batch server never dies there. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
Without recovery, ublk_stop_dev() entered del_gendisk() while requests
could still be owned by the live server:
ublk_stop_dev()
del_gendisk()
waits for an OWNED_BY_SRV request
the request is only failed once the server is gone
Publish ->canceling and fail those requests before del_gendisk() starts
waiting, by calling ublk_abort_dev() there too. It now publishes
->canceling through ublk_start_cancel(), so the char device release path
gains the quiesce that ublk_set_canceling() asks its callers for.
ublk_cancel_dev() stays outside ub->mutex and ub->cancel_mutex:
->cancel_fn() takes cancel_mutex under ->uring_lock, and the submission
path takes ub->mutex under it, so completing a command from under either
mutex inverts the order. del_gendisk() waits for requests, and a parked
command holds none, so the cancel is not needed before it.
Fixes: 71f28f3 ("ublk_drv: add io_uring based userspace block driver")
Cc: stable@kernel.org
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
del_gendisk() waits for started requests, and only the server ends a request it owns, so teardown has to fail those itself before it gets there. Nothing in the suite held a request across a delete, so the ordering was never exercised. --hold_io stops the server completing what it fetched, leaving the tag UBLK_IO_FLAG_OWNED_BY_SRV. The test asserts that state through debugfs before deleting: a delete that finds no owned request proves nothing. Unlike the other teardown cases this needs no race - the server simply never commits - so a single run either reproduces or does not. Replaces selftests-cmd-path, which iterated copy modes on healthy devices and never reached the window. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
A request that ublk_abort_dev() fails to dispose of blocks del_gendisk() in blk_mq_freeze_queue_wait() forever, holding ublk_ctl_mutex and ub->mutex, so every later control command wedges behind it. Nothing in dmesg names the tag, and io->flags is not reachable from userspace once the device is being torn down. ublk/<dev_id>/tags carries the same state, but only for someone already watching on a CONFIG_DEBUG_FS=y kernel; pr_warn() reaches a bug report and a test run's log. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
ublk_queue_cmd_list() handed a chain of requests spanning several tags to
one carrier command's task work, and ublk_cmd_list_tw_cb() walked it by
rq_next. Nothing but that callback could reach those requests, so
teardown has no way to dispose of a dispatch whose task work never runs.
Put the requests on a per-queue list instead, the way UBLK_F_BATCH_IO
already queues tags on evts_fifo, and let the task work take them off it.
A queued request then belongs to whoever unlinks it under disp_lock.
A task work only takes an entry when it may dispatch it: the tag has to
belong to the running task, so UBLK_F_PER_IO_DAEMON keeps dispatching
every tag from its own daemon, and the tag must have no task work of its
own queued, because dispatching hands its command over and only the
callback queued on a command may complete it. Track the latter with
UBLK_IO_FLAG_CMD_TW_PENDING.
For the same reason a batch no longer spans queues, as each queue has its
own list.
The two disposal paths belong in the same commit as the list itself,
because a list nobody can drain strands the requests on it:
- teardown takes the queued requests back in ublk_abort_dev(), as is
already done for the batch event fifo. A request whose dispatch task
work never runs otherwise stays started for ever, so
blk_mq_freeze_queue_wait() in del_gendisk() never returns, and
ublk_stop_dev() holds ublk_ctl_mutex and ub->mutex across that wait --
one such device wedges every other control command on the machine:
ublk_warn_started_rq: dev 0 qid 1 tag 63 still started: io_flags 5
- io_uring runs pending task work with io_tw_token_t.cancel set when the
ring goes away, in io_cancel_local_task_work() from
io_ring_exit_work(), so the driver can finish what it queued. That run
is the only chance the command gets: cancellation refuses a command
with a dispatch in flight, so a command left parked there is stranded,
keeps referencing the ublk char device, ublk_ch_release() never runs,
and UBLK_U_CMD_DEL_DEV waits on ublk_idr_wq for ever. Give back the
queued requests and complete the command, the way
fuse_uring_send_in_task() does. Every entry is taken there rather than
only this task's, since no dispatch follows and one left behind would
strand its request as well.
Cancellation keeps its UBLK_IO_FLAG_DISPATCHING test, taken under
io->lock, so a tag that ublk_prep_req() has taken for dispatch is still
held against cancellation. But teardown clears that flag on the requests
it takes back, so it no longer covers a dispatch whose task work is still
queued. Add the started request as a second test, in the same locked
section.
UBLK_IO_FLAG_CMD_TW_PENDING must not be used for this: only
ublk_cmd_tw_cb() ever clears it, and a task work armed on a ring that is
going away is never run. The tag then stays ACTIVE with the flag set,
ublk_cancel_cmd() refuses it for ever and io_ring_exit_work() spins on it
at 100% of a core:
io_ring_exit_work -> io_uring_try_cancel_requests
-> io_uring_try_cancel_uring_cmd -> ublk_ch_uring_cmd
Measured with per-tag counters: for every stranded tag the command had
been armed exactly one more time than the callback had run, and it never
caught up. A started request instead un-starts when teardown ends it, so
that test lifts whether or not the task work is ever run.
UBLK_IO_FLAG_CMD_TW_PENDING still marks which entries a task work may
take in ublk_take_dispatch_list().
ublk_queue_cmd_list() also has to take the command under the same
ublk_io_lock() that marks the task work pending. ublk_prep_req() checks
->canceling, but nothing holds that gate until the command is consumed:
teardown aborting any request of the batch calls ublk_undo_dispatch(),
which clears UBLK_IO_FLAG_DISPATCHING, and cancellation then clears
->cmd under the submitter, which arms task work on NULL:
BUG: kernel NULL pointer dereference, address: 000000000000004b
RIP: 0010:__io_uring_cmd_do_in_task+0x9/0x30
Call Trace:
ublk_queue_rqs+0x12e/0x1c0 [ublk_drv]
blk_mq_dispatch_queue_requests+0xca/0x360
blk_mq_flush_plug_list+0x177/0x1c0
Taking it under the lock leaves cancellation either before us, so the
command is already gone and the requests go to ublk_abort_dispatch_queue()
instead, or after us, where it finds the tag no longer idle. That helper
pops under disp_lock, so a request is disposed of by whoever pops it and
teardown draining concurrently cannot dispose of it twice.
Squashed from three patches ("queue classic dispatches on a per-queue
list", "abort dispatches still queued at teardown", "complete the command
when task work runs in cancel mode"). Testing showed the first two are
not usable apart: with only the list and the teardown drain, generic/002,
generic/004, generic/005 and generic/006 of the ublksrv test suite
(https://github.com/ublk-org/ublksrv) all fail; the cancel-mode
completion is what brings 004, 005 and 006 back. Splitting them again
would put a commit into the tree that hangs every device-delete test and
wedges the control path.
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
Requests queued on ubq->disp_list are reachable only from the task work armed on the tag's command, so when the ring goes away that one cancel-mode run is the only chance they get. A run that dispatches instead of giving them back leaves them started, del_gendisk() waits for them, and ublk_stop_dev() holds ublk_ctl_mutex across that wait. The batch case covers this from the fetch command's side; this covers the classic path, including the per-io-task arm where a request can sit on a list its own task work is not the one to drain. A server started with --abandon_ring closes its rings on the signal but keeps /dev/ublkcN open and parks in pause(), so it has to be killed from outside, and before the delete rather than after it: DEL_DEV waits for the last reference on the char device to go. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
UBLK_CMD_DEL_DEV waits for the device number to leave the idr, and that only happens in ublk_cdev_rel(), on the last reference to the char device. A ublk server holds those references itself: every parked command is an io_uring request pinning the ublkc file. So a server that deletes its own device can wait forever. It has to exit for the references to go, and it cannot exit while the delete has not returned: ublk_ctrl_del_dev+0x117 [ublk_drv] ublk_ctrl_uring_cmd+0x2b3 [ublk_drv] Nothing about the device is unfinished at that point. ublk_remove() has stopped it, taken the requests back, removed the disk and completed every parked command, and ublk_start_cancel() has already made it impossible to park another one. Free the number there instead, and let the rest of the device go when its last reference does. The number becomes reusable while the device is still there, so ublk/<dev_id>/ cannot keep that name: ublk_remove() moves the device to ublk/stale/<n>, and ublk_cdev_rel() removes it at the final put. A directory under stale/ is therefore a device that was deleted and never freed. Fixes: 71f28f3 ("ublk_drv: add io_uring based userspace block driver") Cc: stable@kernel.org Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
Every delete in the suite comes from a process that holds no reference on the device, so none of them can wait on themselves. A server that deletes its own device does: each command it has parked is a reference on its own ublkc file, and those only go when it exits, which it cannot do until the delete returns. The verdict comes from three places, because the failure shows up in different ones -- a server that never exits, a /dev/ublkcN that outlives it, and a device number the next add cannot have. kublk gains --self_del to reach the ordering at all. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
struct ublk_io kept struct io_uring_cmd *cmd and struct request *req in a union. Which of the two is in use is told by the flags UBLK_IO_FLAG_ACTIVE and UBLK_IO_FLAG_OWNED_BY_SRV. The two are never in use at the same time, so the union is not wrong, but reading the right member depends on checking the right flag, and there is no enforcement and no sanity check. Check the wrong flag, or none, and one gets a pointer to the other type that still looks valid. Earlier commits fixed two instances of that same mistake: - "ublk: don't compare a tag that holds no command" ublk_belong_to_same_batch() read ->cmd without checking ACTIVE, and cancellation cleared ->cmd while leaving ACTIVE set - "ublk: end server-owned requests from the tagset" teardown read ->req for a state it had just tested Split them into two fields and clear the one being left behind, in __ublk_prep_compl_io_cmd() and ublk_fill_io_cmd(). The flags work as before; what changes is that reading the wrong field now gives NULL, so a mistake crashes right there instead of quietly using the wrong type. struct ublk_io is ____cacheline_aligned_in_smp and had padding to spare, so this costs no memory: the size stays 128 bytes here. This is a hardening patch; it fixes no known bug on its own. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
Which of io->cmd and io->req is in use is told by io->flags, and reading the wrong one now gives NULL rather than a pointer of the other type that still looks valid. That is exactly what ublk/<dev_id>/tags prints, so the invariant can be read straight off a running device. Sampling while fio runs walks the tags through every state they take, rather than catching them idle where only one field is ever set anyway. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
Every teardown case creates one device, tears it down and only then moves on, so nothing covers a control command arriving from an unrelated process while an abort walk is running. ublk_stop_dev() holds ublk_ctl_mutex across del_gendisk(), so a teardown that does not finish blocks every device on the machine and not only its own. The verdict comes from three places, because a leak shows up in different ones: a bounded add that never returns, a server or char device that outlives its device, and a driver use count that does not fall back to what it was before the device existed. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
The teardown cases all delete from an unrelated process, so the stop always arrives from outside. A server that stops its own device while its queues are still live is the other ordering a real server picks: it tells the driver first and leaves its rings only once the queues drained. The device is expected to go without any help -- the server exits on its own, an add from another process does not queue behind it, and the delete that follows does not wait. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
__ublk_fail_req() leaves the request on its last reference rather than
completing it, so whoever drops that reference does the disposition. For
an auto registered buffer the reference is held by io_uring and drops
only when the buffer is unregistered.
ublk_abort_dev() takes the tag from a live ublk server, clearing
UBLK_IO_FLAG_OWNED_BY_SRV. The server's next UBLK_IO_COMMIT_AND_FETCH_REQ
is then refused with -EBUSY before reaching ublk_clear_auto_buf_reg(), so
the buffer is never unregistered and the reference never drops:
ublk_warn_started_rq: dev 0 qid 0 tag 0 still started:
io_flags 10 ref 1 task_bufs 0
auto_reg info 1 task_registered_buffers = 1
handover granted 1 server owns the tag
set_canceling
fail_req info 0 not completed, ref left at 1
del_gendisk() then waits in blk_mq_freeze_queue_wait() until the server
exits, which is what finally lets io_uring unregister the buffer.
ublk_ch_release_work_fn() has no such problem: it waits out active
references in ublk_check_and_reset_active_ref() first.
Unregister the buffer on the refused command. The refusal means the tag
is gone, and the server's io_ring_ctx is the only one that can release
what it registered.
Fixes: 5b0f1545e407 ("ublk: hold a request reference during daemon copies")
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
The cherry pick to 7.1-niova differs in respect to the unregister
call name: 7.1 exports io_buffer_unregister_bvec(). 51e08eaf954d
("io_uring/rsrc: rename io_buffer_register_bvec()/io_buffer_unregister_bvec()")
renamed it, and is not picked because it changes io_uring core.
Tag ownership is encoded in three independently set bits - ACTIVE,
DISPATCHING and OWNED_BY_SRV - written at about twenty sites. The
comments where they are defined describe a state machine ("Exclusive
with UBLK_IO_FLAG_ACTIVE", "Set alongside UBLK_IO_FLAG_ACTIVE"), but no
code enforces it, so a combination nobody intended is reached silently
and only shows up later, somewhere else.
Record the ownership state beside the flags and check it wherever it
changes. The flags still decide everything; ->state is only asked
whether a transition started where the caller believes it did, and a
mismatch names the tag, both states and the flags.
Callers that race another taker record without checking, because losing
that race is normal and already arbitrated by UBLK_IO_FLAG_CANCELED.
UBLK_F_BATCH_IO dispatches through the queue's fetch command rather than
a parked one, so its dispatch transition is recorded, not checked.
This is hardening. It found no bug in the paths it now covers; it makes
the next one report itself where it happens.
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
Debug aid for the quiesce race; not for submission.
UBLK_CMD_QUIESCE_DEV can leave one tag holding a parked command that
nothing completes, so the server never exits and the device stays LIVE:
tag 21 flags 0x00000001 ACTIVE cmd ffff888162091000 req 0 started 0
Which path put it there cannot be read off the state, because every
skip in ublk_cancel_cmd() delegates to a different owner and a last
value cannot show which side got there first.
Record the steps instead:
- struct ublk_tag_evt, a ring of 16 per tag, stamped from one
device-wide counter, written under io->lock
- the last park kept out of the ring, which a requeue loop can age
it out of
- set_canceling_seq and the cancel_dev walk's start and end, so a
tag's history can be placed against cancellation
- cancel_skip_dispatch_parked: the walk skipped a tag that was
holding a command, which is a tag it will not come back to
- the ring is dumped for tags left ACTIVE with a command, the shape
a stranded tag has
Two knobs widen one window each, in microseconds, default off, and
only while the queue is canceling, which is the only time the walk can
land in either. Both make the walk skip a tag holding a command:
- delay_prep_cancel_us: between ublk_prep_req() marking the tag and
__ublk_queue_rq_common() testing ->canceling. __ublk_abort_rq()
ends that window and settles no command.
- delay_park_check_us: between the command being parked by
UBLK_IO_COMMIT_AND_FETCH_REQ and ublk_check_canceling() testing
->canceling, which does take the command back.
Only the first should be able to strand a tag.
Everything compiles out without CONFIG_DEBUG_FS.
Assisted-by: Claude Opus 5
Signed-off-by: Bernd Schubert <bernd@niova.io>
The cherry pick to 7.1-niova differs in respect to one context line:
7.1 exports io_buffer_unregister_bvec(). 51e08eaf954d ("io_uring/rsrc:
rename io_buffer_register_bvec()/io_buffer_unregister_bvec()") renamed
it, and is not picked because it changes io_uring core. What this patch
adds is unchanged.
UBLK_IO_FLAG_ACTIVE is documented as "the io command is owned by ublk driver", and UBLK_IO_FLAG_OWNED_BY_SRV as "Exclusive with UBLK_IO_FLAG_ACTIVE: the command has been handed over, so io->cmd is NULL". Both are stated where the flags are defined; neither is checked. Callers rely on them. ublk_belong_to_same_batch() tests ACTIVE and then passes io->cmd to io_uring_cmd_ctx_handle(), which dereferences it, and __ublk_fetch() refuses a re-fetch on ACTIVE alone. A tag carrying ACTIVE without a command therefore reaches code that cannot cope with it, and cancellation did produce that combination. Check the pair wherever it changes, under the lock that owns it, and report the tag rather than only that something was wrong. This is hardening: the combination is reachable, and nothing said so. UBLK_F_BATCH_IO dispatches through the queue's fetch command instead of a parked one, so its tags are ACTIVE without io->cmd and are skipped, as __ublk_fail_req() already skips them for the same reason. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
test_generic_17.sh kills the recovery server mid-fetch and expects it to die of SIGKILL. Whether it does is a race: kublk posts its ready semaphore in __ublk_io_handler_fn() right after ublk_thread_init(), so the main thread runs ahead to END_USER_RECOVERY while the queue thread is still walking tags towards the tag 1 that raises SIGKILL. The queue thread almost always wins, and the case where it loses is the one worth testing. Add --die_during_fetch_delay_us to let the kill be held back until END_USER_RECOVERY is in the kernel, and a test that asserts the recover command still exits. Default 0 keeps generic_17 as it was. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
The END_USER_RECOVERY deadlock needs the io-wq worker to reach ublk_wait_dev_ready_and_lock() with SIGKILL already dequeued, which is what get_signal() does for a PF_USER_WORKER before io_wq_worker() drains the rest of its queue. Winning that race from user space is not practical: the submit has to land between the kill being queued and the worker draining. Emulate the precondition instead. With the knob set the wait drops the pending signal itself, which is deterministic and exercises exactly the state the deadlock needs. Debug only, drop before submission. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
A ublk server that dies before every queue has fetched all its I/O commands can deadlock its own exit. END_USER_RECOVERY and START_DEV both wait in ublk_wait_dev_ready_and_lock() for ublk_dev_ready(), which needs nr_io_ready == q_depth on every queue. Only the server can FETCH, so once it is gone the condition is unreachable. Both commands may sleep, so ublk_ctrl_uring_cmd() returns -EAGAIN for the inline issue and io_uring always runs them from an io-wq worker. That worker is in the server's own thread group, so do_exit() of the last user thread waits for it in io_wq_put_and_exit() while it waits for a readiness that only that thread group could deliver. The wait is interruptible, which is what was meant to break this. It does when the signal arrives while the worker already sleeps in the wait. It does not when the worker picks the work up afterwards: get_signal() dequeues SIGKILL for a PF_USER_WORKER and returns to the worker instead of exiting it, and io_wq_worker() then drains the remaining work with nothing pending. The wait sleeps with no signal to end it and nothing to wake it - the only wake_up_var() on ->nr_queue_ready is on the ready path, and readiness only regresses here. Test the group exit state instead of relying on the signal. It is set in complete_signal() before zap_other_threads() wakes anyone, and is not cleared when the signal is dequeued, so a worker entering the wait at any point after the server started dying sees it and gives up. Found by test_generic_17.sh, which hangs rather than failing: the task is stuck in D state, so its own kselftest timeout cannot kill it. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
ublk_batch_dispatch() checks ublk_io_evts_empty() twice without ubq->evts_lock, while every producer bumps the fifo under it - kfifo_put() in ublk_batch_queue_cmd(), kfifo_in() in ublk_batch_queue_cmd_list(). KCSAN reports the pair: BUG: KCSAN: data-race in ublk_batch_dispatch / ublk_batch_queue_rq value changed: 0x0000012a -> 0x0000012b The unlocked check is deliberate and safe. It is one half of a store-buffer pair: the dispatcher clears ->active_fcmd, does smp_mb(), then checks the fifo; the producer adds to the fifo, does smp_mb() in __ublk_acquire_fcmd(), then checks ->active_fcmd. At least one side observes the other's store, so the producer skipping the wakeup and the dispatcher seeing an empty fifo cannot both happen, which is the case that would strand a tag. So the access wants marking, not locking. Both callers of the helper are the unlocked ones, and ublk_io_evts_deinit() uses kfifo_is_empty() directly, so marking it here covers the intended race and nothing else. Found by KCSAN during test_teardown_batch_cancel_tw, which the report fails. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
_ublk_wait_tag_flag_gone() greps the tags file for the flag name, so it prints the one summary line that contains it. The per-tag event ring prints below that line and carries io_flags values rather than flag names, so the transitions that produced the stranded tag are dropped exactly when a failure needs them. teardown_refuse_handover is the case that wants this: it reports a tag left OWNED_BY_SRV after the cancel pass, and the end state alone does not say which path published it. Print the matching tag together with the indented lines that follow it. Assisted-by: Claude Opus 5 Signed-off-by: Bernd Schubert <bernd@niova.io>
ublk_clear_dispatching() read UBLK_IO_FLAG_DISPATCHING as the sign that the tag was dispatching, but that flag outlives UBLK_IO_S_DISPATCHING: ublk_queue_cmd_list() moved the state to UBLK_IO_S_TW_PENDING and left the flag set across the task work hop. Teardown took such a request back through __ublk_abort_rq(), which overwrote UBLK_IO_S_TW_PENDING with UBLK_IO_S_AVAILABLE, and the already queued ublk_cmd_tw_cb() then warned in ublk_io_move(). io->state is verification only, so nothing outside that warning changed. Assisted-by: Claude Opus 5
All four callers capture __ublk_quiesce_dev() as state=$(...), but its failure path passed the state to return, which takes a numeric exit status. bash rejected "LIVE" with "numeric argument required", so the function printed nothing and _ublk_quiesce_and_recover() reported "quiesce failed" instead of naming the state the device was left in. Fixes: 533c87e ("selftests: ublk: add test for UBLK_F_QUIESCE") Assisted-by: Claude Opus 5
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
4 participants
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.
No description provided.