[PATCH v3 0/3] scsi: libiscsi: tighten task completion

[PATCH v3 0/3] scsi: libiscsi: tighten task completion

[Yehyeong Lee]

1/3 stops the receive path from completing a management task that is still
queued or still being filled in, so a slot cannot be returned to the task
pool while the transmit path still owns it.  2/3 stops it from completing a
NOP-Out sent with RESERVED_ITT, whose single reference belongs to the
transport, so that reference cannot be dropped twice.  3/3 is new: it makes
the transmit path hold a reference before it publishes the state the other
two test, and makes the SCSI command lookup honour that state at all.

v2 -> v3:
 - 1/3 and 2/3 are unchanged.
 - 3/3 is new.

The review of v2 1/2 raised three issues.  All three are older than this
series, which narrows two of them and leaves the third as it was, and 3/3
addresses each one:

 - the window between the ISCSI_TASK_RUNNING store in
   iscsi_prep_mgmt_task() / iscsi_prep_scsi_cmd_pdu() and iscsi_get_task()
   in iscsi_xmit_task(): the store moves into iscsi_xmit_task(), after the
   reference has been taken.  For a SCSI command that alone is not enough,
   because iscsi_itt_to_ctask() never read task->state, so a command still
   queued on conn->cmdqueue in ISCSI_TASK_PENDING could be completed by the
   receive path; that lookup now skips a task that has not been sent, the
   way 1/3 does for the management lookup.

 - iscsi_xmit_task() requeueing in conn->task a task that the receive path
   completed while frwd_lock was dropped: the requeue becomes conditional
   on the task still being ISCSI_TASK_RUNNING, read under back_lock, which
   is the test commit 533ac412fdb4 ("scsi: iscsi: Remove unneeded task
   state check") removed.  The other half of the same window - a task that
   was parked in conn->task and finished before it could be sent again - is
   dropped now instead of sent.

 - transports without ihost->workq, which call session->tt->xmit_task()
   from iscsi_send_mgmt_task() and iscsi_queuecommand() with no reference
   beyond the allocation one: both now take a transmit reference of their
   own, and both finish with a failed send inside the same back_lock
   section that tests for it, so the receive path can no longer complete
   a task whose reference the caller is about to put.
   iscsi_queuecommand() gives the command back to scsi-ml right there;
   iscsi_send_mgmt_task(), whose caller frees the task, parks it in
   ISCSI_TASK_COMPLETED, which is a state no ->cleanup_task() reads
   differently from ISCSI_TASK_RUNNING.

3/3 needs 1/3 underneath, and is marked for stable for the same reason.

v1 of 1/3:
  https://lore.kernel.org/all/20261113145140.282504-1-yhlee@isslab.korea.ac.kr/
v1 of 2/3:
  https://lore.kernel.org/all/20261113232322.395246-1-yhlee@isslab.korea.ac.kr/
v2:
  https://lore.kernel.org/all/20261113233938.397415-1-yhlee@isslab.korea.ac.kr/

Best regards,
Yehyeong Lee

Yehyeong Lee (3):
  scsi: libiscsi: only complete a task that has been sent
  scsi: libiscsi: do not complete a task sent with a reserved ITT
  scsi: libiscsi: take a transmit reference before a task is published

 drivers/scsi/libiscsi.c | 67 +++++++++++++++++++++++++++++++++++------
 1 file changed, 58 insertions(+), 9 deletions(-)


base-commit: 7e1392a42ea288d70a266f6553fe06f57f8647ea
-- 
2.43.0

[PATCH v3 1/3] scsi: libiscsi: only complete a task that has been sent

[Yehyeong Lee]

The RX path looks a management response or a Reject up by ITT and
completes the task if it is neither free nor a SCSI command.  That also
accepts two kinds of slot that were never sent:

- a management task still waiting on conn->mgmtqueue in
  ISCSI_TASK_PENDING.  Completing it returns the slot to the pool while
  it is still linked on the queue, so the xmit worker and the next
  allocation both use it.
- a slot that iscsi_alloc_task() is still filling in.  It stores
  ISCSI_TASK_PENDING before task->conn and task->sc under frwd_lock,
  while the RX check runs under back_lock, so a fresh slot is seen with
  a NULL task->conn.

The target chooses the ITT and the pool index is small, so either needs
nothing but timing.

Reproduced with test-only delays that hold each window open and a
target that sends a Reject naming the slot.  For a queued task the
unpatched driver freed it in 8 of 8 attempts inside the window, this
patch in 0 of 5.  KASAN reported nothing; the xmit worker hit:

[    7.309467] ------------[ cut here ]------------
[    7.309469] WARNING: drivers/scsi/libiscsi.c:1552 at iscsi_xmit_task+0x36c/0x470, CPU#1: kworker/u8:2/42
[    7.311388] Modules linked in:
[    7.311730] CPU: 1 UID: 0 PID: 42 Comm: kworker/u8:2 Not tainted 7.4.0-rc2-00471-g9ab317b63171-dirty #3 PREEMPT(lazy)
[    7.312825] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[    7.314002] Workqueue: iscsi_q_2 iscsi_xmitworker
[    7.314512] RIP: 0010:iscsi_xmit_task+0x36c/0x470
[    7.315011] Code: 74 24 0c 48 8b 4c 24 10 f0 0f b1 31 0f 85 8d 00 00 00 8b 44 24 38 85 c0 78 37 89 c6 83 c6 01 78 30 85 c0 0f 85 2f fd ff ff 90 <0f> 0b 90 45 31 e4 e9 3d fe ff ff 31 f6 4c 89 f7 e8 af 2e 16 ff e9
[    7.316897] RSP: 0018:ffffc900002c7b38 EFLAGS: 00010246
[    7.317450] RAX: 0000000000000000 RBX: ffff88800c30a378 RCX: ffff88800c0b1878
[    7.318192] RDX: ffffed1001816310 RSI: 0000000000000000 RDI: ffff88800c0b1878
[    7.318928] RBP: ffff88800c0b1800 R08: 0000000000000000 R09: ffffed100181630f
[    7.319668] R10: ffff88800c0b187b R11: 0000000000000000 R12: ffff88800c305440
[    7.320410] R13: 1ffff92000058f6a R14: 0000000000000000 R15: ffff88800c30a400
[    7.321146] FS:  0000000000000000(0000) GS:ffff8880e56a8000(0000) knlGS:0000000000000000
[    7.321978] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    7.322589] CR2: 00007f1933873000 CR3: 000000000b73b004 CR4: 0000000000770ef0
[    7.323340] PKRU: 55555554
[    7.323633] Call Trace:
[    7.323902]  <TASK>
[    7.324162]  ? iscsi_sw_tcp_pdu_init+0x81e/0xb00
[    7.324635]  ? __pfx_iscsi_xmit_task+0x10/0x10
[    7.325088]  ? iscsi_prep_mgmt_task+0x381/0x590
[    7.325570]  iscsi_xmitworker+0x258/0xab0
[    7.325997]  process_one_work+0x697/0x1060
[    7.326441]  ? __pfx___schedule+0x10/0x10
[    7.326872]  ? __pfx_process_one_work+0x10/0x10
[    7.327365]  ? _raw_spin_lock_irq+0x81/0xe0
[    7.327816]  ? __pfx_iscsi_xmitworker+0x10/0x10
[    7.328308]  ? assign_work+0x11d/0x370
[    7.328689]  worker_thread+0x45b/0xd10
[    7.329075]  ? __pfx_worker_thread+0x10/0x10
[    7.329514]  kthread+0x2c8/0x3b0
[    7.329848]  ? recalc_sigpending+0x15c/0x1e0
[    7.330298]  ? __pfx_kthread+0x10/0x10
[    7.330683]  ret_from_fork+0x36e/0x5a0
[    7.331069]  ? __pfx_ret_from_fork+0x10/0x10
[    7.331506]  ? __switch_to+0x572/0xdc0
[    7.331889]  ? __pfx_kthread+0x10/0x10
[    7.332278]  ret_from_fork_asm+0x1a/0x30
[    7.332676]  </TASK>
[    7.332909] ---[ end trace 0000000000000000 ]---
[    7.333687] refcount_t: underflow; use-after-free.

For the allocation window, under KASAN, 7 of 7 unpatched and 0 of 5
patched:

[    7.293597] Oops: general protection fault, probably for non-canonical address 0xdffffc000000000c: 0000 [#1] SMP KASAN NOPTI
[    7.295946] KASAN: null-ptr-deref in range [0x0000000000000060-0x0000000000000067]
[    7.296860] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 7.4.0-rc2-00471-g33481dfad534-dirty #1 PREEMPT(lazy)
[    7.298097] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[    7.299457] RIP: 0010:iscsi_nop_out_rsp.constprop.0+0x4e/0x170
[    7.300200] Code: c1 ea 03 48 83 ec 08 80 3c 02 00 0f 85 fd 00 00 00 48 b8 00 00 00 00 00 fc ff df 48 8b 6b 58 48 8d 7d 60 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 c8 00 00 00 48 8b 45 60 48 39 c3 74 55 48 b8 00
[    7.302531] RSP: 0018:ffffc90000006b58 EFLAGS: 00010206
[    7.303213] RAX: dffffc0000000000 RBX: ffff888009e2a000 RCX: 0000000000000000
[    7.304126] RDX: 000000000000000c RSI: ffffc90000006c10 RDI: 0000000000000060
[    7.305037] RBP: 0000000000000000 R08: 0000000000000080 R09: 0000000000000000
[    7.305948] R10: ffffc9000000679f R11: 000000000000004d R12: ffff88800c70c440
[    7.306858] R13: 0000000000000000 R14: ffffc90000006c10 R15: 0000000000000000
[    7.307766] FS:  0000000000000000(0000) GS:ffff8880e55a8000(0000) knlGS:0000000000000000
[    7.308794] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    7.309541] CR2: 0000559011da8ef8 CR3: 000000000be2c005 CR4: 0000000000770ef0
[    7.310472] PKRU: 55555554
[    7.310838] Call Trace:
[    7.311173]  <IRQ>
[    7.311455]  __iscsi_complete_pdu+0x1acf/0x2390
[    7.312054]  ? iscsi_dbg_trace+0xcc/0x100
[    7.312583]  ? iscsi_dbg_trace+0xcc/0x100
[    7.313112]  ? __pfx___iscsi_complete_pdu+0x10/0x10
[    7.313748]  ? __pfx__raw_spin_lock+0x10/0x10
[    7.314327]  iscsi_complete_pdu+0x5c/0x80
[    7.314855]  iscsi_tcp_data_recv_done+0xf4/0x250
[    7.315459]  iscsi_tcp_recv_skb+0x31e/0xec0
[    7.316007]  ? __tcp_transmit_skb+0x2ec9/0x4720
[    7.316602]  ? __pfx_iscsi_tcp_recv_skb+0x10/0x10
[    7.317217]  ? __pfx_iscsi_dbg_trace+0x10/0x10
[    7.317800]  ? __pfx_selinux_parse_skb.constprop.0+0x10/0x10
[    7.318539]  ? __alloc_skb+0x342/0x890
[    7.319041]  iscsi_sw_tcp_recv+0x12f/0x390
[    7.319576]  ? __pfx_iscsi_sw_tcp_recv+0x10/0x10
[    7.320182]  ? __pfx_selinux_netlbl_sock_rcv_skb+0x10/0x10
[    7.320891]  __tcp_read_sock+0x1ab/0x810
[    7.321407]  ? __pfx_iscsi_sw_tcp_recv+0x10/0x10
[    7.322012]  ? __pfx___tcp_read_sock+0x10/0x10
[    7.322593]  iscsi_sw_tcp_data_ready+0x18b/0x510
[    7.323199]  ? __pfx_iscsi_sw_tcp_data_ready+0x10/0x10
[    7.323864]  ? tcp_try_rmem_schedule+0x17f/0x1390
[    7.324476]  ? tcp_event_data_recv+0x36a/0x7b0
[    7.325061]  tcp_data_queue+0x1f2d/0x4b70
[    7.325587]  ? __pfx_tcp_data_queue+0x10/0x10
[    7.326162]  ? tcp_try_undo_loss+0x670/0x730
[    7.326721]  ? kvm_clock_get_cycles+0x19/0x40
[    7.327300]  tcp_rcv_established+0x8a8/0x3990
[    7.327876]  ? security_sock_rcv_skb+0x2b/0x40
[    7.328460]  ? sk_filter_trim_cap+0x400/0x980
[    7.329036]  ? __pfx_tcp_inbound_hash+0x10/0x10
[    7.329629]  ? __pfx_tcp_rcv_established+0x10/0x10
[    7.330255]  ? __pfx_sk_filter_trim_cap+0x10/0x10
[    7.330869]  ? _raw_spin_lock+0x80/0xe0
[    7.331373]  tcp_v4_do_rcv+0x45b/0x970
[    7.331872]  ? tcp_v4_fill_cb+0x26/0x400
[    7.332385]  tcp_v4_rcv+0x223d/0x3b90
[    7.332874]  ? __kasan_unpoison_pages+0x2c/0x40
[    7.333466]  ? __pfx_tcp_v4_rcv+0x10/0x10
[    7.333996]  ? __pfx_raw_local_deliver+0x10/0x10
[    7.334598]  ? get_stack_info_noinstr+0x16/0xe0
[    7.335197]  ? get_stack_info+0x2a/0x90
[    7.335703]  ip_protocol_deliver_rcu+0x61/0x2e0
[    7.336302]  ip_local_deliver_finish+0x329/0x4b0
[    7.336910]  ip_local_deliver+0x18f/0x2d0
[    7.337436]  ? __pfx_ip_local_deliver+0x10/0x10
[    7.338033]  ? ipv4_dst_check+0x10a/0x160
[    7.338557]  ? ip_rcv_finish_core+0x1d0c/0x1dd0
[    7.339160]  ip_sublist_rcv_finish+0x138/0x1b0
[    7.339743]  ip_sublist_rcv+0x3ae/0x670
[    7.340252]  ? kasan_save_stack+0x42/0x60
[    7.340775]  ? kasan_save_track+0x14/0x30
[    7.341301]  ? __pfx_ip_sublist_rcv+0x10/0x10
[    7.341876]  ? __alloc_skb+0x11e/0x890
[    7.342371]  ? net_rx_action+0x8f1/0xfb0
[    7.342891]  ? handle_softirqs+0x1b3/0x600
[    7.343427]  ? __irq_exit_rcu+0xfc/0x160
[    7.343943]  ? common_interrupt+0x7f/0x90
[    7.344467]  ? __asan_memset+0x23/0x50
[    7.344968]  ? ip_rcv_core+0x4e9/0xce0
[    7.345462]  ? common_startup_64+0x13e/0x158
[    7.346028]  ip_list_rcv+0x266/0x380
[    7.346503]  ? csum_partial+0xc/0x20
[    7.346981]  ? __pfx_ip_list_rcv+0x10/0x10
[    7.347517]  __netif_receive_skb_list_core+0x5c3/0x860
[    7.348186]  ? tcp4_gro_receive+0x54a/0x15c0
[    7.348746]  ? __pfx___netif_receive_skb_list_core+0x10/0x10
[    7.349480]  ? inet_gro_receive+0x719/0x1190
[    7.350049]  ? kmalloc_reserve+0x103/0x2b0
[    7.350584]  netif_receive_skb_list_internal+0x553/0xbb0
[    7.351271]  ? dma_map_phys+0xb0/0x290
[    7.351768]  ? __pfx_netif_receive_skb_list_internal+0x10/0x10
[    7.351998] TESTWIN close itt 1
[    7.352946]  ? e1000_alloc_rx_buffers+0x32c/0xd70
[    7.353561]  ? gro_receive_skb+0x279/0x960
[    7.354103]  napi_complete_done+0x1ab/0x7f0
[    7.354648]  ? __pfx_napi_complete_done+0x10/0x10
[    7.355264]  ? __pfx_e1000_clean_rx_irq+0x10/0x10
[    7.355880]  e1000_clean+0x816/0x25a0
[    7.356364]  ? schedule_timeout+0x1cd/0x200
[    7.356916]  ? tmigr_requires_handle_remote_up+0x49/0x160
[    7.357614]  ? kvm_sched_clock_read+0x11/0x20
[    7.358189]  ? sched_clock+0x11/0x30
[    7.358665]  ? sched_clock_cpu+0x6c/0x4c0
[    7.359197]  ? __walk_groups_from+0x40/0x150
[    7.359757]  ? __pfx_e1000_clean+0x10/0x10
[    7.360294]  ? __pfx_sched_clock_cpu+0x10/0x10
[    7.360881]  ? sched_balance_trigger+0xc7/0xf20
[    7.361474]  __napi_poll+0x9c/0x520
[    7.361942]  net_rx_action+0x8f1/0xfb0
[    7.362437]  ? kvm_clock_get_cycles+0x19/0x40
[    7.363012]  ? ktime_get+0xe6/0x160
[    7.363478]  ? __pfx_net_rx_action+0x10/0x10
[    7.364043]  ? timerqueue_linked_add+0x1f4/0x3d0
[    7.364646]  ? _raw_spin_lock+0x80/0xe0
[    7.365156]  handle_softirqs+0x1b3/0x600
[    7.365670]  ? __pfx_handle_softirqs+0x10/0x10
[    7.366255]  __irq_exit_rcu+0xfc/0x160
[    7.366748]  common_interrupt+0x7f/0x90
[    7.367256]  </IRQ>
[    7.367547]  <TASK>
[    7.367840]  asm_common_interrupt+0x26/0x40
[    7.368388] RIP: 0010:pv_native_safe_halt+0xf/0x20
[    7.369013] Code: 04 ce 00 c3 cc cc cc cc 0f 1f 00 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 f3 0f 1e fa eb 07 0f 00 2d 93 44 18 00 fb f4 <e9> 8c 39 02 00 66 2e 0f 1f 84 00 00 00 00 00 66 90 90 90 90 90 90
[    7.371276] RSP: 0018:ffffffff85607d10 EFLAGS: 00000216
[    7.371952] RAX: ffff8880e55a8000 RBX: ffffffff85614700 RCX: ffffffff846d0c6b
[    7.372863] RDX: ffffed100d8862a3 RSI: 0000000000000004 RDI: 000000000000480c
[    7.373768] RBP: fffffbfff0ac28e0 R08: 0000000000000001 R09: ffffed100d8862a2
[    7.374675] R10: ffff88806c431513 R11: 0000000000000002 R12: 0000000000000000
[    7.375584] R13: 0000000000000000 R14: 1ffffffff0ac0fa6 R15: dffffc0000000000
[    7.376474]  ? ct_kernel_exit.constprop.0+0x9b/0xc0
[    7.377078]  default_idle+0x9/0x10
[    7.377533]  default_idle_call+0x39/0x60
[    7.378051]  do_idle+0x37b/0x560
[    7.378485]  ? __pfx_do_idle+0x10/0x10
[    7.378983]  cpu_startup_entry+0x4f/0x60
[    7.379497]  rest_init+0x186/0x1b0
[    7.379956]  ? __pfx_acpi_read_bit_register+0x10/0x10
[    7.380584]  ? __pfx_rest_init+0x10/0x10
[    7.381074]  ? __pfx_kernel_init+0x10/0x10
[    7.381579]  ? acpi_enable_subsystem+0x24b/0x470
[    7.382154]  start_kernel+0x354/0x3b0
[    7.382635]  x86_64_start_reservations+0x18/0x30
[    7.383242]  x86_64_start_kernel+0x111/0x120
[    7.383771]  common_startup_64+0x13e/0x158
[    7.384290]  </TASK>
[    7.384590] Modules linked in:
[    7.385018] ---[ end trace 0000000000000000 ]---

Mark a task ISCSI_TASK_RUNNING with a release once it is ready to go
out, and accept a response only for a task in that state, read with an
acquire.  A response can only follow a transmission: attempts that
arrived after the window closed were still accepted, 24 of 24 unpatched
and 9 of 9 patched.

Fixes: 659743b02c41 ("[SCSI] libiscsi: Reduce locking contention in fast path")
Fixes: 5923d64b7ab6 ("scsi: libiscsi: Drop taskqueuelock")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/libiscsi.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 2d04448432a772..83d726043d3e8d 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -429,7 +429,8 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 	if (session->tt->init_task && session->tt->init_task(task))
 		return -EIO;
 
-	task->state = ISCSI_TASK_RUNNING;
+	/* Pairs with the acquire in the RX task lookups. */
+	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
 	session->cmdsn++;
 
 	conn->scsicmd_pdus_cnt++;
@@ -702,7 +703,8 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
 	if ((hdr->opcode & ISCSI_OPCODE_MASK) == ISCSI_OP_LOGOUT)
 		session->state = ISCSI_STATE_LOGGING_OUT;
 
-	task->state = ISCSI_TASK_RUNNING;
+	/* Pairs with the acquire in the RX task lookups. */
+	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
 	ISCSI_DBG_SESSION(session, "mgmtpdu [op 0x%x hdr->itt 0x%x "
 			  "datalen %d]\n", hdr->opcode & ISCSI_OPCODE_MASK,
 			  hdr->itt, task->data_count);
@@ -1174,7 +1176,8 @@ static int iscsi_handle_reject(struct iscsi_session *session,
 			if (!iscsi_verify_itt(conn, rejected_pdu.itt))
 				task = iscsi_itt_to_task(session, conn,
 							 rejected_pdu.itt);
-			if (!task || task->state == ISCSI_TASK_FREE ||
+			/* Pairs with the release when the task is prepared. */
+			if (!task || smp_load_acquire(&task->state) != ISCSI_TASK_RUNNING ||
 			    task->sc) {
 				iscsi_conn_printk(KERN_ERR, conn,
 						 "Invalid pdu reject. Could "
@@ -1314,7 +1317,9 @@ int __iscsi_complete_pdu(struct iscsi_session *session, struct iscsi_conn *conn,
 	case ISCSI_OP_SCSI_TMFUNC_RSP:
 	case ISCSI_OP_NOOP_IN:
 		task = iscsi_itt_to_task(session, conn, hdr->itt);
-		if (!task || task->state == ISCSI_TASK_FREE || task->sc)
+		/* Pairs with the release when the task is prepared. */
+		if (!task || smp_load_acquire(&task->state) != ISCSI_TASK_RUNNING ||
+		    task->sc)
 			return ISCSI_ERR_BAD_ITT;
 		break;
 	default:
-- 
2.43.0

[PATCH v3 2/3] scsi: libiscsi: do not complete a task sent with a reserved ITT

[Yehyeong Lee]

A NOP-Out sent in answer to a target NOP-In carries RESERVED_ITT.
iscsi_send_nopout() puts RESERVED_ITT in the header and
iscsi_alloc_mgmt_task() keeps it there, because the build_itt() assignment
is skipped for a reserved ITT, so nothing that identifies the task ever
reaches the wire.  No response is expected for such a PDU, so the one
reference the task holds belongs to the transport, which drops it once the
PDU has been sent: libiscsi_tcp and iser do that from their transmit
completion, bnx2i and qedi from their local NOP completion.

task->itt is not touched by any of this.  It stays the task-pool index, so
session->cmds[] still names the task, and iscsi_itt_to_task() looks a task
up by that index alone.  A management response or a Reject PDU naming the
index therefore finds a task that is ISCSI_TASK_RUNNING with no scsi_cmnd
attached, passes both tests, and reaches iscsi_complete_task(), which drops
the reference the transport is about to drop as well.  The window opens
when iscsi_prep_mgmt_task() publishes ISCSI_TASK_RUNNING and closes when
the transport has sent the PDU, and a target that stops reading holds it
open for as long as it likes: iscsi_xmit_task() keeps the task in
conn->task with an extra reference while the transmit keeps failing with
-EAGAIN.

A Task Management Function Response naming the pool index was accepted in
all 11 runs of a KASAN kernel whose window was held open for 200 ms, and
every one of them ended in:

  ------------[ cut here ]------------
  refcount_t: underflow; use-after-free.
  WARNING: lib/refcount.c:37 at refcount_warn_saturate+0xad/0xe0, CPU#0: kworker/u8:4/50
  Modules linked in:
  CPU: 0 UID: 0 PID: 50 Comm: kworker/u8:4 Not tainted 7.4.0-rc2-00472-g76215e83dc82-dirty #1 PREEMPT(lazy)
  Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
  Workqueue: iscsi_q_2 iscsi_xmitworker
  RIP: 0010:refcount_warn_saturate+0xad/0xe0
  Code: a9 9b b7 03 67 48 0f b9 3a 5b 5d c3 cc cc cc cc 48 8d 3d a6 9b b7 03 67 48 0f b9 3a 5b 5d e9 1a 11 33 02 48 8d 3d a3 9b b7 03 <67> 48 0f b9 3a 5b 5d e9 07 11 33 02 48 8d 3d a0 9b b7 03 67 48 0f
  RSP: 0018:ffffc90000347b18 EFLAGS: 00010246
  RAX: 0000000000000003 RBX: ffff88800a74a878 RCX: ffffffff823c2735
  RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffffffff85f3c370
  RBP: 0000000000000003 R08: 0000000000000001 R09: ffffed10014e950f
  R10: ffff88800a74a87b R11: ffff88800c8f26e8 R12: 0000000000000000
  R13: 1ffff92000068f6b R14: ffff88800c75c5c8 R15: ffff88800c8f2400
  FS:  0000000000000000(0000) GS:ffff8880e55a8000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00007f10f26585f0 CR3: 0000000005672002 CR4: 0000000000770ef0
  PKRU: 55555554
  Call Trace:
   <TASK>
   iscsi_xmit_task+0x1c8/0x770
   ? iscsi_sw_tcp_pdu_init+0x268/0xb00
   ? __pfx_iscsi_xmit_task+0x10/0x10
   ? _raw_spin_lock_bh+0x84/0xe0
   ? iscsi_prep_mgmt_task+0x381/0x600
   iscsi_xmitworker+0x1c8/0x8b0
   process_one_work+0x697/0x1060
   ? __pfx___schedule+0x10/0x10
   ? __pfx_process_one_work+0x10/0x10
   ? _raw_spin_lock_irq+0x81/0xe0
   ? __pfx_iscsi_xmitworker+0x10/0x10
   ? assign_work+0x11d/0x370
   worker_thread+0x45b/0xd10
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x2c8/0x3b0
   ? recalc_sigpending+0x15c/0x1e0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x36e/0x5a0
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x572/0xdc0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>
  ---[ end trace 0000000000000000 ]---

The slot is back in the task pool by then, so a concurrent allocation can
hand it out while iscsi_xmit_task() is still working on it.

Refuse such a task in both receive-side lookups.  A task whose
task->hdr->itt is RESERVED_ITT is one the target was never told about, so
no response can name it legitimately.  The test is on the task the lookup
returned rather than on the received ITT, and iscsi_alloc_mgmt_task()
copies the caller's header for every transport, so task->hdr->itt is
RESERVED_ITT there and nowhere else.  It is evaluated last, after the state
test, so it only runs on a task that has been prepared.  With it in place
the same injection was refused in all 10 runs, and a target NOP-In, the
NOP-Out answering it and the initiator's own pings are unaffected.

Fixes: 3e5c28ad0391 ("[SCSI] libiscsi: merge iscsi_mgmt_task and iscsi_cmd_task")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/libiscsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 83d726043d3e8d..220e89d541af68 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -1177,8 +1177,9 @@ static int iscsi_handle_reject(struct iscsi_session *session,
 				task = iscsi_itt_to_task(session, conn,
 							 rejected_pdu.itt);
 			/* Pairs with the release when the task is prepared. */
+			/* A reserved ITT means the transport owns the task. */
 			if (!task || smp_load_acquire(&task->state) != ISCSI_TASK_RUNNING ||
-			    task->sc) {
+			    task->sc || task->hdr->itt == RESERVED_ITT) {
 				iscsi_conn_printk(KERN_ERR, conn,
 						 "Invalid pdu reject. Could "
 						 "not lookup rejected task.\n");
@@ -1318,8 +1319,9 @@ int __iscsi_complete_pdu(struct iscsi_session *session, struct iscsi_conn *conn,
 	case ISCSI_OP_NOOP_IN:
 		task = iscsi_itt_to_task(session, conn, hdr->itt);
 		/* Pairs with the release when the task is prepared. */
+		/* A reserved ITT means the transport owns the task. */
 		if (!task || smp_load_acquire(&task->state) != ISCSI_TASK_RUNNING ||
-		    task->sc)
+		    task->sc || task->hdr->itt == RESERVED_ITT)
 			return ISCSI_ERR_BAD_ITT;
 		break;
 	default:
-- 
2.43.0

[PATCH v3 3/3] scsi: libiscsi: take a transmit reference before a task is published

[Yehyeong Lee]

A task becomes visible to the receive path when its state is stored as
ISCSI_TASK_RUNNING, and the receive path completes it under back_lock
while the transmit path works under frwd_lock.  The transmit path
publishes that state before it owns a reference of its own:

 - iscsi_prep_mgmt_task() and iscsi_prep_scsi_cmd_pdu() store the state
   and the reference is only taken later, in iscsi_xmit_task().  A
   response that lands in between drops the only reference, so the slot
   is back in the pool by the time iscsi_get_task() runs, which then
   warns and drops the PDU.

 - a transport without an xmit workqueue does not call iscsi_xmit_task()
   at all.  iscsi_send_mgmt_task() and iscsi_queuecommand() call
   session->tt->xmit_task() holding only the allocation reference, so
   the slot can be freed and handed out again while the transport is
   still sending from it, and the error path then puts a reference it no
   longer owns.  ihost->workq only exists when iscsi_host_alloc() is
   called with xmit_can_sleep set, which iscsi_tcp and cxgbi do and
   iser, bnx2i, qedi, be2iscsi and qla4xxx do not.

For a SCSI command there is no point publishing the state at all,
because iscsi_itt_to_ctask() does not read it: a task is reachable
through session->cmds[] as soon as iscsi_alloc_task() fills it in, so a
target that answers an ITT it has not been given yet reaches
iscsi_scsi_cmd_rsp() for a command that is still sitting on
conn->cmdqueue in ISCSI_TASK_PENDING.  That drops the only reference,
which returns the pool slot and calls scsi_done() with whatever the
buffer held, and leaves the task on the queue; iscsi_data_xmit() then
builds a PDU from a task whose ->sc has been cleared.

Once session->tt->xmit_task() has failed the task is stored in
conn->task and sent again, without checking whether a response that
arrived while frwd_lock was dropped has completed it in the meantime.
The two senders that have no workqueue report that failure with the
task still ISCSI_TASK_RUNNING, so the receive path can complete it
after back_lock has been dropped and before the caller puts the
reference it was told it still owns.

So: take the transmit reference before the state is stored, in
iscsi_xmit_task() and in the two callers that send a task themselves;
make the SCSI lookup skip a task that has not been sent; finish with a
failed send inside the same back_lock section that tests for it; and do
not hand a task to the transport again once it is no longer running.
The state is read under back_lock, which is where it is written.

Two choices in there are not arbitrary.  The SCSI lookup rejects
ISCSI_TASK_PENDING rather than everything that is not
ISCSI_TASK_RUNNING, because iscsi_eh_abort() deliberately leaves the
receive side running after a successful abort TMF and says so, and a
late response for the ISCSI_TASK_ABRT_TMF task it leaves behind is
absorbed by iscsi_complete_task() today; rejecting it would turn that
into a connection error.  And iscsi_send_mgmt_task(), whose caller
frees the task, parks a failed send in ISCSI_TASK_COMPLETED rather than
ISCSI_TASK_PENDING, because bnx2i_cleanup_task() and
qedi_cleanup_task() read ISCSI_TASK_PENDING as "never sent to us" and
skip their cleanup, and both of those are no-workqueue transports.

All four were reproduced with test-only delays that hold each window
open, in a QEMU guest with KASAN against a userspace target reached
through a proxy that can inject a PDU of its own, and on iSER over
soft-RoCE against an in-guest LIO target.  Every cell is a fresh boot
and each arm was measured five times, with any cell where the window
closed before the response had been processed counted separately.

For the SCSI window the proxy sends a SCSI Response naming a pool index
it has never seen go out, while the command is still on conn->cmdqueue.
Without this patch the slot went back to the pool inside the window in
5 of 5 attempts and the read returned success on a buffer nobody had
written; with it, 0 of 5, because iscsi_itt_to_ctask() refuses the
response and the connection is dropped with ISCSI_ERR_BAD_ITT, which is
what the driver already does with every other unusable ITT.  What the
transmit thread does next with the slot it has lost:

[   14.300338] Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] SMP KASAN NOPTI
[   14.302897] KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
[   14.303981] CPU: 3 UID: 0 PID: 52 Comm: kworker/u16:2 Not tainted 7.4.0-rc2-00473-g7baa29cbc558-dirty #2 PREEMPT(lazy)
[   14.305403] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[   14.306889] Workqueue: iscsi_q_2 iscsi_xmitworker
[   14.307549] RIP: 0010:iscsi_prep_scsi_cmd_pdu+0x2ef/0x2220
[   14.308284] Code: b6 04 02 40 38 f0 7f 08 84 c0 0f 85 c5 0f 00 00 4c 89 e1 41 c6 43 01 01 49 8d 53 08 48 b8 00 00 00 00 00 fc ff df 48 c1 e9 03 <80> 3c 01 00 0f 85 09 17 00 00 48 b9 00 00 00 00 00 fc ff df 49 8b
[   14.310690] RSP: 0018:ffffc90000397b18 EFLAGS: 00010246
[   14.311388] RAX: dffffc0000000000 RBX: ffff88800cfc1800 RCX: 0000000000000000
[   14.312326] RDX: ffff88800cfc1900 RSI: 0000000000000001 RDI: ffff88800cfc1808
[   14.313265] RBP: ffff88800ce40378 R08: 0000000000000001 R09: ffff88800cfc18f8
[   14.314202] R10: 0000000000000000 R11: ffff88800cfc18f8 R12: 0000000000000000
[   14.315139] R13: ffff88800ce42440 R14: ffff88800cfc1858 R15: ffff88800ce40388
[   14.316077] FS:  0000000000000000(0000) GS:ffff8880e5728000(0000) knlGS:0000000000000000
[   14.317135] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   14.317904] CR2: 00000000005a46e2 CR3: 0000000005672006 CR4: 0000000000770ef0
[   14.318844] PKRU: 55555554
[   14.319217] Call Trace:
[   14.319564]  <TASK>
[   14.319872]  iscsi_xmitworker+0x56b/0xf20
[   14.320417]  process_one_work+0x697/0x1060
[   14.320979]  ? __pfx___schedule+0x10/0x10
[   14.321522]  ? __pfx_process_one_work+0x10/0x10
[   14.322136]  ? _raw_spin_lock_irq+0x81/0xe0
[   14.322699]  ? __pfx_iscsi_xmitworker+0x10/0x10
[   14.323313]  ? assign_work+0x11d/0x370
[   14.323830]  worker_thread+0x45b/0xd10
[   14.324341]  ? __pfx_worker_thread+0x10/0x10
[   14.324923]  kthread+0x2c8/0x3b0
[   14.325369]  ? recalc_sigpending+0x15c/0x1e0
[   14.325953]  ? __pfx_kthread+0x10/0x10
[   14.326461]  ret_from_fork+0x36e/0x5a0
[   14.326979]  ? __pfx_ret_from_fork+0x10/0x10
[   14.327556]  ? __switch_to+0x572/0xdc0
[   14.328073]  ? __pfx_kthread+0x10/0x10
[   14.328582]  ret_from_fork_asm+0x1a/0x30
[   14.329119]  </TASK>
[   14.329429] Modules linked in:
[   14.329880] ---[ end trace 0000000000000000 ]---

For the state publication, a NOP-Out answered by the target inside the
window, the slot went back to the pool in 5 of 5 attempts without this
patch and 0 of 5 with it, and the xmit worker hit "WARNING:
drivers/scsi/libiscsi.c:1598 at iscsi_xmit_task+0x6e0/0x920" on the
iscsi_get_task() that then fails.  For conn->task, where the target's
own reply to a ping lands in the window, the completed task was handed
to the transport again in 6 of 6 valid attempts without the patch and 0
of 5 with it.  For the no-workqueue path the reference was put twice in
5 of 5 attempts without the patch and 0 of 5 with it, as "refcount_t:
underflow; use-after-free." from iscsi_send_nopout().

Baseline behaviour is unchanged: discovery, login, write, read-back
compare, a 1 MiB O_DIRECT read, NOP pings and logout pass on iscsi_tcp
and on iSER, and so does an abort TMF driven by swallowing a response -
the path that leaves a task in ISCSI_TASK_ABRT_TMF with the receive
side still up.  cxgbi, bnx2i, qedi, be2iscsi and qla4xxx need vendor
hardware and were not run; their paths were followed in the source
only.

Fixes: 659743b02c41 ("[SCSI] libiscsi: Reduce locking contention in fast path")
Fixes: 533ac412fdb4 ("scsi: iscsi: Remove unneeded task state check")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/libiscsi.c | 58 +++++++++++++++++++++++++++++++++++------
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
index 220e89d541af68..5045f6f5ca01b3 100644
--- a/drivers/scsi/libiscsi.c
+++ b/drivers/scsi/libiscsi.c
@@ -429,8 +429,6 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_task *task)
 	if (session->tt->init_task && session->tt->init_task(task))
 		return -EIO;
 
-	/* Pairs with the acquire in the RX task lookups. */
-	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
 	session->cmdsn++;
 
 	conn->scsicmd_pdus_cnt++;
@@ -703,8 +701,6 @@ static int iscsi_prep_mgmt_task(struct iscsi_conn *conn,
 	if ((hdr->opcode & ISCSI_OPCODE_MASK) == ISCSI_OP_LOGOUT)
 		session->state = ISCSI_STATE_LOGGING_OUT;
 
-	/* Pairs with the acquire in the RX task lookups. */
-	smp_store_release(&task->state, ISCSI_TASK_RUNNING);
 	ISCSI_DBG_SESSION(session, "mgmtpdu [op 0x%x hdr->itt 0x%x "
 			  "datalen %d]\n", hdr->opcode & ISCSI_OPCODE_MASK,
 			  hdr->itt, task->data_count);
@@ -829,7 +825,20 @@ static int iscsi_send_mgmt_task(struct iscsi_task *task)
 		if (rc)
 			return rc;
 
+		iscsi_get_task(task);
+		/* Pairs with the acquire in the RX task lookups. */
+		smp_store_release(&task->state, ISCSI_TASK_RUNNING);
 		rc = session->tt->xmit_task(task);
+		/* regular RX path uses back_lock */
+		spin_lock_bh(&session->back_lock);
+		/* The RX path completing the task is also a successful send. */
+		if (rc && task->state != ISCSI_TASK_RUNNING)
+			rc = 0;
+		else if (rc)
+			/* Keep the RX path off it; the caller still owns the ref. */
+			task->state = ISCSI_TASK_COMPLETED;
+		__iscsi_put_task(session, task);
+		spin_unlock_bh(&session->back_lock);
 		if (rc)
 			return rc;
 	} else {
@@ -1455,6 +1464,10 @@ struct iscsi_task *iscsi_itt_to_ctask(struct iscsi_session *session,
 	if (!task || !task->sc)
 		return NULL;
 
+	/* Pairs with the release in the transmit path. */
+	if (smp_load_acquire(&task->state) == ISCSI_TASK_PENDING)
+		return NULL;
+
 	if (iscsi_cmd(task->sc)->age != session->age) {
 		iscsi_session_printk(KERN_ERR, session,
 				  "task's session age %d, expected %d\n",
@@ -1556,9 +1569,20 @@ static int iscsi_xmit_task(struct iscsi_session *session, struct iscsi_conn *con
 			WARN_ON_ONCE(1);
 			return 0;
 		}
+		if (!was_requeue)
+			/* Pairs with the acquire in the RX task lookups. */
+			smp_store_release(&task->state, ISCSI_TASK_RUNNING);
 	} else {
 		/* Already have a ref from when we failed to send it last call */
 		conn->task = NULL;
+		/* The RX path may have completed it while the tx was stopped. */
+		spin_lock_bh(&session->back_lock);
+		if (task->state != ISCSI_TASK_RUNNING) {
+			__iscsi_put_task(session, task);
+			spin_unlock_bh(&session->back_lock);
+			return 0;
+		}
+		spin_unlock_bh(&session->back_lock);
 	}
 
 	/*
@@ -1584,10 +1608,12 @@ static int iscsi_xmit_task(struct iscsi_session *session, struct iscsi_conn *con
 	spin_unlock_bh(&session->frwd_lock);
 	rc = session->tt->xmit_task(task);
 	spin_lock_bh(&session->frwd_lock);
+	/* regular RX path uses back_lock */
+	spin_lock_bh(&session->back_lock);
 	if (!rc) {
 		/* done with this task */
 		task->last_xfer = jiffies;
-	} else {
+	} else if (task->state == ISCSI_TASK_RUNNING) {
 		/*
 		 * get an extra ref that is released next time we access it
 		 * as conn->task above.
@@ -1596,7 +1622,8 @@ static int iscsi_xmit_task(struct iscsi_session *session, struct iscsi_conn *con
 		conn->task = task;
 	}
 
-	iscsi_put_task(task);
+	__iscsi_put_task(session, task);
+	spin_unlock_bh(&session->back_lock);
 	return rc;
 }
 
@@ -1892,10 +1919,25 @@ enum scsi_qc_status iscsi_queuecommand(struct Scsi_Host *host,
 				goto prepd_fault;
 			}
 		}
-		if (session->tt->xmit_task(task)) {
+		iscsi_get_task(task);
+		/* Pairs with the acquire in the RX task lookups. */
+		smp_store_release(&task->state, ISCSI_TASK_RUNNING);
+		reason = session->tt->xmit_task(task);
+		/* regular RX path uses back_lock */
+		spin_lock_bh(&session->back_lock);
+		/* The RX path completing the task is also a successful send. */
+		if (reason && task->state != ISCSI_TASK_RUNNING)
+			reason = 0;
+		else if (reason)
+			/* Give the command back inside the lock, not after it. */
+			iscsi_complete_task(session, task,
+					    ISCSI_TASK_REQUEUE_SCSIQ);
+		__iscsi_put_task(session, task);
+		spin_unlock_bh(&session->back_lock);
+		if (reason) {
 			session->cmdsn--;
 			reason = FAILURE_SESSION_NOT_READY;
-			goto prepd_reject;
+			goto reject;
 		}
 	} else {
 		list_add_tail(&task->running, &conn->cmdqueue);
-- 
2.43.0

Re: [PATCH v3 3/3] scsi: libiscsi: take a transmit reference before a task is published

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] scsi: libiscsi: UAF on out.segment when iscsi_xmit_task() drops
  completed task
- [High] scsi: libiscsi: incomplete state and ordering check in
  iscsi_itt_to_ctask()
- [High] scsi: libiscsi: completed requeued task sent in
  iscsi_xmit_task()
--

commit 36167e4a6d8cfacb1e2c30d4153e78e02abac811
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: libiscsi: take a transmit reference before a task is published

This commit takes a transmit reference on an iSCSI task before setting its
state to ISCSI_TASK_RUNNING, checks task->state in iscsi_itt_to_ctask(),
and avoids resending or leaking tasks completed by the receive path during
transmission.

> diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c
> index 220e89d541af6..5045f6f5ca01b 100644
> --- a/drivers/scsi/libiscsi.c
> +++ b/drivers/scsi/libiscsi.c

[ ... ]

> @@ -1455,6 +1464,10 @@ struct iscsi_task *iscsi_itt_to_ctask(struct iscsi_session *session,
>  	if (!task || !task->sc)
>  		return NULL;
>  
> +	/* Pairs with the release in the transmit path. */
> +	if (smp_load_acquire(&task->state) == ISCSI_TASK_PENDING)
> +		return NULL;
> +
>  	if (iscsi_cmd(task->sc)->age != session->age) {
>  		iscsi_session_printk(KERN_ERR, session,
>  				  "task's session age %d, expected %d\n",

[Severity: High]
Can iscsi_itt_to_ctask() still return an unsent or already-completed task
here?

First, iscsi_alloc_task() writes task->state = ISCSI_TASK_PENDING and
task->sc = sc under frwd_lock with plain stores and no barrier between
them, while iscsi_itt_to_ctask() reads task->sc before
smp_load_acquire(&task->state):

CPU0 (frwd_lock)                  CPU1 (back_lock)
iscsi_alloc_task()                iscsi_itt_to_ctask()
  task->state = ISCSI_TASK_PENDING
  task->sc = sc
                                    if (!task || !task->sc)
                                    smp_load_acquire(&task->state)
                                      --> sees ISCSI_TASK_FREE

Because smp_load_acquire() only orders loads after it and
iscsi_alloc_task() has no release or write barrier before setting task->sc,
CPU1 can observe the new task->sc while still reading the prior
ISCSI_TASK_FREE state, bypassing the ISCSI_TASK_PENDING check and freeing
the task while it stays on conn->cmdqueue.

Second, while the transmit path holds a second reference on task (for
example across session->tt->xmit_task() or in conn->task), completing the
task on the receive path sets task->state to ISCSI_TASK_COMPLETED and
decrements refcount from 2 to 1 without clearing task->sc.  Because
iscsi_itt_to_ctask() only rejects ISCSI_TASK_PENDING, a subsequent
ISCSI_OP_SCSI_DATA_IN PDU for that ITT still resolves the completed task:

drivers/scsi/libiscsi_tcp.c:iscsi_tcp_hdr_dissect() {
	...
	case ISCSI_OP_SCSI_DATA_IN:
		spin_lock(&session->back_lock);
		task = iscsi_itt_to_ctask(session, conn, hdr->itt);
		if (!task)
			rc = ISCSI_ERR_BAD_ITT;
		else
			rc = iscsi_tcp_data_in(conn, task);
		spin_unlock(&session->back_lock);
	...
}

iscsi_tcp_data_in() points tcp_conn->in.segment at task->sc->sdb.table.sgl
and drops back_lock without a task reference; when the transmit path then
puts its final reference, iscsi_free_task() calls scsi_done() and frees the
scatterlist while iscsi_tcp_recv_skb() writes payload bytes into it
(use-after-free).  A duplicate ISCSI_OP_SCSI_CMD_RSP in that window will
also overwrite sc->result and sense data before scsi_done() runs.

> @@ -1556,9 +1569,20 @@ static int iscsi_xmit_task(struct iscsi_session *session, struct iscsi_conn *con
>  			WARN_ON_ONCE(1);
>  			return 0;
>  		}
> +		if (!was_requeue)
> +			/* Pairs with the acquire in the RX task lookups. */
> +			smp_store_release(&task->state, ISCSI_TASK_RUNNING);
>  	} else {
>  		/* Already have a ref from when we failed to send it last call */
>  		conn->task = NULL;
> +		/* The RX path may have completed it while the tx was stopped. */
> +		spin_lock_bh(&session->back_lock);
> +		if (task->state != ISCSI_TASK_RUNNING) {
> +			__iscsi_put_task(session, task);
> +			spin_unlock_bh(&session->back_lock);
> +			return 0;
> +		}
> +		spin_unlock_bh(&session->back_lock);
>  	}

[Severity: High]
When conn->task is NULL and was_requeue is true, can iscsi_xmit_task()
still pass a completed task to session->tt->xmit_task()?

When iscsi_tcp_r2t_rsp() queues a task onto conn->requeue, it takes an
extra reference on task.  If an early response completes the task before
iscsi_data_xmit() pops it from conn->requeue, task->state becomes
ISCSI_TASK_COMPLETED while the R2T reference keeps the task alive:

CPU0 (RX, back_lock)              CPU1 (xmit worker, frwd_lock)
iscsi_tcp_r2t_rsp()
  iscsi_get_task(task)
  iscsi_requeue_task(task)
iscsi_complete_task()
  task->state = ISCSI_TASK_COMPLETED
  __iscsi_put_task() (refcount 2->1)
                                  iscsi_data_xmit()
                                    iscsi_xmit_task(..., was_requeue=true)
                                      if (!conn->task)
                                        iscsi_get_task(task)
                                      if (was_requeue)
                                        iscsi_put_task(task)
                                      session->tt->xmit_task(task)

Because the !conn->task branch does not check task->state under back_lock
when was_requeue is true, it calls session->tt->xmit_task() on the
completed task.  If xmit_task() then returns -EAGAIN after
iscsi_sw_tcp_pdu_init() points tcp_sw_conn->out.segment at task->hdr and
task->sc->sdb.table.sgl, the post-xmit check sees task->state !=
ISCSI_TASK_RUNNING, skips saving conn->task, and drops the final reference,
freeing task and task->sc while tcp_sw_conn->out.segment still references
them.

> @@ -1584,10 +1608,12 @@ static int iscsi_xmit_task(struct iscsi_session *session, struct iscsi_conn *con
>  	spin_unlock_bh(&session->frwd_lock);
>  	rc = session->tt->xmit_task(task);
>  	spin_lock_bh(&session->frwd_lock);
> +	/* regular RX path uses back_lock */
> +	spin_lock_bh(&session->back_lock);
>  	if (!rc) {
>  		/* done with this task */
>  		task->last_xfer = jiffies;
> -	} else {
> +	} else if (task->state == ISCSI_TASK_RUNNING) {
>  		/*
>  		 * get an extra ref that is released next time we access it
>  		 * as conn->task above.
> @@ -1596,7 +1622,8 @@ static int iscsi_xmit_task(struct iscsi_session *session, struct iscsi_conn *con
>  		conn->task = task;
>  	}
>  
> -	iscsi_put_task(task);
> +	__iscsi_put_task(session, task);
> +	spin_unlock_bh(&session->back_lock);
>  	return rc;
>  }

[Severity: High]
On iscsi_tcp, if session->tt->xmit_task() returns -EAGAIN after partially
sending a PDU and the receive path has completed the task in the meantime,
does dropping task here cause a use-after-free on tcp_sw_conn->out.segment?

During session->tt->xmit_task(), iscsi_sw_tcp_pdu_init() sets the
per-connection tcp_sw_conn->out.segment and out.data_segment to point at
task->hdr and task->sc->sdb.table.sgl.  If the socket buffer fills up
mid-PDU (-EAGAIN) and an incoming response completes the task either before
back_lock is taken here or while the task was parked in conn->task,
__iscsi_put_task() drops the last reference:

CPU0 (xmit worker)                CPU1 (RX, back_lock)
iscsi_xmit_task()
  session->tt->xmit_task(task)
    iscsi_sw_tcp_pdu_init()
      out.segment -> task->sc->sdb
    iscsi_sw_tcp_pdu_xmit()
      -> -EAGAIN (unsent bytes left)
                                  iscsi_complete_task(task)
                                    task->state = ISCSI_TASK_COMPLETED
                                    __iscsi_put_task() (refcount 2->1)
  spin_lock_bh(&session->back_lock)
  /* task->state != RUNNING:
   * conn->task not set
   */
  __iscsi_put_task() (refcount 1->0)
    iscsi_free_task()
      scsi_done(sc) (frees sgl)
...
iscsi_data_xmit() (next requeued task)
  iscsi_tcp_task_xmit()
    session->tt->xmit_pdu()
      iscsi_sw_tcp_xmit()
        --> reads freed sgl in out.segment  <-- use-after-free

iscsi_tcp_cleanup_task() only drains tcp_task->r2tqueue and does not reset
tcp_sw_conn->out.segment:

drivers/scsi/libiscsi_tcp.c:iscsi_tcp_task_xmit() {
	...
flush:
	/* Flush any pending data first. */
	rc = session->tt->xmit_pdu(task);
	if (rc < 0)
		return rc;
	...
}

When the next task on conn->requeue runs iscsi_tcp_task_xmit(), it flushes
the remaining bytes from tcp_sw_conn->out.segment before calling init_pdu(),
reading from the freed scatterlist and pages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115122122.1446791-1-yhlee@isslab.korea.ac.kr?part=3