Re: [PATCH 10/10] qla2xxx: Fix a memory leak in an error path of qla2x00_process_els()

[Saurav Kashyap <saurav.kashyap@xxxxxxxxxx>]

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 <bvanassche@xxxxxxx>
Date: Wed, 5 Jun 2013 15:09:59 +0200
To: linux-scsi <linux-scsi@xxxxxxxxxxxxxxx>
Cc: Chad Dupuis <chad.dupuis@xxxxxxxxxx>, Saurav Kashyap
<saurav.kashyap@xxxxxxxxxx>
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 <bvanassche@xxxxxxx>
>Cc: Chad Dupuis <chad.dupuis@xxxxxxxxxx>
>Cc: Saurav Kashyap <saurav.kashyap@xxxxxxxxxx>
>---
> 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>>