All of lore.kernel.org
 help / color / mirror / Atom feed
From: Leon Romanovsky <leon@kernel.org>
To: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
Cc: sagi@grimberg.me, jgg@ziepe.ca, nab@linux-iscsi.org,
	michael.christie@oracle.com, martin.petersen@oracle.com,
	linux-rdma@vger.kernel.org, target-devel@vger.kernel.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] IB/isert: post full-feature receive buffers after session registration
Date: Wed, 29 Jul 2026 16:01:41 +0300	[thread overview]
Message-ID: <20260729130141.GN12003@unreal> (raw)
In-Reply-To: <20260726144213.933544-1-yhlee@isslab.korea.ac.kr>

On Sun, Jul 26, 2026 at 11:42:13PM +0900, Yehyeong Lee wrote:
> isert_put_login_tx() posts the full-feature receive buffers and then
> sends the final Login Response on the same call:
> 
> 	isert_alloc_rx_descriptors()
> 	isert_post_recvm(isert_conn, ISERT_QP_MAX_RECV_DTOS)
> 	isert_conn->state = ISER_CONN_FULL_FEATURE
> 	isert_login_post_send()
> 
> iscsi_post_login_handler() only runs after that, and it is what calls
> __transport_register_session(), which sets se_sess->se_tpg.  An
> initiator that sends a SCSI command as soon as it sees the Login
> Response can therefore have that command received, executed and
> completed before the session is registered, and target_complete()
> dereferences a NULL se_sess->se_tpg:
> 
>   Oops: general protection fault, probably for non-canonical address ...
>   KASAN: null-ptr-deref in range [0x0000000000000080-0x0000000000000087]
>   Workqueue: ib-comp-wq ib_cq_poll_work
>   RIP: 0010:target_complete_cmd_with_sense.part.0+0x17d/0xc90
>   Call Trace:
>    fd_execute_rw
>    __target_execute_cmd
>    iscsit_execute_cmd
>    iscsit_sequence_cmd
>    isert_recv_done
>    __ib_process_cq
>    ib_cq_poll_work
> 
> The command reaches the backend because transport_lookup_cmd_lun() does
> not need se_tpg on the mapped-LUN path; it uses se_sess->se_node_acl,
> which iscsit sets much earlier in iscsi_target_locate_portal().
> 
> iscsit's own RX thread does not have this problem: it is started at the
> same point but waits on conn->rx_login_comp, which is completed at the
> end of iscsi_post_login_handler(), after __transport_register_session().
> isert takes its PDUs from the completion queue instead, and
> isert_recv_done() has no connection-state check.
> 
> Post the receive buffers from isert_get_rx_pdu() instead.  That callback
> is invoked by iscsi_target_rx_thread() only after the same rx_login_comp
> wait, so the buffers become available to the HCA only once the session
> is registered.  isert_recv_done() runs on ib-comp-wq and cannot wait
> there itself.  The allocation stays in isert_put_login_tx() so that a
> memory allocation failure still cannot happen after the final Login
> Response has been sent.
> 
> rx_descs can legitimately be NULL in isert_get_rx_pdu(): the login
> timeout timer sets login->login_failed from timer context, so
> isert_put_login_tx() can skip the allocation branch while
> iscsi_target_do_login() still returns 1 and the connection reaches
> TARG_CONN_STATE_LOGGED_IN.  Return early in that case, which matches the
> existing behaviour of not posting at all on that path.
> 
> Reproduced with soft-RoCE (rdma_rxe) under KASAN using an initiator that
> issues a WRITE immediately after login, 400 login cycles per run.
> Unpatched: 9 of 10 runs oopsed.  Patched: 0 of 10.  A build carrying a
> diagnostic that reports the NULL without faulting recorded 16 hits
> across 9 of 15 runs unpatched and 0 across 15 runs patched; the two sets
> were run alternately in a single session.
> 
> Not covered by that testing: discovery sessions over iSER, MC/S
> (non-zero TSIH) logins, ERL=2 connection recovery, and real HCAs, where
> RNR behaviour differs from soft-RoCE.
> 
> Fixes: b8d26b3be8b3 ("iser-target: Add iSCSI Extensions for RDMA (iSER) target driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
> This replaces "scsi: target: Fix NULL se_tpg dereference in
> target_complete()" (target-devel, Message-ID
> 20260725165818.769963-1-yhlee@isslab.korea.ac.kr), which guarded the
> symptom in target core.  That patch is being withdrawn.
> 
>  drivers/infiniband/ulp/isert/ib_isert.c | 36 +++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 5 deletions(-)

1. Please keep commit messages concise and easy to read.
2. Include the full kernel Oops in the commit message.
3. Keep code comments to a minimum.
4. Please address Sashiko's concerns about this patch.

Thanks

      reply	other threads:[~2026-07-29 13:01 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-26 14:42 [PATCH] IB/isert: post full-feature receive buffers after session registration Yehyeong Lee
2026-07-29 13:01 ` Leon Romanovsky [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260729130141.GN12003@unreal \
    --to=leon@kernel.org \
    --cc=jgg@ziepe.ca \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=martin.petersen@oracle.com \
    --cc=michael.christie@oracle.com \
    --cc=nab@linux-iscsi.org \
    --cc=sagi@grimberg.me \
    --cc=stable@vger.kernel.org \
    --cc=target-devel@vger.kernel.org \
    --cc=yhlee@isslab.korea.ac.kr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.