| Message ID | 20180711134436.21963-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, 2018-07-11 at 14:44 +0100, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The check of nvmebuf suggests that it can be null, however a recent > change dereferences it to determine oxid before it is null checked, > hence there is a potential null deference on the pointer. Fix this > by performing the null check first. Also remove the oxid from the > debug log message as this is no longer valid. I considered an early > fetch of oxid if nvmebuf was valid, however, what oxid should be set > to if nvembuf is null could lead to an ambiguous logging of an > invalid > oxid, so I thought just removing it from the logging was the least > confusion solution. > > Detected by CoverityScan, CID#1471753 ("Dereference before null > check") > > Fixes: 68c9b55deea5 ("scsi: lpfc: Fix abort error path for NVMET") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/scsi/lpfc/lpfc_nvmet.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c > b/drivers/scsi/lpfc/lpfc_nvmet.c > index 22f8a204b69f..01652d9ac619 100644 > --- a/drivers/scsi/lpfc/lpfc_nvmet.c > +++ b/drivers/scsi/lpfc/lpfc_nvmet.c > @@ -1742,12 +1742,9 @@ lpfc_nvmet_unsol_ls_buffer(struct lpfc_hba > *phba, struct lpfc_sli_ring *pring, > uint32_t *payload; > uint32_t size, oxid, sid, rc; > > - fc_hdr = (struct fc_frame_header *)(nvmebuf->hbuf.virt); > - oxid = be16_to_cpu(fc_hdr->fh_ox_id); > - > if (!nvmebuf || !phba->targetport) { The !nvmebuf is a bogus check, isn't it? since nvmebuf is always obtained from a container_of, it can never be NULL. This would mean the rest of the contortions are unnecessary. James
On 07/11/2018 03:44 PM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > The check of nvmebuf suggests that it can be null, however a recent > change dereferences it to determine oxid before it is null checked, > hence there is a potential null deference on the pointer. Fix this > by performing the null check first. Also remove the oxid from the > debug log message as this is no longer valid. I considered an early > fetch of oxid if nvmebuf was valid, however, what oxid should be set > to if nvembuf is null could lead to an ambiguous logging of an invalid > oxid, so I thought just removing it from the logging was the least > confusion solution. I think that the 'if (!nvmebuf)' test is not needed and it would be better to remove the tests (it is tested for the second time later in the function). Tomas > > Detected by CoverityScan, CID#1471753 ("Dereference before null check") > > Fixes: 68c9b55deea5 ("scsi: lpfc: Fix abort error path for NVMET") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/scsi/lpfc/lpfc_nvmet.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c > index 22f8a204b69f..01652d9ac619 100644 > --- a/drivers/scsi/lpfc/lpfc_nvmet.c > +++ b/drivers/scsi/lpfc/lpfc_nvmet.c > @@ -1742,12 +1742,9 @@ lpfc_nvmet_unsol_ls_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > uint32_t *payload; > uint32_t size, oxid, sid, rc; > > - fc_hdr = (struct fc_frame_header *)(nvmebuf->hbuf.virt); > - oxid = be16_to_cpu(fc_hdr->fh_ox_id); > - > if (!nvmebuf || !phba->targetport) { > lpfc_printf_log(phba, KERN_ERR, LOG_NVME_IOERR, > - "6154 LS Drop IO x%x\n", oxid); > + "6154 LS Drop IO\n"); > oxid = 0; > size = 0; > sid = 0; > @@ -1755,6 +1752,9 @@ lpfc_nvmet_unsol_ls_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, > goto dropit; > } > > + fc_hdr = (struct fc_frame_header *)(nvmebuf->hbuf.virt); > + oxid = be16_to_cpu(fc_hdr->fh_ox_id); > + > tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private; > payload = (uint32_t *)(nvmebuf->dbuf.virt); > size = bf_get(lpfc_rcqe_length, &nvmebuf->cq_event.cqe.rcqe_cmpl);
diff --git a/drivers/scsi/lpfc/lpfc_nvmet.c b/drivers/scsi/lpfc/lpfc_nvmet.c index 22f8a204b69f..01652d9ac619 100644 --- a/drivers/scsi/lpfc/lpfc_nvmet.c +++ b/drivers/scsi/lpfc/lpfc_nvmet.c @@ -1742,12 +1742,9 @@ lpfc_nvmet_unsol_ls_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, uint32_t *payload; uint32_t size, oxid, sid, rc; - fc_hdr = (struct fc_frame_header *)(nvmebuf->hbuf.virt); - oxid = be16_to_cpu(fc_hdr->fh_ox_id); - if (!nvmebuf || !phba->targetport) { lpfc_printf_log(phba, KERN_ERR, LOG_NVME_IOERR, - "6154 LS Drop IO x%x\n", oxid); + "6154 LS Drop IO\n"); oxid = 0; size = 0; sid = 0; @@ -1755,6 +1752,9 @@ lpfc_nvmet_unsol_ls_buffer(struct lpfc_hba *phba, struct lpfc_sli_ring *pring, goto dropit; } + fc_hdr = (struct fc_frame_header *)(nvmebuf->hbuf.virt); + oxid = be16_to_cpu(fc_hdr->fh_ox_id); + tgtp = (struct lpfc_nvmet_tgtport *)phba->targetport->private; payload = (uint32_t *)(nvmebuf->dbuf.virt); size = bf_get(lpfc_rcqe_length, &nvmebuf->cq_event.cqe.rcqe_cmpl);