2016-11-22 14:33:35

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH] scsi: hpsa: fix uninitialized variable access

A bugfix has left the 'sd' variable uninitialized:

drivers/scsi/hpsa.c: In function 'hpsa_slave_alloc':
drivers/scsi/hpsa.c:2033:5: error: 'sd' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This reverts back to calling lookup_hpsa_scsi_dev() for the
HPSA_PHYSICAL_DEVICE_BUS case, but also keeps doing that when
hpsa_find_device_by_sas_rphy() returns NULL, as is currently
done.

The patch that caused this is marked for stable backports,
so this one has to be backported on top as well.

Fixes: 4eb307f7b18d ("scsi: hpsa: use bus '3' for legacy HBA devices")
Signed-off-by: Arnd Bergmann <[email protected]>
---
I did not try hard to figure out what the correct behavior
should be, so please treat this as a bugreport that might contain
the right fix.
---
drivers/scsi/hpsa.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index ea64c01f3d42..d17ee63045c3 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -2029,7 +2029,10 @@ static int hpsa_slave_alloc(struct scsi_device *sdev)
sd->target = sdev_id(sdev);
sd->lun = sdev->lun;
}
+ } else {
+ sd = NULL;
}
+
if (!sd)
sd = lookup_hpsa_scsi_dev(h, sdev_channel(sdev),
sdev_id(sdev), sdev->lun);
--
2.9.0


2016-11-22 14:47:14

by Hannes Reinecke

[permalink] [raw]
Subject: Re: [PATCH] scsi: hpsa: fix uninitialized variable access

On 11/22/2016 03:32 PM, Arnd Bergmann wrote:
> A bugfix has left the 'sd' variable uninitialized:
>
> drivers/scsi/hpsa.c: In function 'hpsa_slave_alloc':
> drivers/scsi/hpsa.c:2033:5: error: 'sd' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>
> This reverts back to calling lookup_hpsa_scsi_dev() for the
> HPSA_PHYSICAL_DEVICE_BUS case, but also keeps doing that when
> hpsa_find_device_by_sas_rphy() returns NULL, as is currently
> done.
>
> The patch that caused this is marked for stable backports,
> so this one has to be backported on top as well.
>
> Fixes: 4eb307f7b18d ("scsi: hpsa: use bus '3' for legacy HBA devices")
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> I did not try hard to figure out what the correct behavior
> should be, so please treat this as a bugreport that might contain
> the right fix.
> ---
> drivers/scsi/hpsa.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index ea64c01f3d42..d17ee63045c3 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -2029,7 +2029,10 @@ static int hpsa_slave_alloc(struct scsi_device *sdev)
> sd->target = sdev_id(sdev);
> sd->lun = sdev->lun;
> }
> + } else {
> + sd = NULL;
> }
> +
> if (!sd)
> sd = lookup_hpsa_scsi_dev(h, sdev_channel(sdev),
> sdev_id(sdev), sdev->lun);
>
Hmm.

I'd prefer this:

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 05f7782..ee6f852 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -2031,7 +2031,7 @@ static struct hpsa_scsi_dev_t
*lookup_hpsa_scsi_dev(struct ctlr_info *h,

static int hpsa_slave_alloc(struct scsi_device *sdev)
{
- struct hpsa_scsi_dev_t *sd;
+ struct hpsa_scsi_dev_t *sd = NULL;
unsigned long flags;
struct ctlr_info *h;


Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

2016-11-22 14:50:27

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH] scsi: hpsa: fix uninitialized variable access

On Tuesday, November 22, 2016 3:47:09 PM CET Hannes Reinecke wrote:
> index 05f7782..ee6f852 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -2031,7 +2031,7 @@ static struct hpsa_scsi_dev_t
> *lookup_hpsa_scsi_dev(struct ctlr_info *h,
>
> static int hpsa_slave_alloc(struct scsi_device *sdev)
> {
> - struct hpsa_scsi_dev_t *sd;
> + struct hpsa_scsi_dev_t *sd = NULL;
> unsigned long flags;
> struct ctlr_info *h;
>
>

I try not to add initializations like this in general, since they
prevent us from finding the bug, but here that seems fine too
as we immediately test it for NULL anyway.

Can you follow up with a patch to do that?

Arnd