* [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling
@ 2026-11-11 17:47 Kyle Mahlkuch
2026-11-11 18:38 ` sashiko-bot
2026-11-12 12:34 ` Hannes Reinecke
0 siblings, 2 replies; 4+ messages in thread
From: Kyle Mahlkuch @ 2026-11-11 17:47 UTC (permalink / raw)
To: linux-kernel, linux-scsi, Paul Ely; +Cc: msmurthy, mkp, Hannes Reinecke
This patch addresses a race condition in the lpfc driver's ELS retry
event handling that can lead to a use-after-free 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. This
creates 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.
Moved capture ndlp and clear evt_arg1 under hbalock while dequeuing
LPFC_EVT_ELS_RETRY in lpfc_work_list_done().
Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
---
Changes since v3:
- More whitespace cleanup. (Paul Ely)
Changes since v2:
- Capture ndlp and clear evt_arg1 under hbalock while dequeuing
LPFC_EVT_ELS_RETRY in lpfc_work_list_done(). A requeue by
lpfc_els_retry_delay() could cause a NULL dereference in the worker.
(AI review)
- Whitespace and line-wrapping cleanup.
Link: https://lore.kernel.org/lkml/29073ebc-b616-475b-bc05-384da09fbb54@linux.ibm.com/T/#u
Changes since v1:
- Leave test_and_clear_bit op and timer_delete_sync at the beginning of
lpfc_cancel_retry_delay_tmo() outside of lock. (Paul Ely)
Link: https://lore.kernel.org/lkml/1c8a764c-fce1-4ce1-b797-47ac328cf3f2@linux.ibm.com/T/#u
---
drivers/scsi/lpfc/lpfc_els.c | 29 +++++++++++++++++++++++------
drivers/scsi/lpfc/lpfc_hbadisc.c | 11 ++++++++++-
2 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index 6f6394a0047c..98d1824aefe1 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -4477,18 +4477,35 @@ 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))
return;
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);
+
+ /*
+ * If retry event is queued, remove it under lock and consume
+ * its payload to prevent double-put. The event holds a reference
+ * to the nodelist that must be released exactly once.
+ */
+ spin_lock_irqsave(&phba->hbalock, flags);
+ if (!list_empty(&evtp->evt_listp)) {
+ list_del_init(&evtp->evt_listp);
+ /* Take local copy and clear to prevent reuse */
+ arg_ndlp = (struct lpfc_nodelist *)evtp->evt_arg1;
+ evtp->evt_arg1 = NULL;
+ smp_wmb(); /* Ensure NULL visible before unlock */
}
+ spin_unlock_irqrestore(&phba->hbalock, flags);
+
+ /* Drop the event-held reference outside the lock */
+ 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) {
diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
index 4c673dffa671..75457f19d4ea 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -840,12 +840,21 @@ lpfc_work_list_done(struct lpfc_hba *phba)
while (!list_empty(&phba->work_list)) {
list_remove_head((&phba->work_list), evtp, typeof(*evtp),
evt_listp);
+ /*
+ * The ELS retry event can be requeued by lpfc_els_retry_delay()
+ * as soon as hbalock is dropped. Consume the payload within the
+ * hbalock to prevent reuse or double-put.
+ */
+ if (evtp->evt == LPFC_EVT_ELS_RETRY) {
+ ndlp = (struct lpfc_nodelist *)(evtp->evt_arg1);
+ evtp->evt_arg1 = NULL;
+ }
spin_unlock_irq(&phba->hbalock);
hba_pci_err = test_bit(HBA_PCI_ERR, &phba->bit_flags);
free_evt = 1;
switch (evtp->evt) {
case LPFC_EVT_ELS_RETRY:
- ndlp = (struct lpfc_nodelist *) (evtp->evt_arg1);
+ /* ndlp was already captured under hbalock */
if (!hba_pci_err) {
lpfc_els_retry_delay_handler(ndlp);
free_evt = 0; /* evt is part of ndlp */
--
2.55.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling
2026-11-11 17:47 [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling Kyle Mahlkuch
@ 2026-11-11 18:38 ` sashiko-bot
2026-11-12 12:34 ` Hannes Reinecke
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-11-11 18:38 UTC (permalink / raw)
To: Kyle Mahlkuch, linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] scsi: lpfc: missing Fixes trailer and stale race description in
changelog
--
commit c4e00459dde635d0b5ecc529a001464e7b80a848
Author: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
> scsi: lpfc: Fix race conditions in ELS retry handling
>
> This patch addresses a race condition in the lpfc driver's ELS retry
> event handling that can lead to a use-after-free 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. This
> creates 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.
>
> Moved capture ndlp and clear evt_arg1 under hbalock while dequeuing
> LPFC_EVT_ELS_RETRY in lpfc_work_list_done().
>
> Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
[Severity: Low]
This fixes the unlocked els_retry_evt removal and lpfc_nlp_put() in
lpfc_cancel_retry_delay_tmo(). Should the commit message include:
Fixes: e47c9093531d ("[SCSI] lpfc 8.2.5 : Correct ndlp referencing issues")
Also, is the first paragraph of the commit message still accurate? The code
still clears NLP_DELAY_TMO and calls timer_delete_sync() before taking
phba->hbalock, and fixes the double-put race by serializing els_retry_evt
dequeue and evt_arg1 consumption under phba->hbalock.
--
Sashiko AI review · https://sashiko.dev/#/patchset/dcea48dd-f76c-4bb0-9275-c12ef9bd78a2@linux.ibm.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling
2026-11-11 17:47 [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling Kyle Mahlkuch
2026-11-11 18:38 ` sashiko-bot
@ 2026-11-12 12:34 ` Hannes Reinecke
1 sibling, 0 replies; 4+ messages in thread
From: Hannes Reinecke @ 2026-11-12 12:34 UTC (permalink / raw)
To: Kyle Mahlkuch, linux-kernel, linux-scsi, Paul Ely; +Cc: msmurthy, mkp
On 11/11/26 6:47 PM, Kyle Mahlkuch wrote:
> This patch addresses a race condition in the lpfc driver's ELS retry
> event handling that can lead to a use-after-free 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. This
> creates 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.
>
> Moved capture ndlp and clear evt_arg1 under hbalock while dequeuing
> LPFC_EVT_ELS_RETRY in lpfc_work_list_done().
>
> Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
> ---
> Changes since v3:
> - More whitespace cleanup. (Paul Ely)
>
> Changes since v2:
> - Capture ndlp and clear evt_arg1 under hbalock while dequeuing
> LPFC_EVT_ELS_RETRY in lpfc_work_list_done(). A requeue by
> lpfc_els_retry_delay() could cause a NULL dereference in the worker.
> (AI review)
> - Whitespace and line-wrapping cleanup.
>
> Link: https://lore.kernel.org/lkml/29073ebc-b616-475b-bc05-384da09fbb54@linux.ibm.com/T/#u
>
> Changes since v1:
> - Leave test_and_clear_bit op and timer_delete_sync at the beginning of
> lpfc_cancel_retry_delay_tmo() outside of lock. (Paul Ely)
>
> Link: https://lore.kernel.org/lkml/1c8a764c-fce1-4ce1-b797-47ac328cf3f2@linux.ibm.com/T/#u
> ---
> drivers/scsi/lpfc/lpfc_els.c | 29 +++++++++++++++++++++++------
> drivers/scsi/lpfc/lpfc_hbadisc.c | 11 ++++++++++-
> 2 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index 6f6394a0047c..98d1824aefe1 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -4477,18 +4477,35 @@ 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))
> return;
> 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);
> +
> + /*
> + * If retry event is queued, remove it under lock and consume
> + * its payload to prevent double-put. The event holds a reference
> + * to the nodelist that must be released exactly once.
> + */
> + spin_lock_irqsave(&phba->hbalock, flags);
> + if (!list_empty(&evtp->evt_listp)) {
> + list_del_init(&evtp->evt_listp);
> + /* Take local copy and clear to prevent reuse */
> + arg_ndlp = (struct lpfc_nodelist *)evtp->evt_arg1;
> + evtp->evt_arg1 = NULL;
> + smp_wmb(); /* Ensure NULL visible before unlock */
> }
> + spin_unlock_irqrestore(&phba->hbalock, flags);
> +
> + /* Drop the event-held reference outside the lock */
> + 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) {
> diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
> index 4c673dffa671..75457f19d4ea 100644
> --- a/drivers/scsi/lpfc/lpfc_hbadisc.c
> +++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
> @@ -840,12 +840,21 @@ lpfc_work_list_done(struct lpfc_hba *phba)
> while (!list_empty(&phba->work_list)) {
> list_remove_head((&phba->work_list), evtp, typeof(*evtp),
> evt_listp);
> + /*
> + * The ELS retry event can be requeued by lpfc_els_retry_delay()
> + * as soon as hbalock is dropped. Consume the payload within the
> + * hbalock to prevent reuse or double-put.
> + */
> + if (evtp->evt == LPFC_EVT_ELS_RETRY) {
> + ndlp = (struct lpfc_nodelist *)(evtp->evt_arg1);
> + evtp->evt_arg1 = NULL;
> + }
> spin_unlock_irq(&phba->hbalock);
> hba_pci_err = test_bit(HBA_PCI_ERR, &phba->bit_flags);
> free_evt = 1;
> switch (evtp->evt) {
> case LPFC_EVT_ELS_RETRY:
> - ndlp = (struct lpfc_nodelist *) (evtp->evt_arg1);
> + /* ndlp was already captured under hbalock */
> if (!hba_pci_err) {
> lpfc_els_retry_delay_handler(ndlp);
> free_evt = 0; /* evt is part of ndlp */
> --
> 2.55.0
I thought I already did so, but here it is again:
Reviewed-by: Hannes Reinecke <hare@kernel.org>
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling
@ 2026-11-11 19:04 Paul Ely
0 siblings, 0 replies; 4+ messages in thread
From: Paul Ely @ 2026-11-11 19:04 UTC (permalink / raw)
To: Kyle Mahlkuch, linux-scsi, linux-kernel, mkp; +Cc: msmurthy, Hannes Reinecke
[-- Attachment #1: Type: text/plain, Size: 4720 bytes --]
From: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
To: linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org,
Paul Ely <paul.ely@broadcom.com>
Cc: msmurthy@linux.ibm.com, mkp@kernel.org, Hannes Reinecke <hare@suse.de>
Subject: [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling
Date: Wed, 11 Nov 2026 11:47:41 -0600 [thread overview]
Message-ID: <dcea48dd-f76c-4bb0-9275-c12ef9bd78a2@linux.ibm.com> (raw)
This patch addresses a race condition in the lpfc driver's ELS retry
event handling that can lead to a use-after-free 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. This
creates 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.
Moved capture ndlp and clear evt_arg1 under hbalock while dequeuing
LPFC_EVT_ELS_RETRY in lpfc_work_list_done().
Signed-off-by: Kyle Mahlkuch <kmahlkuc@linux.ibm.com>
---
Changes since v3:
- More whitespace cleanup. (Paul Ely)
Changes since v2:
- Capture ndlp and clear evt_arg1 under hbalock while dequeuing
LPFC_EVT_ELS_RETRY in lpfc_work_list_done(). A requeue by
lpfc_els_retry_delay() could cause a NULL dereference in the worker.
(AI review)
- Whitespace and line-wrapping cleanup.
Link: https://lore.kernel.org/lkml/29073ebc-b616-475b-bc05-384da09fbb54@linux.ibm.com/T/#u
Changes since v1:
- Leave test_and_clear_bit op and timer_delete_sync at the beginning of
lpfc_cancel_retry_delay_tmo() outside of lock. (Paul Ely)
Link: https://lore.kernel.org/lkml/1c8a764c-fce1-4ce1-b797-47ac328cf3f2@linux.ibm.com/T/#u
---
drivers/scsi/lpfc/lpfc_els.c | 29 +++++++++++++++++++++++------
drivers/scsi/lpfc/lpfc_hbadisc.c | 11 ++++++++++-
2 files changed, 33 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index 6f6394a0047c..98d1824aefe1 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -4477,18 +4477,35 @@ 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))
return;
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);
+
+ /*
+ * If retry event is queued, remove it under lock and consume
+ * its payload to prevent double-put. The event holds a reference
+ * to the nodelist that must be released exactly once.
+ */
+ spin_lock_irqsave(&phba->hbalock, flags);
+ if (!list_empty(&evtp->evt_listp)) {
+ list_del_init(&evtp->evt_listp);
+ /* Take local copy and clear to prevent reuse */
+ arg_ndlp = (struct lpfc_nodelist *)evtp->evt_arg1;
+ evtp->evt_arg1 = NULL;
+ smp_wmb(); /* Ensure NULL visible before unlock */
}
+ spin_unlock_irqrestore(&phba->hbalock, flags);
+
+ /* Drop the event-held reference outside the lock */
+ 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) {
diff --git a/drivers/scsi/lpfc/lpfc_hbadisc.c b/drivers/scsi/lpfc/lpfc_hbadisc.c
index 4c673dffa671..75457f19d4ea 100644
--- a/drivers/scsi/lpfc/lpfc_hbadisc.c
+++ b/drivers/scsi/lpfc/lpfc_hbadisc.c
@@ -840,12 +840,21 @@ lpfc_work_list_done(struct lpfc_hba *phba)
while (!list_empty(&phba->work_list)) {
list_remove_head((&phba->work_list), evtp, typeof(*evtp),
evt_listp);
+ /*
+ * The ELS retry event can be requeued by lpfc_els_retry_delay()
+ * as soon as hbalock is dropped. Consume the payload within the
+ * hbalock to prevent reuse or double-put.
+ */
+ if (evtp->evt == LPFC_EVT_ELS_RETRY) {
+ ndlp = (struct lpfc_nodelist *)(evtp->evt_arg1);
+ evtp->evt_arg1 = NULL;
+ }
spin_unlock_irq(&phba->hbalock);
hba_pci_err = test_bit(HBA_PCI_ERR, &phba->bit_flags);
free_evt = 1;
switch (evtp->evt) {
case LPFC_EVT_ELS_RETRY:
- ndlp = (struct lpfc_nodelist *) (evtp->evt_arg1);
+ /* ndlp was already captured under hbalock */
if (!hba_pci_err) {
lpfc_els_retry_delay_handler(ndlp);
free_evt = 0; /* evt is part of ndlp */
Thanks Kyle - looks good
Reviewed-by: Paul Ely <pely@broadcom.com>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5453 bytes --]
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-11-12 12:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-11 17:47 [PATCH v4] scsi: lpfc: Fix race conditions in ELS retry handling Kyle Mahlkuch
2026-11-11 18:38 ` sashiko-bot
2026-11-12 12:34 ` Hannes Reinecke
-- strict thread matches above, loose matches on Subject: below --
2026-11-11 19:04 Paul Ely
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox