Re: [PATCH 0/6] scsi: make SG_IO user copyback failure-safe

[Bart Van Assche <bvanassche@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]


On 10/18/26 11:32 PM, Wang Yingjie wrote:
>> Has it been considered to disable SG_IO
>> for one-tag devices instead of complicating the SG_IO implementation?
> 
> 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.

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.

[-- Attachment #2: review.md --]
[-- Type: text/markdown, Size: 10424 bytes --]

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 (`20261019.scsi-copyback.9d2bf936038d@outlook.com`).

---

### 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 `cover.1792118770.git.x-professor@outlook.com`). `.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_cmd`s 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.