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.