scsi: make SG_IO user copyback failure-safe
Subsystems (3)
cover scsi: make SG_IO user copyback failure-safe
Several SCSI passthrough completion paths touch userspace while their completed request still owns a queue tag. Sense or READ-bounce copyback can fault on a mapping backed by the same one-tag device, leaving the nested I/O unable to obtain the retained tag. The synchronous and io_uring BSG paths additionally discard data-copyback failures.
Snapshot request-backed completion state, free each request before userspace access, and preserve the existing error precedence while reporting copyback failures. This covers block-device SG_IO, CDDA, and both synchronous and io_uring BSG completion.
Same-device one-tag tests previously left SG_IO, CDDA, and BSG tasks in D state in blk_rq_unmap_user() -> filemap_fault. The fixed paths completed without a stuck task or hung-task report. BSG tests with an unaligned PROT_NONE destination returned false success in 20/20 runs before the error-propagation changes and -EFAULT in 20/20 runs after them, while writable controls continued to succeed.
The series passed scripts/checkpatch.pl --strict, focused W=1 builds, the full bzImage build used for the wider audit, and QEMU differential tests on the affected paths.
Yingjie Wang (6):
scsi: core: release SG_IO request before copying to userspace
scsi: sr: release CDDA request before copying to userspace
scsi: bsg: release synchronous request before user copyback
scsi: bsg: report synchronous data copyback failures
scsi: bsg: release io_uring request before user copyback
scsi: bsg: report io_uring data copyback failures
drivers/scsi/scsi_bsg.c | 60 ++++++++++++++++++++++++++------------- drivers/scsi/scsi_ioctl.c | 45 ++++++++++++++++++----------- drivers/scsi/sr.c | 3 ++ 3 files changed, 72 insertions(+), 36 deletions(-)
base-commit: 0fcb05b72344f40806cbe810de8872f1698bf053
Signature · 1 line
2.43.0
PATCH 1/6 scsi: core: release SG_IO request before copying to userspace
From: Yingjie Wang <[email protected]>
The block-device SG_IO path copies sense and READ bounce data to userspace while the completed request still owns its queue tag. Either copy can fault. If the fault reads from the same one-tag device, the nested I/O cannot obtain a tag and the ioctl remains stuck in uninterruptible sleep.
Snapshot all request-backed status, residual, and sense data, then free the request before either userspace copy. Preserve the existing error precedence between sense copy and data copyback.
A same-device INQUIRY on a one-tag /dev/sr0 queue hung beyond 10 seconds before this change, with sg_io -> blk_rq_unmap_user -> filemap_fault in the stack. The fixed case completed without a stuck task or hung-task warning.
--- drivers/scsi/scsi_ioctl.c | 45 +++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 16 deletions(-)
drivers/scsi/scsi_ioctl.c +29 -16
@@ -388,41 +388,38 @@ static int scsi_fill_sghdr_rq(struct scsi_device *sdev, struct request *rq,return 0 ; } -static int scsi_complete_sghdr_rq (struct request *rq,struct sg_io_hdr *hdr, -struct bio *bio) +static int scsi_complete_sghdr_rq (struct sg_io_hdr *hdr,int result, +unsigned int resid_len, +const u8 *sense_buffer, +unsigned int sense_len) { -struct scsi_cmnd *scmd =blk_mq_rq_to_pdu (rq); -int r, ret =0 ; +int ret =0 ;/* * fill in all the output members */ - hdr->status = scmd->result &0xff ; - hdr->masked_status =sg_status_byte (scmd->result); + hdr->status = result &0xff ; + hdr->masked_status =sg_status_byte (result); hdr->msg_status =COMMAND_COMPLETE ; - hdr->host_status =host_byte (scmd->result); + hdr->host_status =host_byte (result); hdr->driver_status =0 ;if (scsi_status_is_check_condition (hdr->status)) hdr->driver_status =DRIVER_SENSE ; hdr->info =0 ;if (hdr->masked_status || hdr->host_status || hdr->driver_status) hdr->info |=SG_INFO_CHECK ; - hdr->resid = scmd->resid_len; + hdr->resid = resid_len; hdr->sb_len_wr =0 ; -if (scmd->sense_len && hdr->sbp) { -int len =min ((unsigned int ) hdr->mx_sb_len, scmd->sense_len); +if (sense_len && hdr->sbp) { + int len = min_t(unsigned int, hdr->mx_sb_len, sense_len); -if (!copy_to_user (hdr->sbp, scmd->sense_buffer, len)) +if (!copy_to_user (hdr->sbp, sense_buffer, len)) hdr->sb_len_wr = len;else ret = -EFAULT ; } - r =blk_rq_unmap_user (bio); -if (!ret) - ret = r; -return ret; }
@@ -436,6 +433,9 @@ static int sg_io(struct scsi_device *sdev, struct sg_io_hdr *hdr,struct request *rq;struct scsi_cmnd *scmd;struct bio *bio; +u8 sense_buffer[SCSI_SENSE_BUFFERSIZE ]; +unsigned int sense_len, resid_len; +int result, unmap_ret;if (hdr->interface_id !='S' )return -EINVAL ;
@@ -487,7 +487,20 @@ static int sg_io(struct scsi_device *sdev, struct sg_io_hdr *hdr, hdr->duration =jiffies_to_msecs (jiffies - start_time); - ret =scsi_complete_sghdr_rq (rq, hdr, bio); + result = scmd->result; + resid_len = scmd->resid_len; + sense_len = min_t(unsigned int, scmd->sense_len, +sizeof (sense_buffer)); +if (sense_len) +memcpy (sense_buffer, scmd->sense_buffer, sense_len); + +blk_mq_free_request (rq); + ret =scsi_complete_sghdr_rq (hdr, result, resid_len, sense_buffer, + sense_len); + unmap_ret =blk_rq_unmap_user (bio); +if (!ret) + ret = unmap_ret; +return ret; out_put_request:blk_mq_free_request (rq);
Signature · 1 line
2.43.0
PATCH 2/6 scsi: sr: release CDDA request before copying to userspace
In reply to Wang YingjieFrom: Yingjie Wang <[email protected]>
sr_read_cdda_bpc() calls blk_rq_unmap_user() while its completed request still owns the only queue tag. For an unaligned READ buffer, unmap copies bounce data to userspace and can fault on a nonresident mapping of the same CD-ROM. The nested read then waits for the tag held by the outer request.
Free the request after saving all request-backed state and before copying the retained bio back to userspace.
The standalone CDROMREADAUDIO reproducer previously remained in D state in sr_read_cdda_bpc -> blk_rq_unmap_user -> filemap_fault and blocked the disk-events worker in blk_mq_get_tag. With this change it returned immediately, left no stuck process, and produced no hung-task report.
--- drivers/scsi/sr.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/scsi/sr.c +3 -0
@@ -968,8 +968,11 @@ static int sr_read_cdda_bpc(struct cdrom_device_info *cdi, void __user *ubuf, ret = -EIO ; } +blk_mq_free_request (rq);if (blk_rq_unmap_user (bio)) ret = -EFAULT ; +return ret; + out_put_request:blk_mq_free_request (rq);return ret;
Signature · 1 line
2.43.0
Re: [PATCH 2/6] scsi: sr: release CDDA request before copying to userspace
Quoted 6 lines · Wang Yingjie
drivers/scsi/sr.c +0
-0@@ -968,8 +968,11 @@ static int sr_read_cdda_bpc(struct cdrom_device_info *cdi, void __user *ubuf, ret = -EIO ;
drivers/scsi/sr.c static int sr_read_cdda_bpc(struct cdrom_device_info *cdi, void __user *ubuf, · line 978
Quoted 10 lines
drivers/scsi/sr.c +3
-0} +blk_mq_free_request (rq);if (blk_rq_unmap_user (bio)) ret = -EFAULT ; +return ret; + out_put_request:blk_mq_free_request (rq);return ret;
This change triggers a use-after-free because blk_mq_free_request() calls bio_put(), isn't it?
Bart.
Re: [PATCH 2/6] scsi: sr: release CDDA request before copying to userspace
Hi Bart,
Quoted 2 lines
This change triggers a use-after-free because
blk_mq_free_request()callsbio_put(), isn't it?
I checked this on the series base, 0fcb05b72344. In this path, blk_mq_free_request() releases the completed request. It does not put the BIO chain saved from rq->bio; blk_rq_unmap_user() later walks that chain and releases it.
sr_read_cdda_bpc() saves rq->bio before blk_execute_rq(). After completion, the patch reads scmd->result and, when needed, normalizes the sense data before freeing the request. After blk_mq_free_request(), it only calls blk_rq_unmap_user(bio); neither rq nor scmd is accessed again.
The same ownership split is used by sg_rq_end_io(), which frees the request while retaining srp->bio for later unmapping. The deferred completion branch of nvme_uring_cmd_end_io() also returns RQ_END_IO_FREE while leaving pdu->bio to be unmapped in task work.
Based on this ownership, I do not see a use-after-free introduced by patch 2/6. Please let me know if you had a different ownership path in mind.
Thanks,
Yingjie
PATCH 3/6 scsi: bsg: release synchronous request before user copyback
In reply to Wang YingjieFrom: Yingjie Wang <[email protected]>
scsi_bsg_sg_io_fn() retains a completed request while copying both sense data and READ bounce data to userspace. A fault on a mapping of the same one-tag SCSI device starts nested I/O that cannot acquire the retained tag.
Snapshot the request-backed completion fields and sense data, save the transfer direction, and free the request before either userspace copy. The retained bio can then be unmapped safely.
A BSG INQUIRY into a nonresident mapping of the same /dev/sr0 queue previously remained in D state in blk_rq_unmap_user -> filemap_fault. The fixed trigger returned in 0.06 seconds and produced no hung-task report.
--- drivers/scsi/scsi_bsg.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)
drivers/scsi/scsi_bsg.c +22 -9
@@ -199,6 +199,10 @@ static int scsi_bsg_sg_io_fn(struct request_queue *q, struct sg_io_v4 *hdr,struct scsi_cmnd *scmd;struct request *rq;struct bio *bio; +u8 sense_buffer[SCSI_SENSE_BUFFERSIZE ]; +u32 scsi_result, resid_len; +u8 sense_len; +bool is_read;int ret; if (hdr->protocol != BSG_PROTOCOL_SCSI ||
@@ -243,37 +247,46 @@ static int scsi_bsg_sg_io_fn(struct request_queue *q, struct sg_io_v4 *hdr, bio = rq->bio;blk_execute_rq (rq, !(hdr->flags &BSG_FLAG_Q_AT_TAIL )); + scsi_result = scmd->result; + resid_len = scmd->resid_len; + sense_len = min_t(unsigned int, scmd->sense_len, +sizeof (sense_buffer)); +if (sense_len) +memcpy (sense_buffer, scmd->sense_buffer, sense_len); + is_read =rq_data_dir (rq) ==READ ; +blk_mq_free_request (rq);/* * fill in all the output members */ - hdr->device_status = scmd->result &0xff ; - hdr->transport_status =host_byte (scmd->result); + hdr->device_status = scsi_result &0xff ; + hdr->transport_status =host_byte (scsi_result); hdr->driver_status =0 ; -if (scsi_status_is_check_condition (scmd->result)) +if (scsi_status_is_check_condition (scsi_result)) hdr->driver_status =DRIVER_SENSE ; hdr->info =0 ;if (hdr->device_status || hdr->transport_status || hdr->driver_status) hdr->info |=SG_INFO_CHECK ; hdr->response_len =0 ; -if (scmd->sense_len && hdr->response) { +if (sense_len && hdr->response) { int len = min_t(unsigned int, hdr->max_response_len, - scmd->sense_len); + sense_len); -if (copy_to_user (uptr64 (hdr->response), scmd->sense_buffer, +if (copy_to_user (uptr64 (hdr->response), sense_buffer, len)) ret = -EFAULT ;else hdr->response_len = len; } -if (rq_data_dir (rq) ==READ ) - hdr->din_resid = scmd->resid_len; +if (is_read) + hdr->din_resid = resid_len;else - hdr->dout_resid = scmd->resid_len; + hdr->dout_resid = resid_len;blk_rq_unmap_user (bio); +return ret; out_put_request:blk_mq_free_request (rq);
Signature · 1 line
2.43.0
PATCH 4/6 scsi: bsg: report synchronous data copyback failures
In reply to Wang YingjieFrom: Yingjie Wang <[email protected]>
scsi_bsg_sg_io_fn() ignores the result of blk_rq_unmap_user(). A successful command can therefore return success even though copying READ bounce data to userspace failed.
Preserve an earlier sense-buffer copy error and otherwise return the data unmap error.
A virtio-scsi INQUIRY with an unaligned PROT_NONE destination returned false success in 20/20 runs before this change and -EFAULT in 20/20 runs afterward. Writable controls continued to succeed.
--- drivers/scsi/scsi_bsg.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/scsi/scsi_bsg.c +4 -2
@@ -203,7 +203,7 @@ static int scsi_bsg_sg_io_fn(struct request_queue *q, struct sg_io_v4 *hdr,u32 scsi_result, resid_len;u8 sense_len;bool is_read; -int ret; +int ret, unmap_ret;if (hdr->protocol !=BSG_PROTOCOL_SCSI || hdr->subprotocol !=BSG_SUB_PROTOCOL_SCSI_CMD )
@@ -285,7 +285,9 @@ static int scsi_bsg_sg_io_fn(struct request_queue *q, struct sg_io_v4 *hdr,else hdr->dout_resid = resid_len; -blk_rq_unmap_user (bio); + unmap_ret =blk_rq_unmap_user (bio); +if (!ret) + ret = unmap_ret;return ret; out_put_request:
Signature · 1 line
2.43.0
PATCH 5/6 scsi: bsg: release io_uring request before user copyback
In reply to Wang YingjieFrom: Yingjie Wang <[email protected]>
The BSG io_uring end-I/O callback returns RQ_END_IO_NONE, so task work still owns the completed request and its tag. Task work then copies READ bounce data and optional sense data to userspace. A fault on the same one-tag device deadlocks nested I/O behind the retained request.
Snapshot the result, residual, and sense bytes into local storage, then free the request before either userspace copy.
A current-mainline BSG INQUIRY into a nonresident mapping of the same /dev/sr0 queue previously stuck in scsi_bsg_uring_task_cb -> blk_rq_unmap_user -> filemap_fault. The fixed kernel completed 20/20 repetitions and left no blocked I/O or hung-task report.
--- drivers/scsi/scsi_bsg.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
drivers/scsi/scsi_bsg.c +14 -9
@@ -35,32 +35,37 @@ static void scsi_bsg_uring_task_cb(struct io_tw_req tw_req, io_tw_token_t tw)struct scsi_bsg_uring_cmd_pdu *pdu =scsi_bsg_uring_cmd_pdu (ioucmd);struct request *rq = pdu->req;struct scsi_cmnd *scmd =blk_mq_rq_to_pdu (rq); +u8 sense_buffer[SCSI_SENSE_BUFFERSIZE ]; +u32 scsi_result = scmd->result; +u32 resid_len = scmd->resid_len;u64 res2;int ret =0 ;u8 driver_status =0 ;u8 sense_len_wr =0 ; -if (pdu->bio) -blk_rq_unmap_user (pdu->bio); - -if (scsi_status_is_check_condition (scmd->result)) { +if (scsi_status_is_check_condition (scsi_result)) { driver_status =DRIVER_SENSE ;if (pdu->response_addr) sense_len_wr = min_t(unsigned int, pdu->max_response_len, scmd->sense_len); } +if (sense_len_wr) +memcpy (sense_buffer, scmd->sense_buffer, sense_len_wr); + +blk_mq_free_request (rq); +if (pdu->bio) +blk_rq_unmap_user (pdu->bio);if (sense_len_wr) { -if (copy_to_user (uptr64 (pdu->response_addr), scmd->sense_buffer, +if (copy_to_user (uptr64 (pdu->response_addr), sense_buffer, sense_len_wr)) ret = -EFAULT ; } - res2 =bsg_scsi_res2_build (status_byte (scmd->result), driver_status, -host_byte (scmd->result), sense_len_wr, - scmd->resid_len); + res2 =bsg_scsi_res2_build (status_byte (scsi_result), + driver_status,host_byte (scsi_result), + sense_len_wr, resid_len); -blk_mq_free_request (rq);io_uring_cmd_done32 (ioucmd, ret, res2,IO_URING_CMD_TASK_WORK_ISSUE_FLAGS ); }
Signature · 1 line
2.43.0
PATCH 6/6 scsi: bsg: report io_uring data copyback failures
In reply to Wang YingjieFrom: Yingjie Wang <[email protected]>
scsi_bsg_uring_task_cb() discards the return from blk_rq_unmap_user(). A successful data-in command can therefore post CQE 0 even when bounce data was not delivered to userspace.
Use the unmap result as the CQE status. A later sense-buffer copy failure remains -EFAULT as before.
A raw BSG INQUIRY into an unaligned anonymous PROT_NONE mapping posted false success in 20/20 runs before this change and -EFAULT in 20/20 runs afterward. Writable controls continued to succeed.
--- drivers/scsi/scsi_bsg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/scsi_bsg.c +1 -1
@@ -54,7 +54,7 @@ static void scsi_bsg_uring_task_cb(struct io_tw_req tw_req, io_tw_token_t tw)blk_mq_free_request (rq);if (pdu->bio) -blk_rq_unmap_user (pdu->bio); + ret =blk_rq_unmap_user (pdu->bio);if (sense_len_wr) { if (copy_to_user(uptr64(pdu->response_addr), sense_buffer,
Signature · 1 line
2.43.0
Quoted 5 lines · Wang Yingjie
Several SCSI passthrough completion paths touch userspace while their completed request still owns a queue tag. Sense or READ-bounce copyback can fault on a mapping backed by the same one-tag device, leaving the nested I/O unable to obtain the retained tag. The synchronous and
io_uringBSG paths additionally discard data-copyback failures.
How common are one-tag devices? Has it been considered to disable SG_IO for one-tag devices instead of complicating the SG_IO implementation?
Thanks,
Bart.
Hi Bart,
Quoted 1 line
How common are one-tag devices?
I don't have deployment figures, but usb-storage sets .can_queue = 1 in usb_stor_host_template. The QEMU reproducer used an IDE CD-ROM through ata_piix, whose host template also has .can_queue = 1.
Quoted 2 lines
Has it been considered to disable
SG_IO
for one-tag devices instead of complicating theSG_IOimplementation?
My concern with disabling SG_IO is that it would also reject working passthrough commands on those devices. For example, smartctl uses SG_IO on block devices for SCSI/SAT commands. The lifetime changes preserve that interface by saving the request-backed completion data, freeing the completed request, and then doing the potentially faulting user copies. That does require extra storage for the completion results and sense data.
Separately, patches 4/6 and 6/6 fix lost data-copyback errors and do not depend on the queue depth. I can split those out if that helps review.
Thanks,
Yingjie
Quoted 10 lines · Wang Yingjie
Quoted 2 lines
Has it been considered to disable
SG_IO
for one-tag devices instead of complicating theSG_IOimplementation?My concern with disabling
SG_IOis that it would also reject working passthrough commands on those devices. For example, smartctl usesSG_IOon block devices for SCSI/SAT commands. The lifetime changes preserve that interface by saving the request-backed completion data, freeing the completed request, and then doing the potentially faulting user copies. That does require extra storage for the completion results and sense data.
Quoted 2 lines
Separately, patches 4/6 and 6/6 fix lost data-copyback errors and do not depend on the queue depth. I can split those out if that helps review.
Since I don't have the time right now to do an in-depth review, please take a look at the attached LLM-generated review. One way to view the attached markdown file is by using the Linux "glow" command. The review comes from an LLM that is not yet publicly available.
Thanks,
Bart.
Here is a technical review of Wang Yingjie's series ([PATCH 0/6] scsi: make SG_IO user copyback failure-safe) and his follow-up reply ([email protected]).
---
### 1. Architectural Assessment & Evaluation of Yingjie's Reply
1. **Disabling SG_IO on .can_queue == 1 devices is not viable:**
- Yingjie's examples are accurate: [usb_stor_host_template](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/usb/storage/scsiglue.c#L632) sets .can_queue = 1 (covering all non-UAS USB flash drives, USB external HDD/SSD bridges, and USB optical drives), and ATA_SUBBASE_SHT in [include/linux/libata.h](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/linux/libata.h#L1488-L1492) sets .can_queue = ATA_DEF_QUEUE (1) for PATA/BMDMA hosts such as ata_piix (plus sbp2, ppa, imm, etc.).
- Disabling SG_IO (or CDROMREADAUDIO) on those queues would break smartctl -d sat, hdparm, udisks2, fwupd, sg3_utils, and optical media burning/ripping tools (growisofs, cdrecord/wodim, cdparanoia, VLC).
2. **The deadlock is not specific to .can_queue == 1 queues:**
- Holding a blk-mq driver tag across a faulting user copy creates a general lock/resource inversion:
$$\text{blk-mq tag} \;\longrightarrow\; \text{user page fault / folio lock} \;\longrightarrow\; \text{nested same-queue I/O} \;\longrightarrow\; \text{blk-mq tag}$$
- On a queue with can_queue = K, $K$ concurrent passthrough completions faulting onto nonresident mappings on the same device exhaust all $K$ tags and deadlock identically (as Yingjie himself demonstrated with queue depth 2 in his parallel NVMe series [email protected]). .can_queue = 1 simply reduces the concurrency needed from $K$ threads to 1.
- Releasing rq before blk_rq_unmap_user() is sound from a lifetime perspective: blk_rq_map_user*() allocates BIOs independently from fs_bio_set via [blk_rq_map_bio_alloc()](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/blk-map.c#L45-L57), and both [sg_rq_end_io()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/sg.c#L1374-L1382) (since commit 7568615c1054) and [nvme_uring_cmd_end_io()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/nvme/host/ioctl.c#L447-L479) (RQ_END_IO_FREE) already rely on freeing rq before blk_rq_unmap_user().
3. **Build / Checkpatch / Stack Frame Sanity:**
- All 6 patches apply cleanly on 0fcb05b72344, pass scripts/checkpatch.pl --strict, compile cleanly with W=1 C=1 (zero compiler or sparse warnings), and keep stack frames small (120 bytes on x86_64 for sg_io(), scsi_bsg_sg_io_fn(), and scsi_bsg_uring_task_cb()).
---
### 2. Substantive Issues Found in the Patches
#### Issue 1 (Major — Patch 5/6): scsi_bsg_uring_cmd_done() still pins rq across io_uring task_work (RQ_END_IO_NONE)
In [drivers/scsi/scsi_bsg.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_bsg.c#L68-L76), [scsi_bsg_uring_cmd_done()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_bsg.c#L68-L76) returns RQ_END_IO_NONE and defers blk_mq_free_request(rq) to [scsi_bsg_uring_task_cb()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_bsg.c#L32-L66). Patch 5/6 only moves blk_mq_free_request(rq) a few lines earlier inside scsi_bsg_uring_task_cb(), which leaves the request and its blk-mq tag pinned across the entire async boundary between end_io and task_work:
- **Still deadlocks with IORING_SETUP_DEFER_TASKRUN:** Under IORING_SETUP_DEFER_TASKRUN (standard modern liburing usage), [io_req_local_work_add()](file:///usr/local/google/home/bvanassche/software/linux-kernel/io_uring/tw.c#L151-L206) pushes the item onto ctx->work_list without scheduling signal task_work; scsi_bsg_uring_task_cb() **does not run until userspace explicitly calls io_uring_enter(IORING_ENTER_GETEVENTS)**. If the ring task submits 1 BSG command on a 1-tag queue (or can_queue commands on *any* queue) and then touches a nonresident mmap() page or calls pread() on that same device before calling io_uring_enter(GETEVENTS), it **still deadlocks in D state in blk_mq_get_tag()** even with Patch 5/6 applied.
- **Un-reaped CQEs starve the entire queue:** Even without DEFER_TASKRUN, if a task submits can_queue BSG uring_cmds and sleeps (poll()/futex()) without reaping CQEs after I/O finishes, every tag on the device remains pinned, hanging disk_events, writeback, and all other processes in blk_mq_get_tag() — the exact bug fixed for /dev/sg* in commit 7568615c1054 (*"sg: fix unkillable I/O wait deadlock with scsi-mq"*).
- **Better fix (matching [nvme_uring_cmd_end_io()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/nvme/host/ioctl.c#L447-L479) and [sg_rq_end_io()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/sg.c#L1374-L1382)):** Snapshot completion state into struct scsi_bsg_uring_cmd_pdu inside scsi_bsg_uring_cmd_done() *before* calling io_uring_cmd_do_in_task_lazy(), and **return RQ_END_IO_FREE;**:
- pdu->req is no longer needed, and max_response_len is read in end_io while res2 is read in task_cb, so bio (8B) + response_addr (8B) + union { u32 max_response_len; u64 res2; } (8B) + u8 *sense_buf (8B) = **32 bytes**, fitting within ioucmd->pdu[32].
- Allocating sense_buf (min_t(u32, cmd->max_response_len, SCSI_SENSE_BUFFERSIZE)) in [scsi_bsg_uring_cmd()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_bsg.c#L163-L165) only when cmd->response && cmd->max_response_len avoids both GFP_ATOMIC failure in IRQ context and the 96-byte stack array in scsi_bsg_uring_task_cb().
#### Issue 2 (Bug — Patch 5/6): scsi_bsg_uring_task_cb() reports non-zero sense_len_wr in CQE res2 on copy_to_user() failure and omits sizeof(sense_buffer) bound In Patch 5/6 (scsi_bsg_uring_task_cb()):
```c
if (sense_len_wr) {
if (copy_to_user(uptr64(pdu->response_addr), sense_buffer,
sense_len_wr))
ret = -EFAULT;
}
res2 = bsg_scsi_res2_build(status_byte(scsi_result),
driver_status, host_byte(scsi_result),
sense_len_wr, resid_len);
```1. Per [include/uapi/linux/bsg.h](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/uapi/linux/bsg.h#L108), res2 bits [24..31] are sense_len_wr (*"bytes of sense data written to response buffer"*). Both [scsi_complete_sghdr_rq()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_ioctl.c#L411-L420) (hdr->sb_len_wr = 0) and [scsi_bsg_sg_io_fn()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_bsg.c#L258-L269) (hdr->response_len = 0) report 0 bytes written when copy_to_user() fails, whereas scsi_bsg_uring_task_cb() leaves sense_len_wr non-zero. It should zero sense_len_wr when copy_to_user() fails.
2. Patches 1/6 and 3/6 explicitly bound sense_len with sizeof(sense_buffer), whereas Patch 5/6 computes sense_len_wr = min_t(unsigned int, pdu->max_response_len, scmd->sense_len) without clamping to sizeof(sense_buffer) before memcpy(sense_buffer, scmd->sense_buffer, sense_len_wr).
#### Issue 3 (Missed Call Sites): Same deadlock exists in sg_scsi_ioctl() (same file!) and bsg_transport_sg_io_fn()
1. Immediately below [sg_io()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_ioctl.c#L429-L495) in [drivers/scsi/scsi_ioctl.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_ioctl.c#L612-L633), [sg_scsi_ioctl()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_ioctl.c#L523-L634) (SCSI_IOCTL_SEND_COMMAND) calls copy_to_user(sic->data, scmd->sense_buffer, ...) and copy_to_user(sic->data, buffer, out_len) **before** blk_mq_free_request(rq). If sic->data faults onto a mapping backed by the same 1-tag queue (e.g. sic crossing a resident/nonresident page boundary or reclaimed during I/O), it deadlocks identically.
2. [bsg_transport_sg_io_fn()](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bsg-lib.c#L115-L149) in [block/bsg-lib.c](file:///usr/local/google/home/bvanassche/software/linux-kernel/block/bsg-lib.c#L138-L149) likewise runs copy_to_user() and blk_rq_unmap_user() before blk_mq_free_request() and ignores blk_rq_unmap_user() errors.
#### Issue 4 (Metadata / Stable Backporting): Bogus Fixes: tags in Patches 1/6–4/6
- **Patches 1/6 & 2/6** cite Fixes: 31151ba2cef1 ("fix mismerge in ll_rw_blk.c"), an August 2005 merge commit touching drivers/block/ll_rw_blk.c — 9 years before fixed scsi-mq tag pools existed and 16 years before [sr_read_cdda_bpc()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/sr.c#L928-L976) (ba51bdafaafc, v5.15) and [sg_io()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_ioctl.c#L429-L495) (f2542a3be327, v5.15) existed in drivers/scsi/.
- **Patches 3/6 & 4/6** cite Fixes: 10e8855b9451 ("bsg: add SG_IO to SG v4") (block/bsg.c, 2006), whereas [scsi_bsg_sg_io_fn()](file:///usr/local/google/home/bvanassche/software/linux-kernel/drivers/scsi/scsi_bsg.c#L196-L281) in drivers/scsi/scsi_bsg.c was created in 78011042684d / 75ca56409e5b (v5.15).
#### Issue 5 (Series Organization & Minor Type Consistency)
- **Splitting / ordering:** Accepting Yingjie's offer to split (or place first in the series) Patches 4/6 and 6/6 makes sense: lost data-copyback -EFAULT reporting is unconditional and independent of queue depth, so having those self-contained fixes first simplifies review and backporting.
- **Type consistency:** [scmd->result](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/scsi/scsi_cmnd.h#L143) is int and [scmd->sense_len](file:///usr/local/google/home/bvanassche/software/linux-kernel/include/scsi/scsi_cmnd.h#L118) is unsigned, yet Patch 1/6 uses int result / unsigned int sense_len while Patches 3/6 and 5/6 use u32 scsi_result / u8 sense_len. Using int for result consistently across all three functions is cleaner.
Hi Bart,
Thanks for the detailed review. I checked the relevant paths against the series base, 0fcb05b72344.
You're right about the request/tag-retention window in patch 5/6. It still returns RQ_END_IO_NONE, so moving blk_mq_free_request() to the beginning of task work does not address the window before task work runs.
For the reroll, I plan to preallocate the required sense storage at submission and save the request-backed completion state in scsi_bsg_uring_cmd_done(). This would let end-I/O return RQ_END_IO_FREE, while task work handles the unmap, user copies and CQE completion without accessing rq or scmd. I'll check the allocation and cleanup paths, including nonblocking submission, and retest with IORING_SETUP_DEFER_TASKRUN.
The tag/fault dependency itself is not limited to a one-tag queue; it can also arise if completed requests retain all tags available to the nested fault I/O. So far, however, I have only reproduced the one-tag case at runtime. I'll make that distinction explicit in the cover letter.
For the sense data, I'll bound the snapshot by its storage size even though valid-sense processing in the midlayer caps the computed length. The existing sense_len_wr behavior on copy failure predates this series, so that should be a separate fix.
I also found the same copy-before-free ordering in sg_scsi_ioctl() and will validate that path as part of the lifetime reroll. bsg_transport_sg_io_fn() uses a separate transport BSG queue, so I do not want to claim the same demonstrated deadlock there without a separate audit.
Patches 4/6 and 6/6 are independent copyback-error fixes. I'll split them out so they are not held up by the larger lifetime rework. I also need to recheck the lifecycle-related Fixes tags across the earlier implementations and file moves.
Thanks,
Yingjie