[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 <[email protected]>
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);
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
On Feb 9, 2008 7:00 AM, James Bottomley
<[email protected]> 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
fix leaking with scomp leaking when failing.
also remove one extra space.
Signed-off-by: Yinghai Lu <[email protected]>
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);
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 <[email protected]>
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 <[email protected]>
Signed-off-by: James Bottomley <[email protected]>
---
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
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
please check it...
---
From: Yinghai Lu <[email protected]>
[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 <[email protected]>
Signed-off-by: James Bottomley <[email protected]>
---
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);
On Sun, 2008-02-10 at 23:25 -0800, Yinghai Lu wrote:
> please check it...
This one looks perfect, thanks!
James
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
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.
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 <[email protected]>
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 <[email protected]>
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;
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Signed-off-by: James Bottomley <[email protected]>
---
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
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
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 <[email protected]>
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 <[email protected]>
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);
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 <[email protected]>
> 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 <[email protected]>
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) {
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 <[email protected]>
> > 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 <[email protected]>
>
> OK, I added this with a fixup to eliminate the spurious goto
>
good