[PATCH] IB/iser: skip an unregistered direction on remote invalidation

Yehyeong Lee posted 1 patch 1 week, 4 days ago
drivers/infiniband/ulp/iser/iser_initiator.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] IB/iser: skip an unregistered direction on remote invalidation
Posted by Yehyeong Lee 1 week, 4 days ago
A write command whose data is sent entirely as immediate data is not
registered.  iser_reg_mem_fastreg() takes the DMA key path and leaves
rdma_reg[ISER_DIR_OUT].desc at NULL, while iser_dma_map_task_data() has
already set dir[ISER_DIR_OUT].

iser_check_remote_inv() looks at dir[] alone and hands the descriptor to
iser_inv_desc(), which reads desc->sig_protected.  A target that answers
such a command with IB_WR_SEND_WITH_INV faults the initiator.

Leaving those commands unregistered is deliberate.  What is missing is a
check that the target does not invalidate one anyway.

Skip a direction that was not registered.  Returning an error instead
would break a bidirectional command whose read half was registered and
whose write half was immediate, which a conforming target may invalidate.

  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000004: 0000 [#1] SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
  CPU: 0 UID: 0 PID: 40 Comm: kworker/u8:2 Not tainted 7.2.0-rc5-ISERHOST-gf5098b6bae76-dirty #3 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: rxe_wq do_work
  RIP: 0010:iser_task_rsp+0x6d6/0xec0
  Code: 48 c1 ea 03 80 3c 02 00 0f 85 ba 06 00 00 48 8b 9b 78 01 00 00 48 b8 00 00 00 00 00 fc ff df 48 8d 7b 20 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 06 0f 8e 76 06 00 00 80 7b 20 00 0f 84 3d 04
  RSP: 0018:ffff88811b008db8 EFLAGS: 00010202
  RAX: dffffc0000000000 RBX: 0000000000000000 RCX: 0000000000001848
  RDX: 0000000000000004 RSI: 1ffff11021587b12 RDI: 0000000000000020
  RBP: ffff88810adc1ae4 R08: ffff888109b7f860 R09: ffffffff90a922c0
  R10: ffff88810adc1a1c R11: 000000000000003c R12: ffff888109b7f800
  R13: ffff88810adc1acc R14: ffff888109b7f820 R15: 0000000000000000
  FS:  0000000000000000(0000) GS:ffff88818a676000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00000000005afe2b CR3: 000000010af23005 CR4: 0000000000770ef0
  PKRU: 55555554
  Call Trace:
   <IRQ>
   __ib_process_cq+0xe1/0x390
   ib_poll_handler+0x6e/0x200
   irq_poll_softirq+0x1df/0x480
   ? clockevents_program_event+0x2ba/0x860
   ? __pfx_irq_poll_softirq+0x10/0x10
   handle_softirqs+0x18e/0x590
   ? __pfx_handle_softirqs+0x10/0x10
   ? __hrtimer_rearm_deferred+0x156/0x450
   do_softirq+0x3b/0x60
   </IRQ>
   <TASK>
   __local_bh_enable_ip+0x61/0x70
   __alloc_skb+0x732/0x890
   ? _raw_spin_lock_irqsave+0x85/0xe0
   ? __pfx___alloc_skb+0x10/0x10
   ? _raw_read_unlock_irqrestore+0x16/0x50
   rxe_init_packet+0x16b/0x4f0
   prepare_ack_packet+0xb8/0x830
   rxe_receiver+0x499/0x9980
   ? __pfx_rxe_receiver+0x10/0x10
   ? rxe_completer+0x29e5/0x38c0
   ? hrtimer_start_range_ns_common+0x75f/0x1730
   ? hrtimer_start_range_ns+0xa6/0x2c0
   ? __pfx__raw_spin_lock_irqsave+0x10/0x10
   ? __pfx_rxe_receiver+0x10/0x10
   do_work+0x144/0x470
   process_one_work+0x633/0x1030
   ? assign_work+0x11d/0x370
   worker_thread+0x45b/0xd10
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x2c6/0x3b0
   ? recalc_sigpending+0x15c/0x1e0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x36e/0x5a0
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x572/0xdd0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>
  Modules linked in:
  ---[ end trace 0000000000000000 ]---

Fixes: 59caaed7a72a ("IB/iser: Support the remote invalidation exception")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
Measured over rxe with KASAN, with an isert target modified to answer a
command with SEND_WITH_INV carrying an rkey the initiator had registered
for a different command: of the runs that reached the device, 3 of 3
faulted without this patch and 0 of 5 with it, on 7.2-rc5 with no other
change.  A counter on the new branch fired 20 times per run against that
target and not once in three runs against an unmodified one, which
completed 2000 immediate-data writes each.
 drivers/infiniband/ulp/iser/iser_initiator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 12a2d12fef070..25875c6805abf 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -611,13 +611,13 @@ static int iser_check_remote_inv(struct iser_conn *iser_conn, struct ib_wc *wc,
 
 			if (iser_task->dir[ISER_DIR_IN]) {
 				desc = iser_task->rdma_reg[ISER_DIR_IN].desc;
-				if (unlikely(iser_inv_desc(desc, rkey)))
+				if (desc && unlikely(iser_inv_desc(desc, rkey)))
 					return -EINVAL;
 			}
 
 			if (iser_task->dir[ISER_DIR_OUT]) {
 				desc = iser_task->rdma_reg[ISER_DIR_OUT].desc;
-				if (unlikely(iser_inv_desc(desc, rkey)))
+				if (desc && unlikely(iser_inv_desc(desc, rkey)))
 					return -EINVAL;
 			}
 		} else {
-- 
2.43.0
Re: [PATCH] IB/iser: skip an unregistered direction on remote invalidation
Posted by Yehyeong Lee 1 week ago
The AI review on this patch asks two things: whether a bogus remote
invalidation is now silently accepted, and whether that desynchronises
the MR state.  The first is true by design; the second does not happen.

need_inval lives in the ib_mr inside struct iser_fr_desc
(iscsi_iser.h:320 and :333).  With no descriptor there is no flag to
clear, and the cleanup path already returns early on the same condition
this patch adds - iser_unreg_mem_fastreg() at iser_memory.c:156.  A
need_inval left true is cleared by the next registration, which posts
LOCAL_INV ahead of the registration work request (iser_memory.c:316).
The state that would be wrong is need_inval false with a live MR, and
skipping never clears it.

With the default always_register, desc is NULL only for a write sent
entirely as immediate data (iser_memory.c:364, iser_initiator.c:105),
and that path advertises no stag (iser_initiator.c:116), so a conforming
target has nothing to invalidate for such a task.  Returning an error
would fail the whole connection (iser_initiator.c:657).

Two corrections to what I wrote earlier.  The claim that returning an
error would break a bidirectional command, which I made under the ---
and again in a follow-up note, is wrong: ISCSI_FLAG_CMD_READ and
ISCSI_FLAG_CMD_WRITE are set in one if/else in libiscsi.c:368-417, so
they cannot both be set.  And the eight-versus-132 and ten-versus-three
counts in that note were each from a single run; two other runs of the
error-returning build saw three invalidations and no rejections.

Best regards,

Yehyeong Lee