[PATCH] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()

[PATCH] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()

[Wentao Liang]

lpfc_release_rpi() only drops the node reference and frees the mailbox
memory it took when lpfc_sli_issue_mbox() returns MBX_NOT_FINISHED.  The
SLI-4 issue path can also return -EIO without submitting the command, in
which case the completion handler never runs and both are leaked.

Clean up whenever the command was not handed to the port.

Fixes: 4430f7fd09ec ("scsi: lpfc: Rework locations of ndlp reference taking")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/scsi/lpfc/lpfc_nportdisc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
index 9c449055a55e..055f44ec7b94 100644
--- a/drivers/scsi/lpfc/lpfc_nportdisc.c
+++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
@@ -1142,7 +1142,13 @@ lpfc_release_rpi(struct lpfc_hba *phba, struct lpfc_vport *vport,
 				 ndlp->nlp_rpi, ndlp->nlp_DID, ndlp->nlp_flag);
 
 		rc = lpfc_sli_issue_mbox(phba, pmb, MBX_NOWAIT);
-		if (rc == MBX_NOT_FINISHED) {
+
+		/* Only a queued or completed command is handed to the
+		 * port; other results, such as the -EIO returned by the
+		 * SLI-4 path when it cannot submit a non-polled command,
+		 * leave the mailbox and the node reference to the caller.
+		 */
+		if (rc != MBX_SUCCESS && rc != MBX_BUSY) {
 			lpfc_nlp_put(ndlp);
 			mempool_free(pmb, phba->mbox_mem_pool);
 		}
-- 
2.34.1

Re: [PATCH] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()

[Paul Ely]

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

Thanks Wentao,
I need some time to read this one and test it.

Thanks

On Thu, Sep 17, 2026 at 10:48 AM Wentao Liang <vulab@iscas.ac.cn> wrote:
>
> lpfc_release_rpi() only drops the node reference and frees the mailbox
> memory it took when lpfc_sli_issue_mbox() returns MBX_NOT_FINISHED.  The
> SLI-4 issue path can also return -EIO without submitting the command, in
> which case the completion handler never runs and both are leaked.
>
> Clean up whenever the command was not handed to the port.
>
> Fixes: 4430f7fd09ec ("scsi: lpfc: Rework locations of ndlp reference taking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> ---
>  drivers/scsi/lpfc/lpfc_nportdisc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> index 9c449055a55e..055f44ec7b94 100644
> --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
> @@ -1142,7 +1142,13 @@ lpfc_release_rpi(struct lpfc_hba *phba, struct lpfc_vport *vport,
>                                  ndlp->nlp_rpi, ndlp->nlp_DID, ndlp->nlp_flag);
>
>                 rc = lpfc_sli_issue_mbox(phba, pmb, MBX_NOWAIT);
> -               if (rc == MBX_NOT_FINISHED) {
> +
> +               /* Only a queued or completed command is handed to the
> +                * port; other results, such as the -EIO returned by the
> +                * SLI-4 path when it cannot submit a non-polled command,
> +                * leave the mailbox and the node reference to the caller.
> +                */
> +               if (rc != MBX_SUCCESS && rc != MBX_BUSY) {
>                         lpfc_nlp_put(ndlp);
>                         mempool_free(pmb, phba->mbox_mem_pool);
>                 }
> --
> 2.34.1
>

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

Re: [PATCH] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()

[Paul Ely]

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

Since you are fixing up the return values, could you also fix the -EIO
in lpfc_sli_issue_mbox_s4 to return MBXERR_ERROR?  The patch will work
in case of -EIO return but clearly lpfc_release_rpi isn't looking for
a negative MBX return status.

Thanks



On Thu, Sep 17, 2026 at 3:35 PM Paul Ely <paul.ely@broadcom.com> wrote:
>
> Thanks Wentao,
> I need some time to read this one and test it.
>
> Thanks
>
> On Thu, Sep 17, 2026 at 10:48 AM Wentao Liang <vulab@iscas.ac.cn> wrote:
> >
> > lpfc_release_rpi() only drops the node reference and frees the mailbox
> > memory it took when lpfc_sli_issue_mbox() returns MBX_NOT_FINISHED.  The
> > SLI-4 issue path can also return -EIO without submitting the command, in
> > which case the completion handler never runs and both are leaked.
> >
> > Clean up whenever the command was not handed to the port.
> >
> > Fixes: 4430f7fd09ec ("scsi: lpfc: Rework locations of ndlp reference taking")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
> > ---
> >  drivers/scsi/lpfc/lpfc_nportdisc.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
> > index 9c449055a55e..055f44ec7b94 100644
> > --- a/drivers/scsi/lpfc/lpfc_nportdisc.c
> > +++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
> > @@ -1142,7 +1142,13 @@ lpfc_release_rpi(struct lpfc_hba *phba, struct lpfc_vport *vport,
> >                                  ndlp->nlp_rpi, ndlp->nlp_DID, ndlp->nlp_flag);
> >
> >                 rc = lpfc_sli_issue_mbox(phba, pmb, MBX_NOWAIT);
> > -               if (rc == MBX_NOT_FINISHED) {
> > +
> > +               /* Only a queued or completed command is handed to the
> > +                * port; other results, such as the -EIO returned by the
> > +                * SLI-4 path when it cannot submit a non-polled command,
> > +                * leave the mailbox and the node reference to the caller.
> > +                */
> > +               if (rc != MBX_SUCCESS && rc != MBX_BUSY) {
> >                         lpfc_nlp_put(ndlp);
> >                         mempool_free(pmb, phba->mbox_mem_pool);
> >                 }
> > --
> > 2.34.1
> >

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

Re: [PATCH] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()

[Wentao Liang]

Hi Paul,

Sorry for answering so late.  You asked me about the -EIO return a month
ago and I am only getting back to it now; that delay is on me.

You are right about it.  lpfc_release_rpi() cannot tell a negative errno
from a mailbox status, and the value was outside the MBX_* code space the
rest of the function returns.  lpfc_sli_issue_mbox_s4() now returns
MBXERR_ERROR for the non-polled command it cannot issue while interrupts
are disabled, which is also what lpfc_sli4_post_sync_mbox() returns when
it cannot post a command.

While going through the callers for that change I found one that does
depend on the sign: lpfc_bsg_issue_mbox() passes the raw result to bsg,
and bsg-lib only reports a negative result as an error, so a positive MBX
status reaches the ioctl caller as success.  That is already true for
MBX_NOT_FINISHED today; 2/3 fixes it so that 3/3 does not add another way
to hit it.

The series is:

  [PATCH v2 1/3] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()
  [PATCH v2 2/3] scsi: lpfc: Report bsg mailbox failures as an errno
  [PATCH v2 3/3] scsi: lpfc: Return MBX status when a non-polled mbox cannot be issued

1/3 is the patch you were testing.  The check itself is unchanged; it is
still `rc != MBX_SUCCESS && rc != MBX_BUSY`, the test
lpfc_config_port_post() and lpfc_issue_hb_mbox() already use for the same
purpose, and it covers both -EIO and MBXERR_ERROR.  I only rewrote the
comment above it.  3/3 is the change you asked for.  I kept 1/3 apart
because it is the leak fix I would like in stable; 2/3 and 3/3 are not
stable candidates.

One thing I saw and did not touch: NLP_UNREG_INP is set before the issue
and is not cleared when the command is not handed to the port, so a later
lpfc_release_rpi() for the same node returns early at the "1435
release_rpi SKIP UNREG" message.  That is pre-existing on the
MBX_NOT_FINISHED path too.  Clearing it would make a second unreg possible
for the same NPort, so I would rather ask than guess - tell me if you want
it in this series.

Testing: I have no lpfc adapter, so this is build tested only.  Mainline
at d48723711bae, drivers/scsi/lpfc/ builds with W=1 and no warnings.
checkpatch.pl --strict reports nothing on the three patches except
"unknown commit id" on the Fixes tag, which is an artifact of my shallow
checkout - 4430f7fd09ec is in mainline.  I could not run it on hardware,
and the changelogs say so.

Thanks,
Wentao

[PATCH v2 1/3] scsi: lpfc: Fix ndlp reference leak in lpfc_release_rpi()

[Wentao Liang]

lpfc_release_rpi() only drops the node reference and frees the mailbox
memory it took when lpfc_sli_issue_mbox() returns MBX_NOT_FINISHED.  The
SLI-4 issue path can also return -EIO for a command it refuses to
submit, in which case the completion handler never runs and both the
reference and the mailbox are leaked.

Clean up whenever the command was not handed to the port.

The candidate came from a static analysis tool we run over the tree.
DeepSeek and GPT-6.1 Sol were used to audit the finding and to help
write the fix; I reviewed the result line by line and I am responsible
for it.

Fixes: 4430f7fd09ec ("scsi: lpfc: Rework locations of ndlp reference taking")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
Changes in v2:
 - Rebased on v7.4-rc1.
 - Reworded the comment above the check so that it does not depend on the
   SLI-4 return value.  The check itself is unchanged.
 - Sorry for the month it took me to answer the review.

 drivers/scsi/lpfc/lpfc_nportdisc.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/lpfc/lpfc_nportdisc.c b/drivers/scsi/lpfc/lpfc_nportdisc.c
index fb8844c69491..fbe3c97723b2 100644
--- a/drivers/scsi/lpfc/lpfc_nportdisc.c
+++ b/drivers/scsi/lpfc/lpfc_nportdisc.c
@@ -1155,7 +1155,11 @@ lpfc_release_rpi(struct lpfc_hba *phba, struct lpfc_vport *vport,
 				 ndlp->nlp_rpi, ndlp->nlp_DID, ndlp->nlp_flag);
 
 		rc = lpfc_sli_issue_mbox(phba, pmb, MBX_NOWAIT);
-		if (rc == MBX_NOT_FINISHED) {
+		/* The completion handler frees the mailbox and drops the node
+		 * reference, and it only runs when the command was queued or
+		 * already completed.
+		 */
+		if (rc != MBX_SUCCESS && rc != MBX_BUSY) {
 			lpfc_nlp_put(ndlp);
 			mempool_free(pmb, phba->mbox_mem_pool);
 		}

[PATCH v2 2/3] scsi: lpfc: Report bsg mailbox failures as an errno

[Wentao Liang]

lpfc_bsg_issue_mbox() hands a command to the port with MBX_NOWAIT and, if
that does not start the command, returns the raw result to
lpfc_bsg_mbox_cmd(), which stores it in the bsg reply.  bsg-lib only
reports a negative result as an error to the ioctl caller, so a positive
MBX status such as MBX_NOT_FINISHED is passed off as success, with
device_status holding the low byte of the status and no reply data.

Report -EIO instead, as the polled path in the same function already maps
its failures to -ETIME and -ENODEV.

Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
New in v2.

 drivers/scsi/lpfc/lpfc_bsg.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c
index 7354ae9ba8e5..00c54a8818ed 100644
--- a/drivers/scsi/lpfc/lpfc_bsg.c
+++ b/drivers/scsi/lpfc/lpfc_bsg.c
@@ -5106,6 +5106,11 @@ lpfc_bsg_issue_mbox(struct lpfc_hba *phba, struct bsg_job *job,
 	if ((rc == MBX_SUCCESS) || (rc == MBX_BUSY))
 		return 1; /* job started */
 
+	/* bsg-lib only reports a negative result as an error, so a status
+	 * from the MBX code space must not be passed back to it.
+	 */
+	rc = -EIO;
+
 job_done:
 	/* common exit for error or job completed inline */
 	if (pmboxq)

[PATCH v2 3/3] scsi: lpfc: Return MBX status when a non-polled mbox cannot be issued

[Wentao Liang]

lpfc_sli_issue_mbox_s4() reports a failure to hand a command to the port
with an MBX status code in every path except one: when interrupts are not
enabled and the caller asked for MBX_NOWAIT, it returns -EIO.  A negative
errno does not belong in that code space, and a caller has no MBX value to
test for this outcome.

Return MBXERR_ERROR, as lpfc_sli4_post_sync_mbox() already does for a
command it cannot post.  lpfc_issue_lip() maps MBXERR_ERROR to -EIO, and
the bsg mailbox path now converts a failure of its own, so no positive
status is handed to a caller that expects an errno.

Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
New in v2, at Paul's request.

 drivers/scsi/lpfc/lpfc_sli.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 715b328d3d18..05a76b00702a 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -10121,7 +10121,7 @@ lpfc_sli_issue_mbox_s4(struct lpfc_hba *phba, LPFC_MBOXQ_t *mboxq,
 		if (flag == MBX_POLL)
 			rc = lpfc_sli4_post_sync_mbox(phba, mboxq);
 		else
-			rc = -EIO;
+			rc = MBXERR_ERROR;
 		if (rc != MBX_SUCCESS)
 			lpfc_printf_log(phba, KERN_WARNING, LOG_MBOX | LOG_SLI,
 					"(%d):2541 Mailbox command x%x "