[PATCH] scsi: ses fix for len and mem leaking when fail to add intf

[PATCH] scsi: ses fix for len and mem leaking when fail to add intf

[Yinghai Lu]

[PATCH] scsi: ses fix for len and mem leaking when fail to add intf

change to u32 before left shifting char
also fix leaking with scomp leaking when failing.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -369,7 +369,7 @@ static void ses_match_to_enclosure(struc
 			     VPD_INQUIRY_SIZE, NULL, SES_TIMEOUT, SES_RETRIES))
 		goto free;
 
-	len = (buf[2] << 8) + buf[3];
+	len = ((u32)buf[2] << 8) + buf[3];
 	desc = buf + 4;
 	while (desc < buf + len) {
 		enum scsi_protocol proto = desc[0] >> 4;
@@ -420,7 +420,7 @@ static int ses_intf_add(struct class_dev
 
 	if (!scsi_device_enclosure(sdev)) {
 		/* not an enclosure, but might be in one */
-		edev = 	enclosure_find(&sdev->host->shost_gendev);
+		edev = enclosure_find(&sdev->host->shost_gendev);
 		if (edev) {
 			ses_match_to_enclosure(edev, sdev);
 			class_device_put(&edev->cdev);
@@ -451,18 +451,18 @@ static int ses_intf_add(struct class_dev
 		goto err_free;
 	}
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	len = ((u32)hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf)
 		goto err_free;
 
-	ses_dev->page1 = buf;
-	ses_dev->page1_len = len;
-
 	result = ses_recv_diag(sdev, 1, buf, len);
 	if (result)
 		goto recv_failed;
 
+	ses_dev->page1 = buf;
+	ses_dev->page1_len = len;
+
 	types = buf[10];
 	len = buf[11];
 
@@ -474,11 +474,12 @@ static int ses_intf_add(struct class_dev
 			components += type_ptr[1];
 	}
 
+	buf = NULL;
 	result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
 		goto recv_failed;
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	len = ((u32)hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf)
 		goto err_free;
@@ -492,11 +493,12 @@ static int ses_intf_add(struct class_dev
 
 	/* The additional information page --- allows us
 	 * to match up the devices */
+	buf = NULL;
 	result = ses_recv_diag(sdev, 10, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
 		goto no_page10;
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	len = ((u32)hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	buf = kzalloc(len, GFP_KERNEL);
 	if (!buf)
 		goto err_free;
@@ -506,16 +508,18 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 	ses_dev->page10 = buf;
 	ses_dev->page10_len = len;
+	buf = NULL;
 
  no_page10:
 	scomp = kmalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
 	if (!scomp)
-		goto  err_free;
+		goto err_free;
 
 	edev = enclosure_register(cdev->dev, sdev->sdev_gendev.bus_id,
 				  components, &ses_enclosure_callbacks);
 	if (IS_ERR(edev)) {
 		err = PTR_ERR(edev);
+		kfree(scomp);
 		goto err_free;
 	}
 
@@ -524,24 +528,27 @@ static int ses_intf_add(struct class_dev
 		edev->component[i].scratch = scomp++;
 
 	/* Page 7 for the descriptors is optional */
-	buf = NULL;
 	result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
 		goto simple_populate;
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	len = ((u32)hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	/* add 1 for trailing '\0' we'll use */
 	buf = kzalloc(len + 1, GFP_KERNEL);
-	result = ses_recv_diag(sdev, 7, buf, len);
-	if (result) {
+	if (buf)
+		result = ses_recv_diag(sdev, 7, buf, len);
+	else
+		result = 7;
+
  simple_populate:
+	if (result) {
 		kfree(buf);
 		buf = NULL;
 		desc_ptr = NULL;
 		addl_desc_ptr = NULL;
 	} else {
 		desc_ptr = buf + 8;
-		len = (desc_ptr[2] << 8) + desc_ptr[3];
+		len = ((u32)desc_ptr[2] << 8) + desc_ptr[3];
 		/* skip past overall descriptor */
 		desc_ptr += len + 4;
 		addl_desc_ptr = ses_dev->page10 + 8;
@@ -554,7 +561,7 @@ static int ses_intf_add(struct class_dev
 			struct enclosure_component *ecomp;
 
 			if (desc_ptr) {
-				len = (desc_ptr[2] << 8) + desc_ptr[3];
+				len = ((u32)desc_ptr[2] << 8) + desc_ptr[3];
 				desc_ptr += 4;
 				/* Add trailing zero - pushes into
 				 * reserved space */
@@ -575,7 +582,7 @@ static int ses_intf_add(struct class_dev
 							       addl_desc_ptr);
 
 				if (addl_desc_ptr)
-					addl_desc_ptr += addl_desc_ptr[1] + 2;
+					addl_desc_ptr += 2 + addl_desc_ptr[1];
 			}
 		}
 	}
@@ -597,7 +604,6 @@ static int ses_intf_add(struct class_dev
 		    result);
 	err = -ENODEV;
  err_free:
-	kfree(buf);
 	kfree(ses_dev->page10);
 	kfree(ses_dev->page2);
 	kfree(ses_dev->page1);
@@ -630,6 +636,7 @@ static void ses_intf_remove(struct class
 	ses_dev = edev->scratch;
 	edev->scratch = NULL;
 
+	kfree(ses_dev->page10);
 	kfree(ses_dev->page1);
 	kfree(ses_dev->page2);
 	kfree(ses_dev);

Re: [PATCH] scsi: ses fix for len and mem leaking when fail to add intf

[James Bottomley]

On Sat, 2008-02-09 at 04:13 -0800, Yinghai Lu wrote:
> [PATCH] scsi: ses fix for len and mem leaking when fail to add intf
> 
> change to u32 before left shifting char

This one is a bit unnecessary; C promotion rules guarantee that
everything is promoted to int (or above) before doing arithmetic.  Since
it's only ever done on 16 bits, signed or unsigned int is adequate for
the conversion.

> also fix leaking with scomp leaking when failing.

Yes, I see that, thanks!  There's also the kmalloc of scomp which should
be kzalloc if you care to fix that up in the resend.

> -		edev = 	enclosure_find(&sdev->host->shost_gendev);
> +		edev = enclosure_find(&sdev->host->shost_gendev);

Space cleanups also need mention in the changelog.

> -	ses_dev->page1 = buf;
> -	ses_dev->page1_len = len;
> -
>  	result = ses_recv_diag(sdev, 1, buf, len);
>  	if (result)
>  		goto recv_failed;
>  
> +	ses_dev->page1 = buf;
> +	ses_dev->page1_len = len;
> +

Neither of us gets this right.  By removing the kfree(buf) from the
err_free path, you cause a leak here.  I cause a double free.  I think
putting back the kfree(buf) and keeping this hunk is the fix.

>  	types = buf[10];
>  	len = buf[11];
>  
> @@ -474,11 +474,12 @@ static int ses_intf_add(struct class_dev
>  			components += type_ptr[1];
>  	}
>  
> +	buf = NULL;

Yes, prevents double free (but only if buf is freed).

>  	result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
>  	if (result)
>  		goto recv_failed;
>  
> @@ -492,11 +493,12 @@ static int ses_intf_add(struct class_dev
>  
>  	/* The additional information page --- allows us
>  	 * to match up the devices */
> +	buf = NULL;

It's probably better to move these closer to the statements that make
them necessary (in this case above the comment).

>  	if (IS_ERR(edev)) {
>  		err = PTR_ERR(edev);
> +		kfree(scomp);
>  		goto err_free;
>  	}

kfree(scomp) should be in the err_free path just in case someone else
adds something to this.
 
>  	/* add 1 for trailing '\0' we'll use */
>  	buf = kzalloc(len + 1, GFP_KERNEL);
> -	result = ses_recv_diag(sdev, 7, buf, len);
> -	if (result) {
> +	if (buf)
> +		result = ses_recv_diag(sdev, 7, buf, len);
> +	else
> +		result = 7;
> +

What exactly is this supposed to be doing, and why 7?  If you're
thinking of conditioning the page 7 receive on the success of the
allocation, we really need the allocation failure report more than we
need the driver to attach.

> -					addl_desc_ptr += addl_desc_ptr[1] + 2;
> +					addl_desc_ptr += 2 + addl_desc_ptr[1];

This is rather pointless, isn't it?

>   err_free:
> -	kfree(buf);

You can't remove this.  Also add kfree(scomp) here.

>  	kfree(ses_dev->page10);
>  	kfree(ses_dev->page2);
>  	kfree(ses_dev->page1);
> @@ -630,6 +636,7 @@ static void ses_intf_remove(struct class
>  	ses_dev = edev->scratch;
>  	edev->scratch = NULL;
>  
> +	kfree(ses_dev->page10);

Yes, a bug, thanks.

>  	kfree(ses_dev->page1);
>  	kfree(ses_dev->page2);
>  	kfree(ses_dev);

James

Re: [PATCH] scsi: ses fix for len and mem leaking when fail to add intf

[Yinghai Lu]

On Feb 9, 2008 7:00 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Sat, 2008-02-09 at 04:13 -0800, Yinghai Lu wrote:
> > [PATCH] scsi: ses fix for len and mem leaking when fail to add intf
> >
> > change to u32 before left shifting char
>
> This one is a bit unnecessary; C promotion rules guarantee that
> everything is promoted to int (or above) before doing arithmetic.  Since
> it's only ever done on 16 bits, signed or unsigned int is adequate for
> the conversion.

thank. just learned that.

yhlu@yhlunb:~/xx/xx/notes> cat ctest.c
#include <stdio.h>
int main(int argc, char *argv[])
{
        unsigned char buf[20];
        int len;

        buf[2] = 0x02;
        buf[3] = 0x03;

        len = (buf[2] << 8) + buf[3];

        printf("len = %x\n", len);
        return 0;
}
yhlu@yhlunb:~/xx/xx/notes> gcc -o ctest ctest.c
yhlu@yhlunb:~/xx/xx/notes> ./ctest
len = 203
yhlu@yhlunb:~/xx/xx/notes>


>
> > also fix leaking with scomp leaking when failing.
>
> Yes, I see that, thanks!  There's also the kmalloc of scomp which should
> be kzalloc if you care to fix that up in the resend.
>
> > -             edev =  enclosure_find(&sdev->host->shost_gendev);
> > +             edev = enclosure_find(&sdev->host->shost_gendev);
>
> Space cleanups also need mention in the changelog.
>
> > -     ses_dev->page1 = buf;
> > -     ses_dev->page1_len = len;
> > -
> >       result = ses_recv_diag(sdev, 1, buf, len);
> >       if (result)
> >               goto recv_failed;
> >
> > +     ses_dev->page1 = buf;
> > +     ses_dev->page1_len = len;
> > +
>
> Neither of us gets this right.  By removing the kfree(buf) from the
> err_free path, you cause a leak here.  I cause a double free.  I think
> putting back the kfree(buf) and keeping this hunk is the fix.

the buf already become sdev->page1, sdev->pag10, sdev->page2.
so it will be freed via them

>
> >       types = buf[10];
> >       len = buf[11];
> >
> > @@ -474,11 +474,12 @@ static int ses_intf_add(struct class_dev
> >                       components += type_ptr[1];
> >       }
> >
> > +     buf = NULL;
>
> Yes, prevents double free (but only if buf is freed).

it became sdev->page1 already

>
> >       result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
> >       if (result)
> >               goto recv_failed;
> >
> > @@ -492,11 +493,12 @@ static int ses_intf_add(struct class_dev
> >
> >       /* The additional information page --- allows us
> >        * to match up the devices */
> > +     buf = NULL;
>
> It's probably better to move these closer to the statements that make
> them necessary (in this case above the comment).

OK

>
> >       if (IS_ERR(edev)) {
> >               err = PTR_ERR(edev);
> > +             kfree(scomp);
> >               goto err_free;
> >       }
>
> kfree(scomp) should be in the err_free path just in case someone else
> adds something to this.

ok.

>
> >       /* add 1 for trailing '\0' we'll use */
> >       buf = kzalloc(len + 1, GFP_KERNEL);
> > -     result = ses_recv_diag(sdev, 7, buf, len);
> > -     if (result) {
> > +     if (buf)
> > +             result = ses_recv_diag(sdev, 7, buf, len);
> > +     else
> > +             result = 7;
> > +
>
> What exactly is this supposed to be doing, and why 7?  If you're
> thinking of conditioning the page 7 receive on the success of the
> allocation, we really need the allocation failure report more than we
> need the driver to attach.

want to move out label out of if later.

>
> > -                                     addl_desc_ptr += addl_desc_ptr[1] + 2;
> > +                                     addl_desc_ptr += 2 + addl_desc_ptr[1];
>
> This is rather pointless, isn't it?
>
> >   err_free:
> > -     kfree(buf);
>
> You can't remove this.  Also add kfree(scomp) here.

[PATCH] scsi: ses fix mem leaking when fail to add intf

[Yinghai Lu]

[PATCH] scsi: ses fix mem leaking when fail to add intf

fix leaking with scomp leaking when failing.
also remove one extra space.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -416,11 +416,11 @@ static int ses_intf_add(struct class_dev
 	int i, j, types, len, components = 0;
 	int err = -ENOMEM;
 	struct enclosure_device *edev;
-	struct ses_component *scomp;
+	struct ses_component *scomp = NULL;
 
 	if (!scsi_device_enclosure(sdev)) {
 		/* not an enclosure, but might be in one */
-		edev = 	enclosure_find(&sdev->host->shost_gendev);
+		edev = enclosure_find(&sdev->host->shost_gendev);
 		if (edev) {
 			ses_match_to_enclosure(edev, sdev);
 			class_device_put(&edev->cdev);
@@ -456,9 +456,6 @@ static int ses_intf_add(struct class_dev
 	if (!buf)
 		goto err_free;
 
-	ses_dev->page1 = buf;
-	ses_dev->page1_len = len;
-
 	result = ses_recv_diag(sdev, 1, buf, len);
 	if (result)
 		goto recv_failed;
@@ -473,6 +470,9 @@ static int ses_intf_add(struct class_dev
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 			components += type_ptr[1];
 	}
+	ses_dev->page1 = buf;
+	ses_dev->page1_len = len;
+	buf = NULL;
 
 	result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
@@ -489,6 +489,7 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 	ses_dev->page2 = buf;
 	ses_dev->page2_len = len;
+	buf = NULL;
 
 	/* The additional information page --- allows us
 	 * to match up the devices */
@@ -506,11 +507,26 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 	ses_dev->page10 = buf;
 	ses_dev->page10_len = len;
+	buf = NULL;
 
  no_page10:
-	scomp = kmalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
+
+	/* Page 7 for the descriptors is optional */
+	result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
+	if (result)
+		goto simple_populate;
+
+	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	/* add 1 for trailing '\0' we'll use */
+	buf = kzalloc(len + 1, GFP_KERNEL);
+	if (!buf)
+		goto err_free;
+	result = ses_recv_diag(sdev, 7, buf, len);
+
+ simple_populate:
+	scomp = kzalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
 	if (!scomp)
-		goto  err_free;
+		goto err_free;
 
 	edev = enclosure_register(cdev->dev, sdev->sdev_gendev.bus_id,
 				  components, &ses_enclosure_callbacks);
@@ -521,20 +537,10 @@ static int ses_intf_add(struct class_dev
 
 	edev->scratch = ses_dev;
 	for (i = 0; i < components; i++)
-		edev->component[i].scratch = scomp++;
+		edev->component[i].scratch = scomp + i;
 
-	/* Page 7 for the descriptors is optional */
-	buf = NULL;
-	result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
-	if (result)
-		goto simple_populate;
-
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
-	/* add 1 for trailing '\0' we'll use */
-	buf = kzalloc(len + 1, GFP_KERNEL);
-	result = ses_recv_diag(sdev, 7, buf, len);
+	/* result and buf from page 7 check */
 	if (result) {
- simple_populate:
 		kfree(buf);
 		buf = NULL;
 		desc_ptr = NULL;
@@ -598,6 +604,7 @@ static int ses_intf_add(struct class_dev
 	err = -ENODEV;
  err_free:
 	kfree(buf);
+	kfree(scomp);
 	kfree(ses_dev->page10);
 	kfree(ses_dev->page2);
 	kfree(ses_dev->page1);
@@ -630,6 +637,7 @@ static void ses_intf_remove(struct class
 	ses_dev = edev->scratch;
 	edev->scratch = NULL;
 
+	kfree(ses_dev->page10);
 	kfree(ses_dev->page1);
 	kfree(ses_dev->page2);
 	kfree(ses_dev);

Re: [PATCH] scsi: ses fix mem leaking when fail to add intf

[James Bottomley]

On Sat, 2008-02-09 at 15:15 -0800, Yinghai Lu wrote:
> [PATCH] scsi: ses fix mem leaking when fail to add intf
> 
> fix leaking with scomp leaking when failing.
> also remove one extra space.

There are still a few extraneous code moves in this one.  This is about
the correct minimal set, isn't it?

James

---

From: Yinghai Lu <Yinghai.Lu@Sun.COM>
Date: Sat, 9 Feb 2008 15:15:47 -0800
Subject: [SCSI] ses: fix memory leaks

fix leaking with scomp leaking when failing. Also free page10 on
driver removal  and remove one extra space.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ses.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index 2a6e4f4..8abc4a9 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -416,11 +416,11 @@ static int ses_intf_add(struct class_device *cdev,
 	int i, j, types, len, components = 0;
 	int err = -ENOMEM;
 	struct enclosure_device *edev;
-	struct ses_component *scomp;
+	struct ses_component *scomp = NULL;
 
 	if (!scsi_device_enclosure(sdev)) {
 		/* not an enclosure, but might be in one */
-		edev = 	enclosure_find(&sdev->host->shost_gendev);
+		edev = enclosure_find(&sdev->host->shost_gendev);
 		if (edev) {
 			ses_match_to_enclosure(edev, sdev);
 			class_device_put(&edev->cdev);
@@ -456,9 +456,6 @@ static int ses_intf_add(struct class_device *cdev,
 	if (!buf)
 		goto err_free;
 
-	ses_dev->page1 = buf;
-	ses_dev->page1_len = len;
-
 	result = ses_recv_diag(sdev, 1, buf, len);
 	if (result)
 		goto recv_failed;
@@ -473,6 +470,9 @@ static int ses_intf_add(struct class_device *cdev,
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 			components += type_ptr[1];
 	}
+	ses_dev->page1 = buf;
+	ses_dev->page1_len = len;
+	buf = NULL;
 
 	result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
@@ -489,6 +489,7 @@ static int ses_intf_add(struct class_device *cdev,
 		goto recv_failed;
 	ses_dev->page2 = buf;
 	ses_dev->page2_len = len;
+	buf = NULL;
 
 	/* The additional information page --- allows us
 	 * to match up the devices */
@@ -506,11 +507,12 @@ static int ses_intf_add(struct class_device *cdev,
 		goto recv_failed;
 	ses_dev->page10 = buf;
 	ses_dev->page10_len = len;
+	buf = NULL;
 
  no_page10:
-	scomp = kmalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
+	scomp = kzalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
 	if (!scomp)
-		goto  err_free;
+		goto err_free;
 
 	edev = enclosure_register(cdev->dev, sdev->sdev_gendev.bus_id,
 				  components, &ses_enclosure_callbacks);
@@ -521,7 +523,7 @@ static int ses_intf_add(struct class_device *cdev,
 
 	edev->scratch = ses_dev;
 	for (i = 0; i < components; i++)
-		edev->component[i].scratch = scomp++;
+		edev->component[i].scratch = scomp + i;
 
 	/* Page 7 for the descriptors is optional */
 	buf = NULL;
@@ -598,6 +600,7 @@ static int ses_intf_add(struct class_device *cdev,
 	err = -ENODEV;
  err_free:
 	kfree(buf);
+	kfree(scomp);
 	kfree(ses_dev->page10);
 	kfree(ses_dev->page2);
 	kfree(ses_dev->page1);
@@ -630,6 +633,7 @@ static void ses_intf_remove(struct class_device *cdev,
 	ses_dev = edev->scratch;
 	edev->scratch = NULL;
 
+	kfree(ses_dev->page10);
 	kfree(ses_dev->page1);
 	kfree(ses_dev->page2);
 	kfree(ses_dev);
-- 
1.5.3.8

Re: [PATCH] scsi: ses fix mem leaking when fail to add intf

[Yinghai Lu]

On Sunday 10 February 2008 08:28:38 pm James Bottomley wrote:
> 
> On Sat, 2008-02-09 at 15:15 -0800, Yinghai Lu wrote:
> > [PATCH] scsi: ses fix mem leaking when fail to add intf
> > 
> > fix leaking with scomp leaking when failing.
> > also remove one extra space.
> 
> There are still a few extraneous code moves in this one.  This is about
> the correct minimal set, isn't it?


if buf allocation for page 7 get NULL...

if put 
+ if (!buf)
+	goto err_free;

still not right, because still undo 
  	edev = enclosure_register(cdev->dev, sdev->sdev_gendev.bus_id,
 				  components, &ses_enclosure_callbacks);

all just add 
+ if (!buf)
+	goto simple_populate;

there?

YH

[SCSI] ses: fix memory leaks

[Yinghai Lu]

please check it...

---

From: Yinghai Lu <yinghai.lu@sun.com>

[SCSI] ses: fix memory leaks

fix leaking with scomp leaking when failing. Also free page10 on
driver removal  and remove one extra space.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---

 drivers/scsi/ses.c |   23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -416,11 +416,11 @@ static int ses_intf_add(struct class_dev
 	int i, j, types, len, components = 0;
 	int err = -ENOMEM;
 	struct enclosure_device *edev;
-	struct ses_component *scomp;
+	struct ses_component *scomp = NULL;
 
 	if (!scsi_device_enclosure(sdev)) {
 		/* not an enclosure, but might be in one */
-		edev = 	enclosure_find(&sdev->host->shost_gendev);
+		edev = enclosure_find(&sdev->host->shost_gendev);
 		if (edev) {
 			ses_match_to_enclosure(edev, sdev);
 			class_device_put(&edev->cdev);
@@ -456,9 +456,6 @@ static int ses_intf_add(struct class_dev
 	if (!buf)
 		goto err_free;
 
-	ses_dev->page1 = buf;
-	ses_dev->page1_len = len;
-
 	result = ses_recv_diag(sdev, 1, buf, len);
 	if (result)
 		goto recv_failed;
@@ -473,6 +470,9 @@ static int ses_intf_add(struct class_dev
 		    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE)
 			components += type_ptr[1];
 	}
+	ses_dev->page1 = buf;
+	ses_dev->page1_len = len;
+	buf = NULL;
 
 	result = ses_recv_diag(sdev, 2, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
@@ -489,6 +489,7 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 	ses_dev->page2 = buf;
 	ses_dev->page2_len = len;
+	buf = NULL;
 
 	/* The additional information page --- allows us
 	 * to match up the devices */
@@ -506,11 +507,12 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 	ses_dev->page10 = buf;
 	ses_dev->page10_len = len;
+	buf = NULL;
 
  no_page10:
-	scomp = kmalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
+	scomp = kzalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
 	if (!scomp)
-		goto  err_free;
+		goto err_free;
 
 	edev = enclosure_register(cdev->dev, sdev->sdev_gendev.bus_id,
 				  components, &ses_enclosure_callbacks);
@@ -521,10 +523,9 @@ static int ses_intf_add(struct class_dev
 
 	edev->scratch = ses_dev;
 	for (i = 0; i < components; i++)
-		edev->component[i].scratch = scomp++;
+		edev->component[i].scratch = scomp + i;
 
 	/* Page 7 for the descriptors is optional */
-	buf = NULL;
 	result = ses_recv_diag(sdev, 7, hdr_buf, INIT_ALLOC_SIZE);
 	if (result)
 		goto simple_populate;
@@ -532,6 +533,8 @@ static int ses_intf_add(struct class_dev
 	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	/* add 1 for trailing '\0' we'll use */
 	buf = kzalloc(len + 1, GFP_KERNEL);
+	if (!buf)
+		goto simple_polulate;
 	result = ses_recv_diag(sdev, 7, buf, len);
 	if (result) {
  simple_populate:
@@ -598,6 +601,7 @@ static int ses_intf_add(struct class_dev
 	err = -ENODEV;
  err_free:
 	kfree(buf);
+	kfree(scomp);
 	kfree(ses_dev->page10);
 	kfree(ses_dev->page2);
 	kfree(ses_dev->page1);
@@ -630,6 +634,7 @@ static void ses_intf_remove(struct class
 	ses_dev = edev->scratch;
 	edev->scratch = NULL;
 
+	kfree(ses_dev->page10);
 	kfree(ses_dev->page1);
 	kfree(ses_dev->page2);
 	kfree(ses_dev);

Re: [SCSI] ses: fix memory leaks

[James Bottomley]

On Sun, 2008-02-10 at 23:25 -0800, Yinghai Lu wrote:
> please check it...

This one looks perfect, thanks!

James

Re: [SCSI] ses: fix memory leaks

[James Bottomley]

On Mon, 2008-02-11 at 10:23 -0600, James Bottomley wrote:
> On Sun, 2008-02-10 at 23:25 -0800, Yinghai Lu wrote:
> > please check it...
> 
> This one looks perfect, thanks!

Well, nearly perfect.  I corrected this typo:

+       if (!buf)
+               goto simple_polulate;
                            ^^^^^^^^

Which a compile check before you submitted the patch would have picked
up ...

James

Re: [SCSI] ses: fix memory leaks

[Yinghai Lu]

On Monday 11 February 2008 09:02:06 am James Bottomley wrote:
> 
> On Mon, 2008-02-11 at 10:23 -0600, James Bottomley wrote:
> > On Sun, 2008-02-10 at 23:25 -0800, Yinghai Lu wrote:
> > > please check it...
> > 
> > This one looks perfect, thanks!
> 
> Well, nearly perfect.  I corrected this typo:
> 
> +       if (!buf)
> +               goto simple_polulate;
>                             ^^^^^^^^
> 
> Which a compile check before you submitted the patch would have picked
> up ...

sorry for that.

[PATCH] SCSI: fix data corruption caused by ses

[Yinghai Lu]

one system: initrd get courrupted:

RAMDISK: Compressed image found at block 0
RAMDISK: incomplete write (-28 != 2048) 134217728
crc error
VFS: Mounted root (ext2 filesystem).
Freeing unused kernel memory: 388k freed
init_special_inode: bogus i_mode (177777)
Warning: unable to open an initial console.
init_special_inode: bogus i_mode (177777)
init_special_inode: bogus i_mode (177777)
Kernel panic - not syncing: No init found.  Try passing init= option to kernel.

bisected to
commit 9927c68864e9c39cc317b4f559309ba29e642168
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Sun Feb 3 15:48:56 2008 -0600

    [SCSI] ses: add new Enclosure ULD

changes:
1. change char to unsigned char to avoid type change later.
2. preserve len for page1
3. need to move desc_ptr even the entry is not enclosure_component_device/raid.
   so keep desc_ptr on right position
4. also record page7 len, and double check if desc_ptr out of boundary before touch.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -33,9 +33,9 @@
 #include <scsi/scsi_host.h>
 
 struct ses_device {
-	char *page1;
-	char *page2;
-	char *page10;
+	unsigned char *page1;
+	unsigned char *page2;
+	unsigned char *page10;
 	short page1_len;
 	short page2_len;
 	short page10_len;
@@ -67,7 +67,7 @@ static int ses_probe(struct device *dev)
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 			 void *buf, int bufflen)
 {
-	char cmd[] = {
+	unsigned char cmd[] = {
 		RECEIVE_DIAGNOSTIC,
 		1,		/* Set PCV bit */
 		page_code,
@@ -85,7 +85,7 @@ static int ses_send_diag(struct scsi_dev
 {
 	u32 result;
 
-	char cmd[] = {
+	unsigned char cmd[] = {
 		SEND_DIAGNOSTIC,
 		0x10,		/* Set PF bit */
 		0,
@@ -104,13 +104,13 @@ static int ses_send_diag(struct scsi_dev
 
 static int ses_set_page2_descriptor(struct enclosure_device *edev,
 				      struct enclosure_component *ecomp,
-				      char *desc)
+				      unsigned char *desc)
 {
 	int i, j, count = 0, descriptor = ecomp->number;
 	struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
 	struct ses_device *ses_dev = edev->scratch;
-	char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
-	char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
 	/* Clear everything */
 	memset(desc_ptr, 0, ses_dev->page2_len - 8);
@@ -133,14 +133,14 @@ static int ses_set_page2_descriptor(stru
 	return ses_send_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 }
 
-static char *ses_get_page2_descriptor(struct enclosure_device *edev,
+static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 				      struct enclosure_component *ecomp)
 {
 	int i, j, count = 0, descriptor = ecomp->number;
 	struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
 	struct ses_device *ses_dev = edev->scratch;
-	char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
-	char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
 	ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 
@@ -160,17 +160,18 @@ static char *ses_get_page2_descriptor(st
 static void ses_get_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->fault = (desc[3] & 0x60) >> 4;
+	if (desc)
+		ecomp->fault = (desc[3] & 0x60) >> 4;
 }
 
 static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -190,26 +191,28 @@ static int ses_set_fault(struct enclosur
 static void ses_get_status(struct enclosure_device *edev,
 			   struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->status = (desc[0] & 0x0f);
+	if (desc)
+		ecomp->status = (desc[0] & 0x0f);
 }
 
 static void ses_get_locate(struct enclosure_device *edev,
 			   struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
+	if (desc)
+		ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
 }
 
 static int ses_set_locate(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -229,7 +232,7 @@ static int ses_set_active(struct enclosu
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -409,11 +412,11 @@ static int ses_intf_add(struct class_dev
 {
 	struct scsi_device *sdev = to_scsi_device(cdev->dev);
 	struct scsi_device *tmp_sdev;
-	unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr,
-		*addl_desc_ptr;
+	unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr = NULL,
+		*addl_desc_ptr = NULL;
 	struct ses_device *ses_dev;
 	u32 result;
-	int i, j, types, len, components = 0;
+	int i, j, types, len, page7_len = 0, components = 0;
 	int err = -ENOMEM;
 	struct enclosure_device *edev;
 	struct ses_component *scomp = NULL;
@@ -461,9 +464,8 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 
 	types = buf[10];
-	len = buf[11];
 
-	type_ptr = buf + 12 + len;
+	type_ptr = buf + 12 + buf[11];
 
 	for (i = 0; i < types; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
@@ -530,7 +532,7 @@ static int ses_intf_add(struct class_dev
 	if (result)
 		goto simple_populate;
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	/* add 1 for trailing '\0' we'll use */
 	buf = kzalloc(len + 1, GFP_KERNEL);
 	if (!buf)
@@ -547,7 +549,8 @@ static int ses_intf_add(struct class_dev
 		len = (desc_ptr[2] << 8) + desc_ptr[3];
 		/* skip past overall descriptor */
 		desc_ptr += len + 4;
-		addl_desc_ptr = ses_dev->page10 + 8;
+		if (ses_dev->page10)
+			addl_desc_ptr = ses_dev->page10 + 8;
 	}
 	type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
 	components = 0;
@@ -557,6 +560,8 @@ static int ses_intf_add(struct class_dev
 			struct enclosure_component *ecomp;
 
 			if (desc_ptr) {
+				if (desc_ptr >= buf + page7_len)
+					break;
 				len = (desc_ptr[2] << 8) + desc_ptr[3];
 				desc_ptr += 4;
 				/* Add trailing zero - pushes into
@@ -566,16 +571,19 @@ static int ses_intf_add(struct class_dev
 			}
 			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
 			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
-				continue;
+				goto next;
+
 			ecomp =	enclosure_component_register(edev,
 							     components++,
 							     type_ptr[0],
 							     name);
+
+			if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr)
+				ses_process_descriptor(ecomp,
+						       addl_desc_ptr);
+		next:
 			if (desc_ptr) {
 				desc_ptr += len;
-				if (!IS_ERR(ecomp))
-					ses_process_descriptor(ecomp,
-							       addl_desc_ptr);
 
 				if (addl_desc_ptr)
 					addl_desc_ptr += addl_desc_ptr[1] + 2;

Re: [PATCH] SCSI: fix data corruption caused by ses

[James Bottomley]

On Tue, 2008-02-12 at 23:10 -0800, Yinghai Lu wrote:
>  			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
>  			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
> -				continue;
> +				goto next;
> +
>  			ecomp =	enclosure_component_register(edev,
>  							     components++,
>  							     type_ptr[0],
>  							     name);
> +
> +			if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr)
> +				ses_process_descriptor(ecomp,
> +						       addl_desc_ptr);
> +		next:
>  			if (desc_ptr) {
>  				desc_ptr += len;
> -				if (!IS_ERR(ecomp))
> -					ses_process_descriptor(ecomp,
> -							       addl_desc_ptr);
>  
>  				if (addl_desc_ptr)
>  					addl_desc_ptr += addl_desc_ptr[1] + 2;

Everything looks fine, thanks, except this piece.

That 

if (x)
     goto next;
...
next:

Needs to be

if (!x) {
   ...
}

I've fixed it up below. (I suppose the same thing goes for the
no_page10: label as well).

James

---

From: Yinghai Lu <Yinghai.Lu@Sun.COM>
Date: Tue, 12 Feb 2008 23:10:22 -0800
Subject: [SCSI] ses: fix data corruption

one system: initrd get courrupted:

RAMDISK: Compressed image found at block 0
RAMDISK: incomplete write (-28 != 2048) 134217728
crc error
VFS: Mounted root (ext2 filesystem).
Freeing unused kernel memory: 388k freed
init_special_inode: bogus i_mode (177777)
Warning: unable to open an initial console.
init_special_inode: bogus i_mode (177777)
init_special_inode: bogus i_mode (177777)
Kernel panic - not syncing: No init found.  Try passing init= option to kernel.

bisected to
commit 9927c68864e9c39cc317b4f559309ba29e642168
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Sun Feb 3 15:48:56 2008 -0600

    [SCSI] ses: add new Enclosure ULD

changes:
1. change char to unsigned char to avoid type change later.
2. preserve len for page1
3. need to move desc_ptr even the entry is not
   enclosure_component_device/raid.  so keep desc_ptr on right
   position
4. also record page7 len, and double check if desc_ptr out of boundary
   before touch.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/scsi/ses.c |   75 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index a57fed4..614879e 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -33,9 +33,9 @@
 #include <scsi/scsi_host.h>
 
 struct ses_device {
-	char *page1;
-	char *page2;
-	char *page10;
+	unsigned char *page1;
+	unsigned char *page2;
+	unsigned char *page10;
 	short page1_len;
 	short page2_len;
 	short page10_len;
@@ -67,7 +67,7 @@ static int ses_probe(struct device *dev)
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 			 void *buf, int bufflen)
 {
-	char cmd[] = {
+	unsigned char cmd[] = {
 		RECEIVE_DIAGNOSTIC,
 		1,		/* Set PCV bit */
 		page_code,
@@ -85,7 +85,7 @@ static int ses_send_diag(struct scsi_device *sdev, int page_code,
 {
 	u32 result;
 
-	char cmd[] = {
+	unsigned char cmd[] = {
 		SEND_DIAGNOSTIC,
 		0x10,		/* Set PF bit */
 		0,
@@ -104,13 +104,13 @@ static int ses_send_diag(struct scsi_device *sdev, int page_code,
 
 static int ses_set_page2_descriptor(struct enclosure_device *edev,
 				      struct enclosure_component *ecomp,
-				      char *desc)
+				      unsigned char *desc)
 {
 	int i, j, count = 0, descriptor = ecomp->number;
 	struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
 	struct ses_device *ses_dev = edev->scratch;
-	char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
-	char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
 	/* Clear everything */
 	memset(desc_ptr, 0, ses_dev->page2_len - 8);
@@ -133,14 +133,14 @@ static int ses_set_page2_descriptor(struct enclosure_device *edev,
 	return ses_send_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 }
 
-static char *ses_get_page2_descriptor(struct enclosure_device *edev,
+static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 				      struct enclosure_component *ecomp)
 {
 	int i, j, count = 0, descriptor = ecomp->number;
 	struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
 	struct ses_device *ses_dev = edev->scratch;
-	char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
-	char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
 	ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 
@@ -160,17 +160,18 @@ static char *ses_get_page2_descriptor(struct enclosure_device *edev,
 static void ses_get_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->fault = (desc[3] & 0x60) >> 4;
+	if (desc)
+		ecomp->fault = (desc[3] & 0x60) >> 4;
 }
 
 static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -190,26 +191,28 @@ static int ses_set_fault(struct enclosure_device *edev,
 static void ses_get_status(struct enclosure_device *edev,
 			   struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->status = (desc[0] & 0x0f);
+	if (desc)
+		ecomp->status = (desc[0] & 0x0f);
 }
 
 static void ses_get_locate(struct enclosure_device *edev,
 			   struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
+	if (desc)
+		ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
 }
 
 static int ses_set_locate(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -229,7 +232,7 @@ static int ses_set_active(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -409,11 +412,11 @@ static int ses_intf_add(struct class_device *cdev,
 {
 	struct scsi_device *sdev = to_scsi_device(cdev->dev);
 	struct scsi_device *tmp_sdev;
-	unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr,
-		*addl_desc_ptr;
+	unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr = NULL,
+		*addl_desc_ptr = NULL;
 	struct ses_device *ses_dev;
 	u32 result;
-	int i, j, types, len, components = 0;
+	int i, j, types, len, page7_len = 0, components = 0;
 	int err = -ENOMEM;
 	struct enclosure_device *edev;
 	struct ses_component *scomp = NULL;
@@ -461,9 +464,8 @@ static int ses_intf_add(struct class_device *cdev,
 		goto recv_failed;
 
 	types = buf[10];
-	len = buf[11];
 
-	type_ptr = buf + 12 + len;
+	type_ptr = buf + 12 + buf[11];
 
 	for (i = 0; i < types; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
@@ -530,7 +532,7 @@ static int ses_intf_add(struct class_device *cdev,
 	if (result)
 		goto simple_populate;
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	/* add 1 for trailing '\0' we'll use */
 	buf = kzalloc(len + 1, GFP_KERNEL);
 	if (!buf)
@@ -547,7 +549,8 @@ static int ses_intf_add(struct class_device *cdev,
 		len = (desc_ptr[2] << 8) + desc_ptr[3];
 		/* skip past overall descriptor */
 		desc_ptr += len + 4;
-		addl_desc_ptr = ses_dev->page10 + 8;
+		if (ses_dev->page10)
+			addl_desc_ptr = ses_dev->page10 + 8;
 	}
 	type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
 	components = 0;
@@ -557,6 +560,8 @@ static int ses_intf_add(struct class_device *cdev,
 			struct enclosure_component *ecomp;
 
 			if (desc_ptr) {
+				if (desc_ptr >= buf + page7_len)
+					break;
 				len = (desc_ptr[2] << 8) + desc_ptr[3];
 				desc_ptr += 4;
 				/* Add trailing zero - pushes into
@@ -564,18 +569,20 @@ static int ses_intf_add(struct class_device *cdev,
 				desc_ptr[len] = '\0';
 				name = desc_ptr;
 			}
-			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
-			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
-				continue;
-			ecomp =	enclosure_component_register(edev,
+			if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
+			    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
+
+				ecomp = enclosure_component_register(edev,
 							     components++,
 							     type_ptr[0],
 							     name);
-			if (desc_ptr) {
-				desc_ptr += len;
-				if (!IS_ERR(ecomp))
+
+				if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr)
 					ses_process_descriptor(ecomp,
 							       addl_desc_ptr);
+			}
+			if (desc_ptr) {
+				desc_ptr += len;
 
 				if (addl_desc_ptr)
 					addl_desc_ptr += addl_desc_ptr[1] + 2;
-- 
1.5.3.8

Re: [PATCH] SCSI: fix data corruption caused by ses

[Yinghai Lu]

On Wednesday 13 February 2008 03:25:27 pm James Bottomley wrote:
> On Tue, 2008-02-12 at 23:10 -0800, Yinghai Lu wrote:
> >  			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
> >  			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
> > -				continue;
> > +				goto next;
> > +
> >  			ecomp =	enclosure_component_register(edev,
> >  							     components++,
> >  							     type_ptr[0],
> >  							     name);
> > +
> > +			if (desc_ptr && !IS_ERR(ecomp) && addl_desc_ptr)
> > +				ses_process_descriptor(ecomp,
> > +						       addl_desc_ptr);
> > +		next:
> >  			if (desc_ptr) {
> >  				desc_ptr += len;
> > -				if (!IS_ERR(ecomp))
> > -					ses_process_descriptor(ecomp,
> > -							       addl_desc_ptr);
> >  
> >  				if (addl_desc_ptr)
> >  					addl_desc_ptr += addl_desc_ptr[1] + 2;
> 
> Everything looks fine, thanks, except this piece.
> 
> That 
> 
> if (x)
>      goto next;
> ...
> next:
> 
> Needs to be
> 
> if (!x) {
>    ...
> }
> 

find other problems about sub_enclosure...

will send you updated one.

YH

[PATCH] SCSI: fix data corruption caused by ses v2

[Yinghai Lu]

one system: initrd get courrupted:

RAMDISK: Compressed image found at block 0
RAMDISK: incomplete write (-28 != 2048) 134217728
crc error
VFS: Mounted root (ext2 filesystem).
Freeing unused kernel memory: 388k freed
init_special_inode: bogus i_mode (177777)
Warning: unable to open an initial console.
init_special_inode: bogus i_mode (177777)
init_special_inode: bogus i_mode (177777)
Kernel panic - not syncing: No init found.  Try passing init= option to kernel.

bisected to
commit 9927c68864e9c39cc317b4f559309ba29e642168
Author: James Bottomley <James.Bottomley@HansenPartnership.com>
Date:   Sun Feb 3 15:48:56 2008 -0600

    [SCSI] ses: add new Enclosure ULD

changes:
1. change char to unsigned char to avoid type change later.
2. preserve len for page1
3. need to move desc_ptr even the entry is not enclosure_component_device/raid.
   so keep desc_ptr on right position
4. record page7 len, and double check if desc_ptr out of boundary before touch.
5. fix typo in subenclosure checking: should use hdr_buf instead.

Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

Index: linux-2.6/drivers/scsi/ses.c
===================================================================
--- linux-2.6.orig/drivers/scsi/ses.c
+++ linux-2.6/drivers/scsi/ses.c
@@ -33,9 +33,9 @@
 #include <scsi/scsi_host.h>
 
 struct ses_device {
-	char *page1;
-	char *page2;
-	char *page10;
+	unsigned char *page1;
+	unsigned char *page2;
+	unsigned char *page10;
 	short page1_len;
 	short page2_len;
 	short page10_len;
@@ -67,7 +66,7 @@ static int ses_probe(struct device *dev)
 static int ses_recv_diag(struct scsi_device *sdev, int page_code,
 			 void *buf, int bufflen)
 {
-	char cmd[] = {
+	unsigned char cmd[] = {
 		RECEIVE_DIAGNOSTIC,
 		1,		/* Set PCV bit */
 		page_code,
@@ -85,7 +84,7 @@ static int ses_send_diag(struct scsi_dev
 {
 	u32 result;
 
-	char cmd[] = {
+	unsigned char cmd[] = {
 		SEND_DIAGNOSTIC,
 		0x10,		/* Set PF bit */
 		0,
@@ -104,13 +103,13 @@ static int ses_send_diag(struct scsi_dev
 
 static int ses_set_page2_descriptor(struct enclosure_device *edev,
 				      struct enclosure_component *ecomp,
-				      char *desc)
+				      unsigned char *desc)
 {
 	int i, j, count = 0, descriptor = ecomp->number;
 	struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
 	struct ses_device *ses_dev = edev->scratch;
-	char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
-	char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
 	/* Clear everything */
 	memset(desc_ptr, 0, ses_dev->page2_len - 8);
@@ -133,14 +132,14 @@ static int ses_set_page2_descriptor(stru
 	return ses_send_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 }
 
-static char *ses_get_page2_descriptor(struct enclosure_device *edev,
+static unsigned char *ses_get_page2_descriptor(struct enclosure_device *edev,
 				      struct enclosure_component *ecomp)
 {
 	int i, j, count = 0, descriptor = ecomp->number;
 	struct scsi_device *sdev = to_scsi_device(edev->cdev.dev);
 	struct ses_device *ses_dev = edev->scratch;
-	char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
-	char *desc_ptr = ses_dev->page2 + 8;
+	unsigned char *type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
+	unsigned char *desc_ptr = ses_dev->page2 + 8;
 
 	ses_recv_diag(sdev, 2, ses_dev->page2, ses_dev->page2_len);
 
@@ -160,17 +159,18 @@ static char *ses_get_page2_descriptor(st
 static void ses_get_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->fault = (desc[3] & 0x60) >> 4;
+	if (desc)
+		ecomp->fault = (desc[3] & 0x60) >> 4;
 }
 
 static int ses_set_fault(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			 enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -190,26 +190,28 @@ static int ses_set_fault(struct enclosur
 static void ses_get_status(struct enclosure_device *edev,
 			   struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->status = (desc[0] & 0x0f);
+	if (desc)
+		ecomp->status = (desc[0] & 0x0f);
 }
 
 static void ses_get_locate(struct enclosure_device *edev,
 			   struct enclosure_component *ecomp)
 {
-	char *desc;
+	unsigned char *desc;
 
 	desc = ses_get_page2_descriptor(edev, ecomp);
-	ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
+	if (desc)
+		ecomp->locate = (desc[2] & 0x02) ? 1 : 0;
 }
 
 static int ses_set_locate(struct enclosure_device *edev,
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -229,7 +231,7 @@ static int ses_set_active(struct enclosu
 			  struct enclosure_component *ecomp,
 			  enum enclosure_component_setting val)
 {
-	char desc[4] = {0 };
+	unsigned char desc[4] = {0 };
 
 	switch (val) {
 	case ENCLOSURE_SETTING_DISABLED:
@@ -409,11 +409,11 @@ static int ses_intf_add(struct class_dev
 {
 	struct scsi_device *sdev = to_scsi_device(cdev->dev);
 	struct scsi_device *tmp_sdev;
-	unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr,
-		*addl_desc_ptr;
+	unsigned char *buf = NULL, *hdr_buf, *type_ptr, *desc_ptr = NULL,
+		*addl_desc_ptr = NULL;
 	struct ses_device *ses_dev;
 	u32 result;
-	int i, j, types, len, components = 0;
+	int i, j, types, len, page7_len = 0, components = 0;
 	int err = -ENOMEM;
 	struct enclosure_device *edev;
 	struct ses_component *scomp = NULL;
@@ -447,7 +447,7 @@ static int ses_intf_add(struct class_dev
 		 * traversal routines more complex */
 		sdev_printk(KERN_ERR, sdev,
 			"FIXME driver has no support for subenclosures (%d)\n",
-			buf[1]);
+			hdr_buf[1]);
 		goto err_free;
 	}
 
@@ -461,9 +461,8 @@ static int ses_intf_add(struct class_dev
 		goto recv_failed;
 
 	types = buf[10];
-	len = buf[11];
 
-	type_ptr = buf + 12 + len;
+	type_ptr = buf + 12 + buf[11];
 
 	for (i = 0; i < types; i++, type_ptr += 4) {
 		if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
@@ -494,22 +493,21 @@ static int ses_intf_add(struct class_dev
 	/* The additional information page --- allows us
 	 * to match up the devices */
 	result = ses_recv_diag(sdev, 10, hdr_buf, INIT_ALLOC_SIZE);
-	if (result)
-		goto no_page10;
+	if (!result) {
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
-	buf = kzalloc(len, GFP_KERNEL);
-	if (!buf)
-		goto err_free;
-
-	result = ses_recv_diag(sdev, 10, buf, len);
-	if (result)
-		goto recv_failed;
-	ses_dev->page10 = buf;
-	ses_dev->page10_len = len;
-	buf = NULL;
+		len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+		buf = kzalloc(len, GFP_KERNEL);
+		if (!buf)
+			goto err_free;
+
+		result = ses_recv_diag(sdev, 10, buf, len);
+		if (result)
+			goto recv_failed;
+		ses_dev->page10 = buf;
+		ses_dev->page10_len = len;
+		buf = NULL;
+	}
 
- no_page10:
 	scomp = kzalloc(sizeof(struct ses_component) * components, GFP_KERNEL);
 	if (!scomp)
 		goto err_free;
@@ -530,7 +528,7 @@ static int ses_intf_add(struct class_dev
 	if (result)
 		goto simple_populate;
 
-	len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
+	page7_len = len = (hdr_buf[2] << 8) + hdr_buf[3] + 4;
 	/* add 1 for trailing '\0' we'll use */
 	buf = kzalloc(len + 1, GFP_KERNEL);
 	if (!buf)
@@ -547,7 +545,8 @@ static int ses_intf_add(struct class_dev
 		len = (desc_ptr[2] << 8) + desc_ptr[3];
 		/* skip past overall descriptor */
 		desc_ptr += len + 4;
-		addl_desc_ptr = ses_dev->page10 + 8;
+		if (ses_dev->page10)
+			addl_desc_ptr = ses_dev->page10 + 8;
 	}
 	type_ptr = ses_dev->page1 + 12 + ses_dev->page1[11];
 	components = 0;
@@ -557,6 +556,10 @@ static int ses_intf_add(struct class_dev
 			struct enclosure_component *ecomp;
 
 			if (desc_ptr) {
+				if (desc_ptr >= buf + page7_len) {
+					desc_ptr = NULL;
+					goto noname;
+				}
 				len = (desc_ptr[2] << 8) + desc_ptr[3];
 				desc_ptr += 4;
 				/* Add trailing zero - pushes into
@@ -564,22 +567,25 @@ static int ses_intf_add(struct class_dev
 				desc_ptr[len] = '\0';
 				name = desc_ptr;
 			}
-			if (type_ptr[0] != ENCLOSURE_COMPONENT_DEVICE &&
-			    type_ptr[0] != ENCLOSURE_COMPONENT_ARRAY_DEVICE)
-				continue;
-			ecomp =	enclosure_component_register(edev,
+		noname:
+			if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
+			    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
+
+				ecomp =	enclosure_component_register(edev,
 							     components++,
 							     type_ptr[0],
 							     name);
-			if (desc_ptr) {
-				desc_ptr += len;
-				if (!IS_ERR(ecomp))
+
+				if (!IS_ERR(ecomp) && addl_desc_ptr)
 					ses_process_descriptor(ecomp,
 							       addl_desc_ptr);
-
-				if (addl_desc_ptr)
-					addl_desc_ptr += addl_desc_ptr[1] + 2;
 			}
+			if (desc_ptr)
+				desc_ptr += len;
+
+			if (addl_desc_ptr)
+				addl_desc_ptr += addl_desc_ptr[1] + 2;
+
 		}
 	}
 	kfree(buf);

Re: [PATCH] SCSI: fix data corruption caused by ses v2

[James Bottomley]

On Wed, 2008-02-13 at 16:25 -0800, Yinghai Lu wrote:
> one system: initrd get courrupted:
> 
> RAMDISK: Compressed image found at block 0
> RAMDISK: incomplete write (-28 != 2048) 134217728
> crc error
> VFS: Mounted root (ext2 filesystem).
> Freeing unused kernel memory: 388k freed
> init_special_inode: bogus i_mode (177777)
> Warning: unable to open an initial console.
> init_special_inode: bogus i_mode (177777)
> init_special_inode: bogus i_mode (177777)
> Kernel panic - not syncing: No init found.  Try passing init= option to kernel.
> 
> bisected to
> commit 9927c68864e9c39cc317b4f559309ba29e642168
> Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> Date:   Sun Feb 3 15:48:56 2008 -0600
> 
>     [SCSI] ses: add new Enclosure ULD
> 
> changes:
> 1. change char to unsigned char to avoid type change later.
> 2. preserve len for page1
> 3. need to move desc_ptr even the entry is not enclosure_component_device/raid.
>    so keep desc_ptr on right position
> 4. record page7 len, and double check if desc_ptr out of boundary before touch.
> 5. fix typo in subenclosure checking: should use hdr_buf instead.
> 
> Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>

OK, I added this with a fixup to eliminate the spurious goto

Thanks,

James

---

diff --git a/drivers/scsi/ses.c b/drivers/scsi/ses.c
index cbba012..a6d9669 100644
--- a/drivers/scsi/ses.c
+++ b/drivers/scsi/ses.c
@@ -561,16 +561,15 @@ static int ses_intf_add(struct class_device *cdev,
 			if (desc_ptr) {
 				if (desc_ptr >= buf + page7_len) {
 					desc_ptr = NULL;
-					goto noname;
+				} else {
+					len = (desc_ptr[2] << 8) + desc_ptr[3];
+					desc_ptr += 4;
+					/* Add trailing zero - pushes into
+					 * reserved space */
+					desc_ptr[len] = '\0';
+					name = desc_ptr;
 				}
-				len = (desc_ptr[2] << 8) + desc_ptr[3];
-				desc_ptr += 4;
-				/* Add trailing zero - pushes into
-				 * reserved space */
-				desc_ptr[len] = '\0';
-				name = desc_ptr;
 			}
-		noname:
 			if (type_ptr[0] == ENCLOSURE_COMPONENT_DEVICE ||
 			    type_ptr[0] == ENCLOSURE_COMPONENT_ARRAY_DEVICE) {
 

Re: [PATCH] SCSI: fix data corruption caused by ses v2

[Yinghai Lu]

On Friday 15 February 2008 07:53:06 am James Bottomley wrote:
> On Wed, 2008-02-13 at 16:25 -0800, Yinghai Lu wrote:
> > one system: initrd get courrupted:
> > 
> > RAMDISK: Compressed image found at block 0
> > RAMDISK: incomplete write (-28 != 2048) 134217728
> > crc error
> > VFS: Mounted root (ext2 filesystem).
> > Freeing unused kernel memory: 388k freed
> > init_special_inode: bogus i_mode (177777)
> > Warning: unable to open an initial console.
> > init_special_inode: bogus i_mode (177777)
> > init_special_inode: bogus i_mode (177777)
> > Kernel panic - not syncing: No init found.  Try passing init= option to kernel.
> > 
> > bisected to
> > commit 9927c68864e9c39cc317b4f559309ba29e642168
> > Author: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Date:   Sun Feb 3 15:48:56 2008 -0600
> > 
> >     [SCSI] ses: add new Enclosure ULD
> > 
> > changes:
> > 1. change char to unsigned char to avoid type change later.
> > 2. preserve len for page1
> > 3. need to move desc_ptr even the entry is not enclosure_component_device/raid.
> >    so keep desc_ptr on right position
> > 4. record page7 len, and double check if desc_ptr out of boundary before touch.
> > 5. fix typo in subenclosure checking: should use hdr_buf instead.
> > 
> > Signed-off-by: Yinghai Lu <yinghai.lu@sun.com>
> 
> OK, I added this with a fixup to eliminate the spurious goto
> 

good