[PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Kyle Mahlkuch]

This patch addresses critical race conditions in the lpfc driver's ELS
retry event handling that can lead to a use-after-free.

The primary issue is a TOCTOU (Time-of-Check to Tim-of-Use) race in the
lpfc_cancel_retry_delay_tmo(), where the NLP_DELAY_TMO flag is cleared
before acquiring the lock to check if the retry event is queued, and the
worker lpfc_els_retry_delay_handler(). This create a window where the
timer can be rescheduled and fire, causing both the cancel path and the
worker thread to release the same reference, resulting in a double-put
and use-after-free.

Fixes the primary TOCTOU race by
  - moving the flag check inside section protected by hbalock
  - Add a NULL checking in the work handler to gracefully handle cases
    where the event payload has been consumed by the cancel path

Signed-off-by: Thinh Tran <thinhtr@linux.ibm.com>
Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
---
  drivers/scsi/lpfc/lpfc_els.c     | 48 +++++++++++++++++++++++++-------
  drivers/scsi/lpfc/lpfc_hbadisc.c |  9 ++++++
  2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index b71db7d7d747..ccc0734f5daa 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -4329,18 +4329,40 @@ lpfc_issue_els_edc(struct lpfc_vport *vport, 
uint8_t retry)
  void
  lpfc_cancel_retry_delay_tmo(struct lpfc_vport *vport, struct 
lpfc_nodelist *nlp)
  {
-    struct lpfc_work_evt *evtp;
+    struct lpfc_hba *phba = vport->phba;
+    struct lpfc_work_evt *evtp = &nlp->els_retry_evt;
+    struct lpfc_nodelist *arg_ndlp = NULL;
+    unsigned long flags;

-    if (!test_and_clear_bit(NLP_DELAY_TMO, &nlp->nlp_flag))
+    /*
+     * Check and clear NLP_DELAY_TMO flag inside critical section to
+     * prevent TOCTOU race with timer rescheduling. If retry event is
+     * queued, remove it and consume its payload to prevent double-put.
+     * This protects against concurrent execution with 
lpfc_work_list_done()
+     * which may be processing this event. The event holds a reference to
+     * the nodelist that must be released exactly once.
+     */
+    spin_lock_irqsave(&phba->hbalock, flags);
+    if (!test_and_clear_bit(NLP_DELAY_TMO, &nlp->nlp_flag)) {
+        spin_unlock_irqrestore(&phba->hbalock, flags);
          return;
+    }
+
+    if (!list_empty(&evtp->evt_listp)) {
+        list_del_init(&evtp->evt_listp);
+        arg_ndlp = (struct lpfc_nodelist *)evtp->evt_arg1;
+        evtp->evt_arg1 = NULL;
+    }
+    spin_unlock_irqrestore(&phba->hbalock, flags);
+
+    /* Delete timer and clear state outside the lock */
      timer_delete_sync(&nlp->nlp_delayfunc);
      nlp->nlp_last_elscmd = 0;
-    if (!list_empty(&nlp->els_retry_evt.evt_listp)) {
-        list_del_init(&nlp->els_retry_evt.evt_listp);
-        /* Decrement nlp reference count held for the delayed retry */
-        evtp = &nlp->els_retry_evt;
-        lpfc_nlp_put((struct lpfc_nodelist *)evtp->evt_arg1);
-    }
+
+    /* Drop the event-held reference */
+    if (arg_ndlp)
+        lpfc_nlp_put(arg_ndlp);
+
      if (test_and_clear_bit(NLP_NPR_2B_DISC, &nlp->nlp_flag)) {
          if (vport->num_disc_nodes) {
              if (vport->port_state < LPFC_VPORT_READY) {
@@ -4422,10 +4444,16 @@ lpfc_els_retry_delay_handler(struct 
lpfc_nodelist *ndlp)
      spin_lock_irq(&ndlp->lock);
      cmd = ndlp->nlp_last_elscmd;
      ndlp->nlp_last_elscmd = 0;
-    spin_unlock_irq(&ndlp->lock);

-    if (!test_and_clear_bit(NLP_DELAY_TMO, &ndlp->nlp_flag))
+    /*
+     * Check and clear NLP_DELAY_TMO flag inside critical section to
+         * prevent TOCTOU race with lpfc_cancel_retry_delay_tmo()
+     */
+    if (!test_and_clear_bit(NLP_DELAY_TMO, &ndlp->nlp_flag)) {
+        spin_unlock_irq(&ndlp->lock);
          return;
+    }
+    spin_unlock_irq(&ndlp->lock);

      /*
       * If a discovery event readded nlp_delayfunc after timer
diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c 
b/drivers/scsi/lpfc/lpfc_hbadisc.c
index 43d246c5c049..e318e3f5aa7c 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -846,6 +846,15 @@ lpfc_work_list_done(struct lpfc_hba *phba)
          switch (evtp->evt) {
          case LPFC_EVT_ELS_RETRY:
              ndlp = (struct lpfc_nodelist *) (evtp->evt_arg1);
+            /*
+            * Consume the payload to prevent reuse or double-put.
+            * evt_arg1 was populated when event was queued.
+            */
+            evtp->evt_arg1 = NULL;
+            if (!ndlp) {
+                /* Event already consumed by cancel path */
+                break;
+            }
              if (!hba_pci_err) {
                  lpfc_els_retry_delay_handler(ndlp);
                  free_evt = 0; /* evt is part of ndlp */
-- 
2.52.0

Re: [PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Justin Tee]

Hi Kyle,

Broadcom is currently reviewing this patch set and will report back.

Regards,
Justin Tee

Re: [PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Daniel Wagner]

Hi Justin,

On Mon, Apr 13, 2026 at 09:28:38AM -0700, Justin Tee wrote:
> Broadcom is currently reviewing this patch set and will report back.

Any updates here? We just started to see crashes in our QA which
show the same backtrace. I am going to ship these patches to our QA to
see if it addresses the issue we are seeing. But it would be great to
get this reviewed too.

Thanks,
Daniel

Re: [PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Paul Ely]

[-- Attachment #1.1: Type: text/plain, Size: 1280 bytes --]

Hello Daniel,

I have been testing Patch 1 of 3 for a while now but no reproduction.  I
reviewed Patch 1 of 3 yesterday with Justin and we think it needs some
minor rework.  The hbalock is not required around the bitops functions but
generally, yes, it does seem to
close a hole with ELS Delay retry.  I wanted to review the logs again today
to see what is necessary to get this race in the first place because
Broadcom stresses this path a lot during our testing and we just don't see
this.  Our reproduction of this issue
on x86 and P10 with the Avocado framework did not produce the same crash
either; the crashes were very different.

Is a bug opened at SUSE for the crash?  I would like to see what testing
SUSE is doing to produce this.

Paul





On Thu, Jun 18, 2026 at 7:42 AM Daniel Wagner <dwagner@suse.de> wrote:

> Hi Justin,
>
> On Mon, Apr 13, 2026 at 09:28:38AM -0700, Justin Tee wrote:
> > Broadcom is currently reviewing this patch set and will report back.
>
> Any updates here? We just started to see crashes in our QA which
> show the same backtrace. I am going to ship these patches to our QA to
> see if it addresses the issue we are seeing. But it would be great to
> get this reviewed too.
>
> Thanks,
> Daniel
>
>

[-- Attachment #1.2: Type: text/html, Size: 2553 bytes --]

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5453 bytes --]

Re: [PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Kyle Mahlkuch]

On 6/18/26 8:39 AM, Paul Ely wrote:
> Hello Daniel,
> 
> I have been testing Patch 1 of 3 for a while now but no reproduction.  I 
> reviewed Patch 1 of 3 yesterday with Justin and we think it needs some 
> minor rework.  

Hi Paul,
If you have a reworked patch I'd be happy to test it.

Thanks,
Kyle

Re: [PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Paul Ely]

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

Hello Kyle,

Sorry for the long delay.

I think the routine lpfc_cancel_retry_delay_tmo needs to do the
test_and_clear_bit op first and without a lock.  The Linux BitOps guarantee
atomicity.

Then the timer_delete_sync needs to run next.  This two operations are
the primary
purpose of the routine and try to close the window with a schedule
retry delay timer.

I think the hbalock around the evtp->evt_listp is correct.  The
evtp_listp is a phba->worklist
controlled element so the coarse grain hbalock is required.

The rest of the patch looks good.

I'll post my version to the bug we are using to track this issue.

Thanks

Paul

On Mon, Jul 13, 2026 at 12:26 PM Kyle Mahlkuch <kmahlkuc@linux.ibm.com> wrote:
>
> On 6/18/26 8:39 AM, Paul Ely wrote:
> > Hello Daniel,
> >
> > I have been testing Patch 1 of 3 for a while now but no reproduction.  I
> > reviewed Patch 1 of 3 yesterday with Justin and we think it needs some
> > minor rework.
>
> Hi Paul,
> If you have a reworked patch I'd be happy to test it.
>
> Thanks,
> Kyle

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5453 bytes --]

Re: [PATCH 1/3] scsi: lpfc: Fix race conditions in ELS retry handling

[Maram Srimannarayana Murthy]

On 09/04/26 8:42 pm, Kyle Mahlkuch wrote:
> This patch addresses critical race conditions in the lpfc driver's ELS
> retry event handling that can lead to a use-after-free.
>
> The primary issue is a TOCTOU (Time-of-Check to Tim-of-Use) race in the
> lpfc_cancel_retry_delay_tmo(), where the NLP_DELAY_TMO flag is cleared
> before acquiring the lock to check if the retry event is queued, and the
> worker lpfc_els_retry_delay_handler(). This create a window where the
> timer can be rescheduled and fire, causing both the cancel path and the
> worker thread to release the same reference, resulting in a double-put
> and use-after-free.
>
> Fixes the primary TOCTOU race by
>  - moving the flag check inside section protected by hbalock
>  - Add a NULL checking in the work handler to gracefully handle cases
>    where the event payload has been consumed by the cancel path
>
> Signed-off-by: Thinh Tran <thinhtr@linux.ibm.com>
> Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
> ---


Tested-by: Maram Srimannarayana Murthy <msmurthy@linux.ibm.com>

Tested the complete 3-patch series on an IBM Power11 (ppc64le) server 
equipped with an Emulex FC HBA.
The patches applied cleanly, and FC driver parameter validation testing 
was executed continuously for 36 hours.
No crashes, hangs, or functional issues were observed during the test 
period.

Thanks,
Maram Srimannarayana Murthy


>  drivers/scsi/lpfc/lpfc_els.c | 48 +++++++++++++++++++++++++-------
>  drivers/scsi/lpfc/lpfc_hbadisc.c |  9 ++++++
>  2 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index b71db7d7d747..ccc0734f5daa 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -4329,18 +4329,40 @@ lpfc_issue_els_edc(struct lpfc_vport *vport, 
> uint8_t retry)
>  void
>  lpfc_cancel_retry_delay_tmo(struct lpfc_vport *vport, struct 
> lpfc_nodelist *nlp)
>  {
> -    struct lpfc_work_evt *evtp;
> +    struct lpfc_hba *phba = vport->phba;
> +    struct lpfc_work_evt *evtp = &nlp->els_retry_evt;
> +    struct lpfc_nodelist *arg_ndlp = NULL;
> +    unsigned long flags;
>
> -    if (!test_and_clear_bit(NLP_DELAY_TMO, &nlp->nlp_flag))
> +    /*
> +     * Check and clear NLP_DELAY_TMO flag inside critical section to
> +     * prevent TOCTOU race with timer rescheduling. If retry event is
> +     * queued, remove it and consume its payload to prevent double-put.
> +     * This protects against concurrent execution with 
> lpfc_work_list_done()
> +     * which may be processing this event. The event holds a 
> reference to
> +     * the nodelist that must be released exactly once.
> +     */
> +    spin_lock_irqsave(&phba->hbalock, flags);
> +    if (!test_and_clear_bit(NLP_DELAY_TMO, &nlp->nlp_flag)) {
> +        spin_unlock_irqrestore(&phba->hbalock, flags);
>          return;
> +    }
> +
> +    if (!list_empty(&evtp->evt_listp)) {
> +        list_del_init(&evtp->evt_listp);
> +        arg_ndlp = (struct lpfc_nodelist *)evtp->evt_arg1;
> +        evtp->evt_arg1 = NULL;
> +    }
> +    spin_unlock_irqrestore(&phba->hbalock, flags);
> +
> +    /* Delete timer and clear state outside the lock */
>      timer_delete_sync(&nlp->nlp_delayfunc);
>      nlp->nlp_last_elscmd = 0;
> -    if (!list_empty(&nlp->els_retry_evt.evt_listp)) {
> -        list_del_init(&nlp->els_retry_evt.evt_listp);
> -        /* Decrement nlp reference count held for the delayed retry */
> -        evtp = &nlp->els_retry_evt;
> -        lpfc_nlp_put((struct lpfc_nodelist *)evtp->evt_arg1);
> -    }
> +
> +    /* Drop the event-held reference */
> +    if (arg_ndlp)
> +        lpfc_nlp_put(arg_ndlp);
> +
>      if (test_and_clear_bit(NLP_NPR_2B_DISC, &nlp->nlp_flag)) {
>          if (vport->num_disc_nodes) {
>              if (vport->port_state < LPFC_VPORT_READY) {
> @@ -4422,10 +4444,16 @@ lpfc_els_retry_delay_handler(struct 
> lpfc_nodelist *ndlp)
>      spin_lock_irq(&ndlp->lock);
>      cmd = ndlp->nlp_last_elscmd;
>      ndlp->nlp_last_elscmd = 0;
> -    spin_unlock_irq(&ndlp->lock);
>
> -    if (!test_and_clear_bit(NLP_DELAY_TMO, &ndlp->nlp_flag))
> +    /*
> +     * Check and clear NLP_DELAY_TMO flag inside critical section to
> +         * prevent TOCTOU race with lpfc_cancel_retry_delay_tmo()
> +     */
> +    if (!test_and_clear_bit(NLP_DELAY_TMO, &ndlp->nlp_flag)) {
> +        spin_unlock_irq(&ndlp->lock);
>          return;
> +    }
> +    spin_unlock_irq(&ndlp->lock);
>
>      /*
>       * If a discovery event readded nlp_delayfunc after timer
> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c 
> b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index 43d246c5c049..e318e3f5aa7c 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
> @@ -846,6 +846,15 @@ lpfc_work_list_done(struct lpfc_hba *phba)
>          switch (evtp->evt) {
>          case LPFC_EVT_ELS_RETRY:
>              ndlp = (struct lpfc_nodelist *) (evtp->evt_arg1);
> +            /*
> +            * Consume the payload to prevent reuse or double-put.
> +            * evt_arg1 was populated when event was queued.
> +            */
> +            evtp->evt_arg1 = NULL;
> +            if (!ndlp) {
> +                /* Event already consumed by cancel path */
> +                break;
> +            }
>              if (!hba_pci_err) {
>                  lpfc_els_retry_delay_handler(ndlp);
>                  free_evt = 0; /* evt is part of ndlp */