Skip to contentSkip to conversation

qla2x00t: Fix a memory leak in an error path

PATCH In review Bart Van Assche 2 participants 3 messages May 11, 2013 – May 17, 2013

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 qla2x00t: Fix a memory leak in an error path

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. Detected by Coverity.

Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>
---
 drivers/scsi/qla2xxx/qla_bsg.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_bsg.c +1 -1
index 39719f8..af35707 100644
@@ -329,7 +329,7 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
 	if (!vha->flags.online) {
 		ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
 		rval = -EIO;
-		goto done;
+		goto done_free_fcport;
 	}
 
 	req_sg_cnt =
Signature · 1 line
1.7.10.4
#1 lore raw

Hi Bart,

Instead of doing this I would move this check to the top of the function, something like this
-----------------8<----------

(quoted diff) +0 -0

b/drivers/scsi/qla2xxx/qla_bsg.c
index 371bb86..b5f84ae 100644

drivers/scsi/qla2xxx/qla_bsg.c +6 -0
@@ -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);

----------------8<---------

Thanks,
~Saurav

Quoted 33 lines

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. Detected by Coverity.

Cc Chad Dupuis <[email protected]>
Cc Saurav Kashyap <[email protected]>

---
drivers/scsi/qla2xxx/qla_bsg.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/qla2xxx/qla_bsg.c +1 -1
index 39719f8..af35707 100644
@@ -329,7 +329,7 @@ qla2x00_process_els(struct fc_bsg_job *bsg_job)
 	if (!vha->flags.online) {
 		ql_log(ql_log_warn, vha, 0x7005, "Host not online.\n");
 		rval = -EIO;
-		goto done;
+		goto done_free_fcport;
 	}
 
 	req_sg_cnt =
Signature · 6 lines
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw
Quoted 36 lines · Saurav Kashyap

Instead of doing this I would move this check to the top of the function, something like this
-----------------8<----------

drivers/scsi/qla2xxx/qla_bsg.c +6 -0
index 371bb86..b5f84ae 100644
@@ -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;
drivers/scsi/qla2xxx/qla_bsg.c +0 -6
@@ -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,

Quoted 3 lines
                 bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);

----------------8<---------

Hello Saurav,

I will update the patch, retest and repost it. Thanks for the feedback.

Bart.