Commit 97f97582 authored by Justin Tee's avatar Justin Tee Committed by Martin K. Petersen
Browse files

scsi: lpfc: Fix double free in lpfc_cmpl_els_logo_acc() caused by lpfc_nlp_not_used() 



Smatch detected a double free path because lpfc_nlp_not_used() releases an
ndlp object before reaching lpfc_nlp_put() at the end of
lpfc_cmpl_els_logo_acc().

Remove the outdated lpfc_nlp_not_used() routine.  In
lpfc_mbx_cmpl_ns_reg_login(), replace the call with lpfc_nlp_put().  In
lpfc_cmpl_els_logo_acc(), replace the call with lpfc_unreg_rpi() and keep
the lpfc_nlp_put() at the end of the routine.  If ndlp's rpi was
registered, then lpfc_unreg_rpi()'s completion routine performs the final
ndlp clean up after lpfc_nlp_put() is called from lpfc_cmpl_els_logo_acc().
Otherwise if ndlp has no rpi registered, the lpfc_nlp_put() at the end of
lpfc_cmpl_els_logo_acc() is the final ndlp clean up.

Fixes: 4430f7fd ("scsi: lpfc: Rework locations of ndlp reference taking")
Cc: <stable@vger.kernel.org> # v5.11+
Reported-by: default avatarDan Carpenter <error27@gmail.com>
Link: https://lore.kernel.org/all/Y3OefhyyJNKH%2Fiaf@kili/


Signed-off-by: default avatarJustin Tee <justin.tee@broadcom.com>
Link: https://lore.kernel.org/r/20230417191558.83100-3-justintee8345@gmail.com


Signed-off-by: default avatarMartin K. Petersen <martin.petersen@oracle.com>
parent 84c868a7

drivers/scsi/lpfc/lpfc_crtn.h

+0 −1
Original line number Diff line number Diff line
@@ -134,7 +134,6 @@ void lpfc_check_nlp_post_devloss(struct lpfc_vport *vport, 
				 struct lpfc_nodelist *ndlp);

void lpfc_ignore_els_cmpl(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,

			  struct lpfc_iocbq *rspiocb);

int  lpfc_nlp_not_used(struct lpfc_nodelist *ndlp);

struct lpfc_nodelist *lpfc_setup_disc_node(struct lpfc_vport *, uint32_t);

void lpfc_disc_list_loopmap(struct lpfc_vport *);

void lpfc_disc_start(struct lpfc_vport *);

drivers/scsi/lpfc/lpfc_els.c

+7 −23
Original line number Diff line number Diff line
@@ -5205,14 +5205,9 @@ lpfc_els_free_iocb(struct lpfc_hba *phba, struct lpfc_iocbq *elsiocb) 
 *

 * This routine is the completion callback function to the Logout (LOGO)

 * Accept (ACC) Response ELS command. This routine is invoked to indicate

 * the completion of the LOGO process. It invokes the lpfc_nlp_not_used() to

 * release the ndlp if it has the last reference remaining (reference count

 * is 1). If succeeded (meaning ndlp released), it sets the iocb ndlp

 * field to NULL to inform the following lpfc_els_free_iocb() routine no

 * ndlp reference count needs to be decremented. Otherwise, the ndlp

 * reference use-count shall be decremented by the lpfc_els_free_iocb()

 * routine. Finally, the lpfc_els_free_iocb() is invoked to release the

 * IOCB data structure.

 * the completion of the LOGO process. If the node has transitioned to NPR,

 * this routine unregisters the RPI if it is still registered. The

 * lpfc_els_free_iocb() is invoked to release the IOCB data structure.

 **/

static void

lpfc_cmpl_els_logo_acc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
@@ -5253,19 +5248,9 @@ lpfc_cmpl_els_logo_acc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb, 
		    (ndlp->nlp_last_elscmd == ELS_CMD_PLOGI))

			goto out;



		/* NPort Recovery mode or node is just allocated */

		if (!lpfc_nlp_not_used(ndlp)) {

			/* A LOGO is completing and the node is in NPR state.

			 * Just unregister the RPI because the node is still

			 * required.

			 */

		if (ndlp->nlp_flag & NLP_RPI_REGISTERED)

			lpfc_unreg_rpi(vport, ndlp);

		} else {

			/* Indicate the node has already released, should

			 * not reference to it from within lpfc_els_free_iocb.

			 */

			cmdiocb->ndlp = NULL;

		}



	}

 out:

	/*
@@ -5285,9 +5270,8 @@ lpfc_cmpl_els_logo_acc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb, 
 * RPI (Remote Port Index) mailbox command to the @phba. It simply releases

 * the associated lpfc Direct Memory Access (DMA) buffer back to the pool and

 * decrements the ndlp reference count held for this completion callback

 * function. After that, it invokes the lpfc_nlp_not_used() to check

 * whether there is only one reference left on the ndlp. If so, it will

 * perform one more decrement and trigger the release of the ndlp.

 * function. After that, it invokes the lpfc_drop_node to check

 * whether it is appropriate to release the node.

 **/

void

lpfc_mbx_cmpl_dflt_rpi(struct lpfc_hba *phba, LPFC_MBOXQ_t *pmb)

drivers/scsi/lpfc/lpfc_hbadisc.c

+3 −21
Original line number Diff line number Diff line
@@ -4333,13 +4333,14 @@ lpfc_mbx_cmpl_ns_reg_login(struct lpfc_hba *phba, LPFC_MBOXQ_t *pmb) 


		/* If the node is not registered with the scsi or nvme

		 * transport, remove the fabric node.  The failed reg_login

		 * is terminal.

		 * is terminal and forces the removal of the last node

		 * reference.

		 */

		if (!(ndlp->fc4_xpt_flags & (SCSI_XPT_REGD | NVME_XPT_REGD))) {

			spin_lock_irq(&ndlp->lock);

			ndlp->nlp_flag &= ~NLP_NPR_2B_DISC;

			spin_unlock_irq(&ndlp->lock);

			lpfc_nlp_not_used(ndlp);

			lpfc_nlp_put(ndlp);

		}



		if (phba->fc_topology == LPFC_TOPOLOGY_LOOP) {
@@ -6704,25 +6705,6 @@ lpfc_nlp_put(struct lpfc_nodelist *ndlp) 
	return ndlp ? kref_put(&ndlp->kref, lpfc_nlp_release) : 0;

}



/* This routine free's the specified nodelist if it is not in use

 * by any other discovery thread. This routine returns 1 if the

 * ndlp has been freed. A return value of 0 indicates the ndlp is

 * not yet been released.

 */

int

lpfc_nlp_not_used(struct lpfc_nodelist *ndlp)

{

	lpfc_debugfs_disc_trc(ndlp->vport, LPFC_DISC_TRC_NODE,

		"node not used:   did:x%x flg:x%x refcnt:x%x",

		ndlp->nlp_DID, ndlp->nlp_flag,

		kref_read(&ndlp->kref));



	if (kref_read(&ndlp->kref) == 1)

		if (lpfc_nlp_put(ndlp))

			return 1;

	return 0;

}



/**

 * lpfc_fcf_inuse - Check if FCF can be unregistered.

 * @phba: Pointer to hba context object.