[PATCH] fix kzalloc in scsi device handler

[PATCH] fix kzalloc in scsi device handler

[Hillf Danton]

They look like typo, since there are four instances of kzalloc almost
typed by the same finger.

The pointer is replaced with a struct, which helps kzalloc return
correct result.

thanks for any comment on the possibility that mem overflow could happen.

Signed-off-by: Hillf Danton <dhillf@gmail.com>
---

--- a/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-01
19:54:12.000000000 +0800
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-16
20:40:36.000000000 +0800
@@ -759,7 +759,7 @@ static int alua_bus_attach(struct scsi_d
 	unsigned long flags;
 	int err = SCSI_DH_OK;

-	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
+	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
 			       + sizeof(*h) , GFP_KERNEL);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
--- a/drivers/scsi/device_handler/scsi_dh_emc.c	2010-11-01
19:54:12.000000000 +0800
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c	2010-11-16
20:40:38.000000000 +0800
@@ -650,7 +650,7 @@ static int clariion_bus_attach(struct sc
 	unsigned long flags;
 	int err;

-	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
+	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
 			       + sizeof(*h) , GFP_KERNEL);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c	2010-11-01
19:54:12.000000000 +0800
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c	2010-11-16
20:40:36.000000000 +0800
@@ -338,7 +338,7 @@ static int hp_sw_bus_attach(struct scsi_
 	unsigned long flags;
 	int ret;

-	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
+	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
 			       + sizeof(struct hp_sw_dh_data) , GFP_KERNEL);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c	2010-11-01
19:54:12.000000000 +0800
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c	2010-11-16
20:40:32.000000000 +0800
@@ -800,7 +800,7 @@ static int rdac_bus_attach(struct scsi_d
 	int err;
 	char array_name[ARRAY_LABEL_LEN];

-	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
+	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
 			       + sizeof(*h) , GFP_KERNEL);
 	if (!scsi_dh_data) {
 		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",

RE: [PATCH] fix kzalloc in scsi device handler

[Moger, Babu]

Agree.. But this may not fix the problem completely.. Please see my comments below.. 
Also copied Chandra if he has any comments.. 

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Hillf Danton
> Sent: Tuesday, November 16, 2010 7:34 AM
> To: linux-scsi@vger.kernel.org
> Subject: [PATCH] fix kzalloc in scsi device handler
> 
> They look like typo, since there are four instances of kzalloc almost
> typed by the same finger.
> 
> The pointer is replaced with a struct, which helps kzalloc return
> correct result.
> 
> thanks for any comment on the possibility that mem overflow could
> happen.
> 
> Signed-off-by: Hillf Danton <dhillf@gmail.com>
> ---
> 
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-01
> 19:54:12.000000000 +0800
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-16
> 20:40:36.000000000 +0800
> @@ -759,7 +759,7 @@ static int alua_bus_attach(struct scsi_d
>  	unsigned long flags;
>  	int err = SCSI_DH_OK;
> 
> -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
>  			       + sizeof(*h) , GFP_KERNEL);

I think this should be like this below. 

      scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
  			       + sizeof(struct alua_dh_data) , GFP_KERNEL);

Same thing should apply for all the below segments..

>  	if (!scsi_dh_data) {
>  		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
> --- a/drivers/scsi/device_handler/scsi_dh_emc.c	2010-11-01
> 19:54:12.000000000 +0800
> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c	2010-11-16
> 20:40:38.000000000 +0800
> @@ -650,7 +650,7 @@ static int clariion_bus_attach(struct sc
>  	unsigned long flags;
>  	int err;
> 
> -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
>  			       + sizeof(*h) , GFP_KERNEL);
>  	if (!scsi_dh_data) {
>  		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
> --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c	2010-11-01
> 19:54:12.000000000 +0800
> +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c	2010-11-16
> 20:40:36.000000000 +0800
> @@ -338,7 +338,7 @@ static int hp_sw_bus_attach(struct scsi_
>  	unsigned long flags;
>  	int ret;
> 
> -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
>  			       + sizeof(struct hp_sw_dh_data) , GFP_KERNEL);
>  	if (!scsi_dh_data) {
>  		sdev_printk(KERN_ERR, sdev, "%s: Attach Failed\n",
> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c	2010-11-01
> 19:54:12.000000000 +0800
> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c	2010-11-16
> 20:40:32.000000000 +0800
> @@ -800,7 +800,7 @@ static int rdac_bus_attach(struct scsi_d
>  	int err;
>  	char array_name[ARRAY_LABEL_LEN];
> 
> -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
>  			       + sizeof(*h) , GFP_KERNEL);
>  	if (!scsi_dh_data) {
>  		sdev_printk(KERN_ERR, sdev, "%s: Attach failed\n",
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix kzalloc in scsi device handler

[Rolf Eike Beer]

[-- Attachment #1: Type: Text/Plain, Size: 1571 bytes --]

Moger, Babu wrote:
> Agree.. But this may not fix the problem completely.. Please see my
> comments below.. Also copied Chandra if he has any comments..
> 
> > -----Original Message-----
> > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > owner@vger.kernel.org] On Behalf Of Hillf Danton
> > Sent: Tuesday, November 16, 2010 7:34 AM
> > To: linux-scsi@vger.kernel.org
> > Subject: [PATCH] fix kzalloc in scsi device handler
> > 
> > They look like typo, since there are four instances of kzalloc almost
> > typed by the same finger.
> > 
> > The pointer is replaced with a struct, which helps kzalloc return
> > correct result.
> > 
> > thanks for any comment on the possibility that mem overflow could
> > happen.
> > 
> > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > ---
> > 
> > --- a/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-01
> > 19:54:12.000000000 +0800
> > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-16
> > 20:40:36.000000000 +0800
> > @@ -759,7 +759,7 @@ static int alua_bus_attach(struct scsi_d
> > 
> >  	unsigned long flags;
> >  	int err = SCSI_DH_OK;
> > 
> > -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> > +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> > 
> >  			       + sizeof(*h) , GFP_KERNEL);
> 
> I think this should be like this below.
> 
>       scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
>   			       + sizeof(struct alua_dh_data) , GFP_KERNEL);

How about kzalloc(sizeof(*scsi_dh_data) + sizeof(*h), GFP_KERNEL)?

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH] fix kzalloc in scsi device handler

[Moger, Babu]

> -----Original Message-----
> From: Rolf Eike Beer [mailto:eike-kernel@sf-tec.de]
> Sent: Tuesday, November 16, 2010 11:36 AM
> To: Moger, Babu
> Cc: Hillf Danton; linux-scsi@vger.kernel.org; Chandra Seetharaman
> Subject: Re: [PATCH] fix kzalloc in scsi device handler
> 
> Moger, Babu wrote:
> > Agree.. But this may not fix the problem completely.. Please see my
> > comments below.. Also copied Chandra if he has any comments..
> >
> > > -----Original Message-----
> > > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > > owner@vger.kernel.org] On Behalf Of Hillf Danton
> > > Sent: Tuesday, November 16, 2010 7:34 AM
> > > To: linux-scsi@vger.kernel.org
> > > Subject: [PATCH] fix kzalloc in scsi device handler
> > >
> > > They look like typo, since there are four instances of kzalloc
> almost
> > > typed by the same finger.
> > >
> > > The pointer is replaced with a struct, which helps kzalloc return
> > > correct result.
> > >
> > > thanks for any comment on the possibility that mem overflow could
> > > happen.
> > >
> > > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > > ---
> > >
> > > --- a/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-01
> > > 19:54:12.000000000 +0800
> > > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-16
> > > 20:40:36.000000000 +0800
> > > @@ -759,7 +759,7 @@ static int alua_bus_attach(struct scsi_d
> > >
> > >  	unsigned long flags;
> > >  	int err = SCSI_DH_OK;
> > >
> > > -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> > > +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> > >
> > >  			       + sizeof(*h) , GFP_KERNEL);
> >
> > I think this should be like this below.
> >
> >       scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> >   			       + sizeof(struct alua_dh_data) , GFP_KERNEL);
> 
> How about kzalloc(sizeof(*scsi_dh_data) + sizeof(*h), GFP_KERNEL)?

No.. This would only allocate couple of pointers..  The below should be good enough..

     scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
   			       + sizeof(struct alua_dh_data) , GFP_KERNEL);

Re: [PATCH] fix kzalloc in scsi device handler

[Rolf Eike Beer]

[-- Attachment #1: Type: Text/Plain, Size: 2484 bytes --]

Moger, Babu wrote:
> > -----Original Message-----
> > From: Rolf Eike Beer [mailto:eike-kernel@sf-tec.de]
> > Sent: Tuesday, November 16, 2010 11:36 AM
> > To: Moger, Babu
> > Cc: Hillf Danton; linux-scsi@vger.kernel.org; Chandra Seetharaman
> > Subject: Re: [PATCH] fix kzalloc in scsi device handler
> > 
> > Moger, Babu wrote:
> > > Agree.. But this may not fix the problem completely.. Please see my
> > > comments below.. Also copied Chandra if he has any comments..
> > > 
> > > > -----Original Message-----
> > > > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > > > owner@vger.kernel.org] On Behalf Of Hillf Danton
> > > > Sent: Tuesday, November 16, 2010 7:34 AM
> > > > To: linux-scsi@vger.kernel.org
> > > > Subject: [PATCH] fix kzalloc in scsi device handler
> > > > 
> > > > They look like typo, since there are four instances of kzalloc
> > 
> > almost
> > 
> > > > typed by the same finger.
> > > > 
> > > > The pointer is replaced with a struct, which helps kzalloc return
> > > > correct result.
> > > > 
> > > > thanks for any comment on the possibility that mem overflow could
> > > > happen.
> > > > 
> > > > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > > > ---
> > > > 
> > > > --- a/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-01
> > > > 19:54:12.000000000 +0800
> > > > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-16
> > > > 20:40:36.000000000 +0800
> > > > @@ -759,7 +759,7 @@ static int alua_bus_attach(struct scsi_d
> > > > 
> > > >  	unsigned long flags;
> > > >  	int err = SCSI_DH_OK;
> > > > 
> > > > -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> > > > +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> > > > 
> > > >  			       + sizeof(*h) , GFP_KERNEL);
> > > 
> > > I think this should be like this below.
> > > 
> > >       scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> > >       
> > >   			       + sizeof(struct alua_dh_data) , GFP_KERNEL);
> > 
> > How about kzalloc(sizeof(*scsi_dh_data) + sizeof(*h), GFP_KERNEL)?
> 
> No.. This would only allocate couple of pointers..  The below should be
> good enough..

No, it would not.

>      scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
>    			       + sizeof(struct alua_dh_data) , GFP_KERNEL);

And that is exactly what I tried to avoid. Look at the * before the variable 
name. That would allocate the space for what that pointer is pointing to.

Eike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH] fix kzalloc in scsi device handler

[Moger, Babu]

> -----Original Message-----
> From: Rolf Eike Beer [mailto:eike-kernel@sf-tec.de]
> Sent: Tuesday, November 16, 2010 1:18 PM
> To: Moger, Babu
> Cc: Hillf Danton; linux-scsi@vger.kernel.org; Chandra Seetharaman
> Subject: Re: [PATCH] fix kzalloc in scsi device handler
> 
> Moger, Babu wrote:
> > > -----Original Message-----
> > > From: Rolf Eike Beer [mailto:eike-kernel@sf-tec.de]
> > > Sent: Tuesday, November 16, 2010 11:36 AM
> > > To: Moger, Babu
> > > Cc: Hillf Danton; linux-scsi@vger.kernel.org; Chandra Seetharaman
> > > Subject: Re: [PATCH] fix kzalloc in scsi device handler
> > >
> > > Moger, Babu wrote:
> > > > Agree.. But this may not fix the problem completely.. Please see
> my
> > > > comments below.. Also copied Chandra if he has any comments..
> > > >
> > > > > -----Original Message-----
> > > > > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> > > > > owner@vger.kernel.org] On Behalf Of Hillf Danton
> > > > > Sent: Tuesday, November 16, 2010 7:34 AM
> > > > > To: linux-scsi@vger.kernel.org
> > > > > Subject: [PATCH] fix kzalloc in scsi device handler
> > > > >
> > > > > They look like typo, since there are four instances of kzalloc
> > >
> > > almost
> > >
> > > > > typed by the same finger.
> > > > >
> > > > > The pointer is replaced with a struct, which helps kzalloc
> return
> > > > > correct result.
> > > > >
> > > > > thanks for any comment on the possibility that mem overflow
> could
> > > > > happen.
> > > > >
> > > > > Signed-off-by: Hillf Danton <dhillf@gmail.com>
> > > > > ---
> > > > >
> > > > > --- a/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-01
> > > > > 19:54:12.000000000 +0800
> > > > > +++ b/drivers/scsi/device_handler/scsi_dh_alua.c	2010-11-16
> > > > > 20:40:36.000000000 +0800
> > > > > @@ -759,7 +759,7 @@ static int alua_bus_attach(struct scsi_d
> > > > >
> > > > >  	unsigned long flags;
> > > > >  	int err = SCSI_DH_OK;
> > > > >
> > > > > -	scsi_dh_data = kzalloc(sizeof(struct scsi_device_handler *)
> > > > > +	scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> > > > >
> > > > >  			       + sizeof(*h) , GFP_KERNEL);
> > > >
> > > > I think this should be like this below.
> > > >
> > > >       scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> > > >
> > > >   			       + sizeof(struct alua_dh_data) ,
> GFP_KERNEL);
> > >
> > > How about kzalloc(sizeof(*scsi_dh_data) + sizeof(*h), GFP_KERNEL)?
> >
> > No.. This would only allocate couple of pointers..  The below should
> be
> > good enough..
> 
> No, it would not.
> 
> >      scsi_dh_data = kzalloc(sizeof(struct scsi_dh_data)
> >    			       + sizeof(struct alua_dh_data) , GFP_KERNEL);
> 
> And that is exactly what I tried to avoid. Look at the * before the
> variable
> name. That would allocate the space for what that pointer is pointing
> to.

Ok.. got it.. Then, Your suggestion should work fine.. Thanks. 

Re: [PATCH] fix kzalloc in scsi device handler

[Boaz Harrosh]

On 11/16/2010 07:35 PM, Rolf Eike Beer wrote:
> 
> How about:
> 	scsi_dh_data = kzalloc(sizeof(*scsi_dh_data) + sizeof(*h), GFP_KERNEL)?
> 
> Eike

I always prefer this form because this way I have all the information in
that single statement. I'm allocating space for scsi_dh_data for what ever
that would be. The other form I have to go looking up to see what is
scsi_dh_data's type and there is place for error.

Naturally for above it would follow:
	h = typeof(h)(scsi_dh_data + 1);

My $0.017
Boaz

Re: [PATCH] fix kzalloc in scsi device handler

[Hillf Danton]

On Wed, Nov 17, 2010 at 6:04 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On 11/16/2010 07:35 PM, Rolf Eike Beer wrote:
>>
>> How about:
>>       scsi_dh_data = kzalloc(sizeof(*scsi_dh_data) + sizeof(*h), GFP_KERNEL)?
>>
>> Eike
>

I like the above, very cool :)

> I always prefer this form because this way I have all the information in
> that single statement. I'm allocating space for scsi_dh_data for what ever
> that would be. The other form I have to go looking up to see what is
> scsi_dh_data's type and there is place for error.
>
> Naturally for above it would follow:
>        h = typeof(h)(scsi_dh_data + 1);
>
> My $0.017
> Boaz
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html