* [PATCH 1/3] lpfc: Fix pointer checks and comments in LS receive refactoring
2020-05-20 18:59 [PATCH 0/3] lpfc: Fix errors in LS receive refactoring James Smart
@ 2020-05-20 18:59 ` James Smart
2020-05-20 18:59 ` [PATCH 2/3] lpfc: fix axchg pointer reference after free and double frees James Smart
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: James Smart @ 2020-05-20 18:59 UTC (permalink / raw)
To: linux-nvme
Cc: axboe, James Smart, martin.petersen, linux-scsi, jejb,
kernel-janitors, hch, paul.ely, hare, dan.carpenter
Additional testing encountered null pointers that weren't fully qualified
in lpfc_nvmet_xmt_ls_abort_cmp() and lpfc_nvmet_unsol_issue_abort().
The same error was detected and reported by static checker reporting:
drivers/scsi/lpfc/lpfc_sli.c:2905 lpfc_nvme_unsol_ls_handler()
error: we previously assumed 'phba->targetport' could be null
(see line 2837)
Fix by making phba->nvmet_support and phba->targetport validity checks
in lpfc_nvmet_xmt_ls_abort_cmp() and lpfc_nvmet_unsol_issue_abort().
Fixes: 3a8070c567aaa (“lpfc: Refactor NVME LS receive handling”)
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Paul Ely <paul.ely@broadcom.com>
Signed-off-by: James Smart <jsmart2021@gmail.com>
---
drivers/scsi/lpfc/lpfc_nvmet.c | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index 1c6bbbba70b5..bccf9da302ee 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -3207,8 +3207,10 @@ lpfc_nvmet_xmt_ls_abort_cmp(struct lpfc_hba *phba, struct lpfc_iocbq *cmdwqe,
ctxp = cmdwqe->context2;
result = wcqe->parameter;
- tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
- atomic_inc(&tgtp->xmt_ls_abort_cmpl);
+ if (phba->nvmet_support) {
+ tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
+ atomic_inc(&tgtp->xmt_ls_abort_cmpl);
+ }
lpfc_printf_log(phba, KERN_INFO, LOG_NVME_ABTS,
"6083 Abort cmpl: ctx x%px WCQE:%08x %08x %08x %08x\n",
@@ -3244,7 +3246,7 @@ lpfc_nvmet_unsol_issue_abort(struct lpfc_hba *phba,
struct lpfc_async_xchg_ctx *ctxp,
uint32_t sid, uint16_t xri)
{
- struct lpfc_nvmet_tgtport *tgtp;
+ struct lpfc_nvmet_tgtport *tgtp = NULL;
struct lpfc_iocbq *abts_wqeq;
union lpfc_wqe128 *wqe_abts;
struct lpfc_nodelist *ndlp;
@@ -3253,13 +3255,15 @@ lpfc_nvmet_unsol_issue_abort(struct lpfc_hba *phba,
"6067 ABTS: sid %x xri x%x/x%x\n",
sid, xri, ctxp->wqeq->sli4_xritag);
- tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
+ if (phba->nvmet_support && phba->targetport)
+ tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
ndlp = lpfc_findnode_did(phba->pport, sid);
if (!ndlp || !NLP_CHK_NODE_ACT(ndlp) ||
((ndlp->nlp_state != NLP_STE_UNMAPPED_NODE) &&
(ndlp->nlp_state != NLP_STE_MAPPED_NODE))) {
- atomic_inc(&tgtp->xmt_abort_rsp_error);
+ if (tgtp)
+ atomic_inc(&tgtp->xmt_abort_rsp_error);
lpfc_printf_log(phba, KERN_ERR, LOG_NVME_ABTS,
"6134 Drop ABTS - wrong NDLP state x%x.\n",
(ndlp) ? ndlp->nlp_state : NLP_STE_MAX_STATE);
@@ -3538,7 +3542,7 @@ lpfc_nvme_unsol_ls_issue_abort(struct lpfc_hba *phba,
struct lpfc_async_xchg_ctx *ctxp,
uint32_t sid, uint16_t xri)
{
- struct lpfc_nvmet_tgtport *tgtp;
+ struct lpfc_nvmet_tgtport *tgtp = NULL;
struct lpfc_iocbq *abts_wqeq;
unsigned long flags;
int rc;
@@ -3555,7 +3559,9 @@ lpfc_nvme_unsol_ls_issue_abort(struct lpfc_hba *phba,
ctxp->state = LPFC_NVME_STE_LS_ABORT;
}
- tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
+ if (phba->nvmet_support && phba->targetport)
+ tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private;
+
if (!ctxp->wqeq) {
/* Issue ABTS for this WQE based on iotag */
ctxp->wqeq = lpfc_sli_get_iocbq(phba);
@@ -3582,11 +3588,13 @@ lpfc_nvme_unsol_ls_issue_abort(struct lpfc_hba *phba,
rc = lpfc_sli4_issue_wqe(phba, ctxp->hdwq, abts_wqeq);
spin_unlock_irqrestore(&phba->hbalock, flags);
if (rc == WQE_SUCCESS) {
- atomic_inc(&tgtp->xmt_abort_unsol);
+ if (tgtp)
+ atomic_inc(&tgtp->xmt_abort_unsol);
return 0;
}
out:
- atomic_inc(&tgtp->xmt_abort_rsp_error);
+ if (tgtp)
+ atomic_inc(&tgtp->xmt_abort_rsp_error);
abts_wqeq->context2 = NULL;
abts_wqeq->context3 = NULL;
lpfc_sli_release_iocbq(phba, abts_wqeq);
--
2.26.1
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] lpfc: fix axchg pointer reference after free and double frees
2020-05-20 18:59 [PATCH 0/3] lpfc: Fix errors in LS receive refactoring James Smart
2020-05-20 18:59 ` [PATCH 1/3] lpfc: Fix pointer checks and comments " James Smart
@ 2020-05-20 18:59 ` James Smart
2020-05-20 18:59 ` [PATCH 3/3] lpfc: Fix return value in __lpfc_nvme_ls_abort James Smart
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: James Smart @ 2020-05-20 18:59 UTC (permalink / raw)
To: linux-nvme
Cc: axboe, James Smart, Dick Kennedy, martin.petersen, linux-scsi,
jejb, kernel-janitors, hch, paul.ely, hare, dan.carpenter
The axchg structure is a structure allocated early in the
lpfc_nvme_unsol_ls_handler() to represent the newly received exchange.
Upon error, the out_fail path in the routine unconditionally frees the
pointer, yet subsequently passes the pointer to the abort routine.
Additionally, the abort routine, lpfc_nvme_unsol_ls_issue_abort(), also
has a failure path that will attempt to delete the pointer on error.
Fix these errors by:
- Removing the unconditional free so that it stays valid if passed
to the abort routine.
- Revise the abort routine to not free the pointer. Instead, return
a success/failure status. Note: if success, the later completion of
the abort frees the structure.
- Back in the unsol_ls_handler() error path, if the abort routine was
skipped (thus no possible reference) or the abort routine returned
error, free the pointer.
Fixes: 3a8070c567aa ("lpfc: Refactor NVME LS receive handling")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
Signed-off-by: James Smart <jsmart2021@gmail.com>
---
drivers/scsi/lpfc/lpfc_nvmet.c | 3 +--
drivers/scsi/lpfc/lpfc_sli.c | 10 ++++++----
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c
index bccf9da302ee..32eb5e873e9b 100644
--- a/drivers/scsi/lpfc/lpfc_nvmet.c
+++ b/drivers/scsi/lpfc/lpfc_nvmet.c
@@ -3598,10 +3598,9 @@ lpfc_nvme_unsol_ls_issue_abort(struct lpfc_hba *phba,
abts_wqeq->context2 = NULL;
abts_wqeq->context3 = NULL;
lpfc_sli_release_iocbq(phba, abts_wqeq);
- kfree(ctxp);
lpfc_printf_log(phba, KERN_ERR, LOG_NVME_ABTS,
"6056 Failed to Issue ABTS. Status x%x\n", rc);
- return 0;
+ return 1;
}
/**
diff --git a/drivers/scsi/lpfc/lpfc_sli.c b/drivers/scsi/lpfc/lpfc_sli.c
index 1aaf40081e21..9e21c4f3b009 100644
--- a/drivers/scsi/lpfc/lpfc_sli.c
+++ b/drivers/scsi/lpfc/lpfc_sli.c
@@ -2813,7 +2813,7 @@ lpfc_nvme_unsol_ls_handler(struct lpfc_hba *phba, struct lpfc_iocbq *piocb)
struct lpfc_async_xchg_ctx *axchg = NULL;
char *failwhy = NULL;
uint32_t oxid, sid, did, fctl, size;
- int ret;
+ int ret = 1;
d_buf = piocb->context2;
@@ -2897,14 +2897,16 @@ lpfc_nvme_unsol_ls_handler(struct lpfc_hba *phba, struct lpfc_iocbq *piocb)
(phba->nvmet_support) ? "T" : "I", ret);
out_fail:
- kfree(axchg);
/* recycle receive buffer */
lpfc_in_buf_free(phba, &nvmebuf->dbuf);
/* If start of new exchange, abort it */
- if (fctl & FC_FC_FIRST_SEQ && !(fctl & FC_FC_EX_CTX))
- lpfc_nvme_unsol_ls_issue_abort(phba, axchg, sid, oxid);
+ if (axchg && (fctl & FC_FC_FIRST_SEQ && !(fctl & FC_FC_EX_CTX)))
+ ret = lpfc_nvme_unsol_ls_issue_abort(phba, axchg, sid, oxid);
+
+ if (ret)
+ kfree(axchg);
}
/**
--
2.26.1
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] lpfc: Fix return value in __lpfc_nvme_ls_abort
2020-05-20 18:59 [PATCH 0/3] lpfc: Fix errors in LS receive refactoring James Smart
2020-05-20 18:59 ` [PATCH 1/3] lpfc: Fix pointer checks and comments " James Smart
2020-05-20 18:59 ` [PATCH 2/3] lpfc: fix axchg pointer reference after free and double frees James Smart
@ 2020-05-20 18:59 ` James Smart
2020-05-22 7:29 ` [PATCH 0/3] lpfc: Fix errors in LS receive refactoring Dan Carpenter
2020-05-26 14:53 ` Christoph Hellwig
4 siblings, 0 replies; 6+ messages in thread
From: James Smart @ 2020-05-20 18:59 UTC (permalink / raw)
To: linux-nvme
Cc: axboe, James Smart, Dick Kennedy, martin.petersen, linux-scsi,
jejb, kernel-janitors, hch, paul.ely, hare, dan.carpenter
A static checker reported the following issue:
drivers/scsi/lpfc/lpfc_nvmet.c:1366 lpfc_nvmet_ls_abort()
warn: 'ret' can be either negative or positive
The comment indicates a non-zero value indicates error in the
form of -Exxx, but the code is returning "1".
Fix the code to return -EINVAL to be compliant to comment.
Fixes: e96a22b0b7c2 ("lpfc: Refactor Send LS Abort support")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
Signed-off-by: James Smart <jsmart2021@gmail.com>
---
drivers/scsi/lpfc/lpfc_nvme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/lpfc/lpfc_nvme.c b/drivers/scsi/lpfc/lpfc_nvme.c
index 21bbccf0dc31..b46ba70f78da 100644
--- a/drivers/scsi/lpfc/lpfc_nvme.c
+++ b/drivers/scsi/lpfc/lpfc_nvme.c
@@ -895,7 +895,7 @@ __lpfc_nvme_ls_abort(struct lpfc_vport *vport, struct lpfc_nodelist *ndlp,
lpfc_printf_vlog(vport, KERN_INFO, LOG_NVME_DISC | LOG_NVME_ABTS,
"6213 NVMEx LS REQ Abort: Unable to locate req x%p\n",
pnvme_lsreq);
- return 1;
+ return -EINVAL;
}
static int
--
2.26.1
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] lpfc: Fix errors in LS receive refactoring
2020-05-20 18:59 [PATCH 0/3] lpfc: Fix errors in LS receive refactoring James Smart
` (2 preceding siblings ...)
2020-05-20 18:59 ` [PATCH 3/3] lpfc: Fix return value in __lpfc_nvme_ls_abort James Smart
@ 2020-05-22 7:29 ` Dan Carpenter
2020-05-26 14:53 ` Christoph Hellwig
4 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2020-05-22 7:29 UTC (permalink / raw)
To: James Smart
Cc: axboe, martin.petersen, linux-scsi, jejb, kernel-janitors,
linux-nvme, hch, paul.ely, hare
On Wed, May 20, 2020 at 11:59:26AM -0700, James Smart wrote:
> A prior patch set refactored lpfc to create common routines for NVME LS
> handling for use by both the initiator and target paths. The refactoring
> introduced several errors spotted by additional testing and static checker
> reporting.
>
> This patch set corrects those errors.
>
> The patches should enter via the nvme tree, as the lpfc modifications were
> in support of nvme-fc transport api deltas merged via the nvme tree.
>
> -- james
Thanks!
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
regards,
dan carpenter
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] lpfc: Fix errors in LS receive refactoring
2020-05-20 18:59 [PATCH 0/3] lpfc: Fix errors in LS receive refactoring James Smart
` (3 preceding siblings ...)
2020-05-22 7:29 ` [PATCH 0/3] lpfc: Fix errors in LS receive refactoring Dan Carpenter
@ 2020-05-26 14:53 ` Christoph Hellwig
4 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2020-05-26 14:53 UTC (permalink / raw)
To: James Smart
Cc: axboe, linux-scsi, martin.petersen, jejb, kernel-janitors,
linux-nvme, hch, paul.ely, hare, dan.carpenter
Thanks,
applied to nvme-5.8.
_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 6+ messages in thread