2003-11-18 02:48:36

by Amit Patel

[permalink] [raw]
Subject: scsi_report_lun_scan bug?

Hi,

I am using 2.6-test9-mm3. I noticed while doing
scsi_report_lun_scan(scsi_scan.c:891) the data
returned is assigned(scsi_scan.c:993) to signed char
array which causes the reported number of luns to be
huge while calculating num_luns to scan. Is there any
particular reason to be data is signed or just a bug?

I changed it to unsigned char and it seems to work
fine. I have attached a diff of scsi_scan.c. Let me
know if I am missing something.

Thanks
Amit

[root@Host200-w2k root]# diff
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
902c902
< char *data;
---
> unsigned char *data;
993c993
< data = (char *) lun_data->scsi_lun;
---
> data = (unsigned char *) lun_data->scsi_lun;
[root@Host200-w2k root]#


__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree


2003-11-18 02:54:47

by Matthew Wilcox

[permalink] [raw]
Subject: Re: scsi_report_lun_scan bug?

On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel wrote:
> [root@Host200-w2k root]# diff
> /cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> /cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> 902c902
> < char *data;
> ---
> > unsigned char *data;

Hi Amit. Can you send diffs in unified format in the future, ie diff -u
Thanks.

--
"It's not Hollywood. War is real, war is primarily not about defeat or
victory, it is about death. I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

2003-11-18 03:10:29

by Amit Patel

[permalink] [raw]
Subject: Re: scsi_report_lun_scan bug?

Oops sorry did not know about that...

Here it is.

[root@Host200-w2k root]# diff -u
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
---
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
2003-11-04 11:52:30.000000000 -0800
+++
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
2003-11-17 18:25:30.534512992 -0800
@@ -899,7 +899,7 @@
unsigned int retries;
struct scsi_lun *lunp, *lun_data;
struct scsi_request *sreq;
- char *data;
+ unsigned char *data;

/*
* Only support SCSI-3 and up devices.
@@ -990,7 +990,7 @@
/*
* Get the length from the first four bytes of
lun_data.
*/
- data = (char *) lun_data->scsi_lun;
+ data = (unsigned char *) lun_data->scsi_lun;
length = ((data[0] << 24) | (data[1] << 16) |
(data[2] << 8) | (data[3] << 0));

[root@Host200-w2k root]#
[root@Host200-w2k root]#

--- Matthew Wilcox <[email protected]> wrote:
> On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel
> wrote:
> > [root@Host200-w2k root]# diff
> >
>
/cdrive/mm1/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> >
>
/cdrive/mm3/linux-2.6.0-test9/drivers/scsi/scsi_scan.c
> > 902c902
> > < char *data;
> > ---
> > > unsigned char *data;
>
> Hi Amit. Can you send diffs in unified format in
> the future, ie diff -u
> Thanks.
>
> --
> "It's not Hollywood. War is real, war is primarily
> not about defeat or
> victory, it is about death. I've seen thousands and
> thousands of dead bodies.
> Do you think I want to have an academic debate on
> this subject?" -- Robert Fisk


__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree

2003-11-18 05:53:01

by Patrick Mansfield

[permalink] [raw]
Subject: Re: scsi_report_lun_scan bug?

On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel wrote:
> Hi,
>
> I am using 2.6-test9-mm3. I noticed while doing
> scsi_report_lun_scan(scsi_scan.c:891) the data
> returned is assigned(scsi_scan.c:993) to signed char
> array which causes the reported number of luns to be
> huge while calculating num_luns to scan. Is there any
> particular reason to be data is signed or just a bug?
>
> I changed it to unsigned char and it seems to work
> fine. I have attached a diff of scsi_scan.c. Let me
> know if I am missing something.

I don't see why making it signed or unsigned would make any difference.

What values did you see before and after your patch?

It should really be a u8, since it is a pointer to an array of bytes.

(And all the scsi_cmd[]'s should be u8.)

-- Patrick Mansfield

2003-11-18 06:55:24

by Andrew Morton

[permalink] [raw]
Subject: Re: scsi_report_lun_scan bug?

Patrick Mansfield <[email protected]> wrote:
>
> On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit Patel wrote:
> > Hi,
> >
> > I am using 2.6-test9-mm3. I noticed while doing
> > scsi_report_lun_scan(scsi_scan.c:891) the data
> > returned is assigned(scsi_scan.c:993) to signed char
> > array which causes the reported number of luns to be
> > huge while calculating num_luns to scan. Is there any
> > particular reason to be data is signed or just a bug?
> >
> > I changed it to unsigned char and it seems to work
> > fine. I have attached a diff of scsi_scan.c. Let me
> > know if I am missing something.
>
> I don't see why making it signed or unsigned would make any difference.


length = ((data[0] << 24) | (data[1] << 16) |
(data[2] << 8) | (data[3] << 0));

If data[3] is 0xff, this expression will always evaluate to
0xffffffff. etcetera.

> It should really be a u8, since it is a pointer to an array of bytes.
>
> (And all the scsi_cmd[]'s should be u8.)

Yup.

diff -puN drivers/scsi/scsi_scan.c~scsi_report_lun-fix drivers/scsi/scsi_scan.c
--- 25/drivers/scsi/scsi_scan.c~scsi_report_lun-fix 2003-11-17 20:22:49.000000000 -0800
+++ 25-akpm/drivers/scsi/scsi_scan.c 2003-11-17 20:22:49.000000000 -0800
@@ -899,7 +899,7 @@ static int scsi_report_lun_scan(struct s
unsigned int retries;
struct scsi_lun *lunp, *lun_data;
struct scsi_request *sreq;
- char *data;
+ u8 *data;

/*
* Only support SCSI-3 and up devices.
@@ -990,7 +990,7 @@ static int scsi_report_lun_scan(struct s
/*
* Get the length from the first four bytes of lun_data.
*/
- data = (char *) lun_data->scsi_lun;
+ data = (u8 *) lun_data->scsi_lun;
length = ((data[0] << 24) | (data[1] << 16) |
(data[2] << 8) | (data[3] << 0));


_

2003-11-18 07:42:58

by Amit Patel

[permalink] [raw]
Subject: Re: scsi_report_lun_scan bug?

Patrick,

In my case, I have 16 luns for which report lun
command byte 3 has value 0x80. While doing the length
calculation
length = ((data[0] << 24) | (data[1] << 16) |
(data[2] << 8) | (data[3] << 0));

It calucates length as 0xffffff80 and actual number of
luns is calculated as 536870896. I just write small
program with signed and unsigned char array and it
does evaluate as 0xffffff80 as Andrew mentioned.

Thanks
Amit


#include<stdio.h>
main()
{
char x[10]={0x00,0x00,0x00,0x80};
char *p;
unsigned char *q;

int len;
int ulen;

p=(char *)x;
q=(unsigned char *) x;
len=((p[0] << 24) | (p[1] << 16) | (p[2] << 8)
| (p[3] <<0));
ulen=((q[0] << 24) |(q[1] << 16) | (q[2] << 8)
| (q[3] <<0));
printf("len= %d len=%x\n",len,len);
printf("ulen= %d ulen=%x\n",ulen,ulen);
}





--- Andrew Morton <[email protected]> wrote:
> Patrick Mansfield <[email protected]> wrote:
> >
> > On Mon, Nov 17, 2003 at 06:48:33PM -0800, Amit
> Patel wrote:
> > > Hi,
> > >
> > > I am using 2.6-test9-mm3. I noticed while doing
> > > scsi_report_lun_scan(scsi_scan.c:891) the data
> > > returned is assigned(scsi_scan.c:993) to signed
> char
> > > array which causes the reported number of luns
> to be
> > > huge while calculating num_luns to scan. Is
> there any
> > > particular reason to be data is signed or just a
> bug?
> > >
> > > I changed it to unsigned char and it seems to
> work
> > > fine. I have attached a diff of scsi_scan.c. Let
> me
> > > know if I am missing something.
> >
> > I don't see why making it signed or unsigned would
> make any difference.
>
>
> length = ((data[0] << 24) | (data[1] << 16) |
> (data[2] << 8) | (data[3] << 0));
>
> If data[3] is 0xff, this expression will always
> evaluate to
> 0xffffffff. etcetera.
>
> > It should really be a u8, since it is a pointer to
> an array of bytes.
> >
> > (And all the scsi_cmd[]'s should be u8.)
>
> Yup.
>
> diff -puN
> drivers/scsi/scsi_scan.c~scsi_report_lun-fix
> drivers/scsi/scsi_scan.c
> --- 25/drivers/scsi/scsi_scan.c~scsi_report_lun-fix
> 2003-11-17 20:22:49.000000000 -0800
> +++ 25-akpm/drivers/scsi/scsi_scan.c 2003-11-17
> 20:22:49.000000000 -0800
> @@ -899,7 +899,7 @@ static int
> scsi_report_lun_scan(struct s
> unsigned int retries;
> struct scsi_lun *lunp, *lun_data;
> struct scsi_request *sreq;
> - char *data;
> + u8 *data;
>
> /*
> * Only support SCSI-3 and up devices.
> @@ -990,7 +990,7 @@ static int
> scsi_report_lun_scan(struct s
> /*
> * Get the length from the first four bytes of
> lun_data.
> */
> - data = (char *) lun_data->scsi_lun;
> + data = (u8 *) lun_data->scsi_lun;
> length = ((data[0] << 24) | (data[1] << 16) |
> (data[2] << 8) | (data[3] << 0));
>
>
> _
>


__________________________________
Do you Yahoo!?
Protect your identity with Yahoo! Mail AddressGuard
http://antispam.yahoo.com/whatsnewfree