scsi_dh_alua: uninitialized variable in alua_rtpg()

PATCH Superseded Dan Carpenter 4 participants 7 messages Apr 14, 2016 – Apr 15, 2016

Subsystems (2)

SCSI Kernel janitors

lore

Share & tools

lore

Dan Carpenter

author

2016-04-14 09:39 UTC #0 lore raw

PATCH 1/1 scsi_dh_alua: uninitialized variable in alua_rtpg()

It's possible to use "err" without initializing it. If it happens to be a 2 which is SCSI_DH_RETRY then that could cause a bug.

Signed-off-by Dan Carpenter <[email protected]>

drivers/scsi/device_handler/scsi_dh_alua.c +2 -1

index 8eaed05..f3c994f 100644
@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	struct alua_port_group *tmp_pg;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
 	unsigned char *desc, *buff;
-	unsigned err, retval;
+	unsigned int err = 0;
+	unsigned int retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 	unsigned long flags;

Bart Van Assche

2016-04-14 15:45 UTC #1 lore raw

Quoted 10 lines · Dan Carpenter

It's possible to use "err" without initializing it. If it happens to be a 2 which is SCSI_DH_RETRY then that could cause a bug.
Signed-off-by Dan Carpenter <[email protected]>
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 8eaed05..f3c994f 100644

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) · line 520

Quoted 9 lines

drivers/scsi/device_handler/scsi_dh_alua.c +2 -1

@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
  	struct alua_port_group *tmp_pg;
  	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
  	unsigned char *desc, *buff;
-	unsigned err, retval;
+	unsigned int err = 0;
+	unsigned int retval;
  	unsigned int tpg_desc_tbl_off;
  	unsigned char orig_transition_tmo;
  	unsigned long flags;

Hello Dan,

The code that uses the 'err' variable occurs in a loop. I think the initialization of 'err' should occur after the "retry:" label.

Bart.

Dan Carpenter

author

2016-04-14 18:20 UTC #2 lore raw

PATCH v2 1/1 scsi_dh_alua: uninitialized variable in alua_rtpg()

It's possible to use "err" without initializing it. If it happens to be a 2 which is SCSI_DH_RETRY then that could cause a bug. Bart Van Assche pointed out that we should probably re-initialize it for every iteration through the retry loop.

Signed-off-by Dan Carpenter <[email protected]>

---
v2: The first version just initialized it at the start of the function.

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

index 8eaed05..a655cf2 100644
@@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_DEV_TEMP_BUSY;
 
  retry:
+	err = 0;
 	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
 	if (retval) {

Bart Van Assche

2016-04-14 18:55 UTC #3 lore raw

Quoted 14 lines · Dan Carpenter

It's possible to use "err" without initializing it. If it happens to be a 2 which is SCSI_DH_RETRY then that could cause a bug. Bart Van Assche pointed out that we should probably re-initialize it for every iteration through the retry loop.
Signed-off-by Dan Carpenter <[email protected]>
---
v2: The first version just initialized it at the start of the function.
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 8eaed05..a655cf2 100644

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) · line 538

Quoted 7 lines

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

@@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
  		return SCSI_DH_DEV_TEMP_BUSY;
 
   retry:
+	err = 0;
  	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
  	if (retval) {

Although I would have preferred that that initialization would have been closer to the other 'err' assignments this patch looks fine to me. If this patch does not get integrated in kernel v4.6 a "Cc: stable" tag will be needed.

Bart.

Hannes Reinecke

Reviewed-by on v2 1/1

2016-04-15 05:59 UTC #4 lore raw

In reply to Dan Carpenter

Quoted 15 lines · Dan Carpenter

It's possible to use "err" without initializing it. If it happens to be a 2 which is SCSI_DH_RETRY then that could cause a bug. Bart Van Assche pointed out that we should probably re-initialize it for every iteration through the retry loop.
Signed-off-by Dan Carpenter <[email protected]>
---
v2: The first version just initialized it at the start of the function.
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 8eaed05..a655cf2 100644

drivers/scsi/device_handler/scsi_dh_alua.c static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) · line 538

Quoted 7 lines

drivers/scsi/device_handler/scsi_dh_alua.c +1 -0

@@ -532,6 +532,7 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 		return SCSI_DH_DEV_TEMP_BUSY;
 
  retry:
+	err = 0;
 	retval = submit_rtpg(sdev, buff, bufflen, &sense_hdr, pg->flags);
 
 	if (retval) {

Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		   Teamlead Storage & Networking
[email protected]			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Martin K. Petersen

Applied

2016-04-15 20:26 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Dan" = Dan Carpenter <[email protected]> writes:

Dan> It's possible to use "err" without initializing it. If it happens Dan> to be a 2 which is SCSI_DH_RETRY then that could cause a bug. Bart Dan> Van Assche pointed out that we should probably re-initialize it for Dan> every iteration through the retry loop.

Applied to 4.6/scsi-fixes.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Dan Carpenter

author

2016-04-14 18:20 UTC #6 lore raw

In reply to Bart Van Assche

Quoted 23 lines · Bart Van Assche

Quoted 10 lines · Dan Carpenter
It's possible to use "err" without initializing it. If it happens to be a 2 which is SCSI_DH_RETRY then that could cause a bug.
Signed-off-by Dan Carpenter <[email protected]>
drivers/scsi/device_handler/scsi_dh_alua.c +0 -0
index 8eaed05..f3c994f 100644
drivers/scsi/device_handler/scsi_dh_alua.c static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg) · line 520
Quoted 9 lines
drivers/scsi/device_handler/scsi_dh_alua.c +2 -1
@@ -513,7 +513,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
 	struct alua_port_group *tmp_pg;
 	int len, k, off, valid_states = 0, bufflen = ALUA_RTPG_SIZE;
 	unsigned char *desc, *buff;
-	unsigned err, retval;
+	unsigned int err = 0;
+	unsigned int retval;
 	unsigned int tpg_desc_tbl_off;
 	unsigned char orig_transition_tmo;
 	unsigned long flags;
Hello Dan,

Quoted 2 lines

The code that uses the 'err' variable occurs in a loop. I think the initialization of 'err' should occur after the "retry:" label.

It looks like you're right. I'll resend. I don't know this code very well, obviously and it's a static checker fix not something I have tested.

regards,
dan carpenter