Hi Bart, In this case online check is move to far, the vha is still not dereferenced. The right patch is moving online flag just after getting vha.
diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
b/drivers/scsi/qla2xxx/qla_bsg.c
index 9520b1f..11f84dc 100644
--- a/drivers/scsi/qla2xxx/qla_bsg.c
+++ b/drivers/scsi/qla2xxx/qla_bsg.c
@@ -269,6 +269,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
type = "FC_BSG_HST_ELS_NOLOGIN";
}
+ if (!vha->flags.online) {
+ ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
+ rval = -EIO;
+ goto done;
+ }
+
/* pass through is supported only for ISP 4Gb or higher */
if (!IS_FWI2_CAPABLE(ha)) {
ql_dbg(ql_dbg_user, vha, 0x7001,
@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
NPH_FABRIC_CONTROLLER : NPH_F_PORT;
}
- if (!vha->flags.online) {
- ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
- rval = -EIO;
- goto done;
- }
-
req_sg_cnt =
dma_map_sg(&ha->pdev->dev,
bsg_job->request_payload.sg_list,
bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
@@ -399,7 +399,7 @@ done_unmap_sg:
goto done_free_fcport;
done_free_fcport:
- if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
+ if (bsg_job->request->msgcode == FC_BSG_RPT_ELS)
kfree(fcport);
done:
return rval;
Thanks,
~Saurav
-----Original Message-----
From: Bart Van Assche <[email protected]>
Date: Wed, 5 Jun 2013 15:09:59 +0200
To: linux-scsi <[email protected]>
Cc: Chad Dupuis <[email protected]>, Saurav Kashyap
<[email protected]>
Subject: [PATCH 10/10] qla2xxx: Fix a memory leak in an error path of
qla2x00_process_els()
>Avoid that the fcport structure gets leaked if
>bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN, the fcport
>allocation succeeds and the !vha->flags.online branch is taken.
>This was detected by Coverity. However, Coverity does not recognize
>that all qla2x00_process_els() callers specify either
>FC_BSG_RPT_ELS or FC_BSG_HST_ELS_NOLOGIN in the field
>bsg_job->request->msgcode and that the value of that field is not
>modified inside that function. This results in a false positive
>report about a possible memory leak in an error path for
>bsg_job->request->msgcode values other than the two mentioned
>values. Make it easy for Coverity (and for humans) to recognize
>that there is no fcport leak in the error path by changing the
>bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN test into
>bsg_job->request->msgcode != FC_BSG_RPT_ELS.
>
>Signed-off-by: Bart Van Assche <[email protected]>
>Cc: Chad Dupuis <[email protected]>
>Cc: Saurav Kashyap <[email protected]>
>---
> drivers/scsi/qla2xxx/qla_bsg.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_bsg.c
>b/drivers/scsi/qla2xxx/qla_bsg.c
>index cf07491..f8a2634 100644
>--- a/drivers/scsi/qla2xxx/qla_bsg.c
>+++ b/drivers/scsi/qla2xxx/qla_bsg.c
>@@ -255,6 +255,12 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
> int rval = (DRIVER_ERROR << 16);
> uint16_t nextlid = 0;
>
>+ if (!vha->flags.online) {
>+ ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
>+ rval = -EIO;
>+ goto done;
>+ }
>+
> if (bsg_job->request->msgcode == FC_BSG_RPT_ELS) {
> rport = bsg_job->rport;
> fcport = *(fc_port_t **) rport->dd_data;
>@@ -326,12 +332,6 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
> NPH_FABRIC_CONTROLLER : NPH_F_PORT;
> }
>
>- if (!vha->flags.online) {
>- ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
>- rval = -EIO;
>- goto done;
>- }
>-
> req_sg_cnt =
> dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
> bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
>@@ -399,7 +399,7 @@ done_unmap_sg:
> goto done_free_fcport;
>
> done_free_fcport:
>- if (bsg_job->request->msgcode == FC_BSG_HST_ELS_NOLOGIN)
>+ if (bsg_job->request->msgcode != FC_BSG_RPT_ELS)
> kfree(fcport);
> done:
> return rval;
>--
>1.7.10.4
>
<<attachment: winmail.dat>>
