Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

[-- Attachment #1: Type: text/plain, Size: 2584 bytes --]

Hello,
There is a problem in udev-165/extras/ata_id/ata_id.c resulting in 
random early boot kernel panics.  As it stands, udev-165 is not usable 
because the boot panics occur to frequently.  The systems are GNU/Linux 
i686 with linux-2.6.36.2 and linux-2.6.37, gcc-4.5.1, and glibc-2.12.1.

By "random," I mean that the panics don't occur on every boot, or on 
every pc I've tried. The panics do not occur for udev-164.  The problem 
code in udev-165 is in the "disk_identify_packet_device_command" 
function (see line 270) where an sg3 'ATA Pass-Through' command is 
issued to a cd/dvd device:

253         ret = ioctl(fd, SG_IO, &io_v4);
254         if (ret != 0) {
255                 /* could be that the driver doesn't do version 4, 
try version 3 */
256                 if (errno == EINVAL) {
257                         struct sg_io_hdr io_hdr;
258
259                         memset(&io_hdr, 0, sizeof(struct sg_io_hdr));
260                         io_hdr.interface_id = 'S';
261                         io_hdr.cmdp = (unsigned char*) cdb;
262                         io_hdr.cmd_len = sizeof (cdb);
263                         io_hdr.dxferp = buf;
264                         io_hdr.dxfer_len = buf_len;
265                         io_hdr.sbp = sense;
266                         io_hdr.mx_sb_len = sizeof (sense);
267                         io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
268                         io_hdr.timeout = COMMAND_TIMEOUT_MSEC;
269
270                         ret = ioctl(fd, SG_IO, &io_hdr); <-- random 
kernel panics result from this call ***
271                         if (ret != 0)
272                                 goto out;
273                 } else {
274                         goto out;
275                 }
276         }
277
278         if (!(sense[0] == 0x72 && desc[0] == 0x9 && desc[1] == 0x0c)) {
279                 errno = EIO;
280                 ret = -1;
281                 goto out;
282         }
283
284  out:
285         return ret;
286 }

While by no means a fix of course, simply commenting out line 270 above 
appears to side-step the issue. Also, suspecting a buffer alignment 
issue, I built udev-165 with the attached patch, and thus far, no kernel 
panics have occurred.

I realize that allocating the sense and response buffers in this manner 
shouldn't be necessary or even a good idea; my hope is that it might 
shed some light on the cause of the panics. I suspect that this issue is 
likely a kernel problem (not a udev problem),  but do you have any 
thoughts/ideas on how to track down the problem?

John

[-- Attachment #2: 02-udev-165-ata-pass-through-memalign.patch --]
[-- Type: text/plain, Size: 1378 bytes --]

--- udev-165.old/extras/ata_id/ata_id.c	2010-11-09 19:30:53.000000000 -0500
+++ udev-165.new/extras/ata_id/ata_id.c	2011-01-05 03:11:06.629999372 -0500
@@ -208,7 +208,9 @@
 {
 	struct sg_io_v4 io_v4;
 	uint8_t cdb[16];
-	uint8_t sense[32];
+	// Allocate page-aligned memory, rather than using an array (uint8_t sense[32]); jps
+	uint8_t* sense;
+	posix_memalign((void**)&sense, sysconf(_SC_PAGESIZE), 32);
 	uint8_t *desc = sense+8;
 	int ret;
 
@@ -251,7 +253,7 @@
 	io_v4.timeout = COMMAND_TIMEOUT_MSEC;
 
 	ret = ioctl(fd, SG_IO, &io_v4);
-	if (ret != 0) {
+	if (ret < 0) {
 		/* could be that the driver doesn't do version 4, try version 3 */
 		if (errno == EINVAL) {
 			struct sg_io_hdr io_hdr;
@@ -268,7 +270,7 @@
 			io_hdr.timeout = COMMAND_TIMEOUT_MSEC;
 
 			ret = ioctl(fd, SG_IO, &io_hdr);
-			if (ret != 0)
+			if (ret < 0)
 				goto out;
 		} else {
 			goto out;
@@ -282,6 +284,7 @@
 	}
 
  out:
+	free(sense);
 	return ret;
 }
 
@@ -447,7 +450,9 @@
 {
 	struct udev *udev;
 	struct hd_driveid id;
-	uint8_t identify[512];
+	// Allocate page-aligned memory, rather than using an array (uint8_t identify[512]); jps
+	uint8_t* identify;
+	posix_memalign((void**)&identify, sysconf(_SC_PAGESIZE), 512);
 	char model[41];
 	char model_enc[256];
 	char serial[21];
@@ -709,5 +714,6 @@
 exit:
 	udev_unref(udev);
 	udev_log_close();
+	free(identify);
 	return rc;
 }

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Greg KH]

On Thu, Jan 06, 2011 at 05:02:30PM -0500, John Stanley wrote:
> Hello,
> There is a problem in udev-165/extras/ata_id/ata_id.c resulting in
> random early boot kernel panics.  As it stands, udev-165 is not
> usable because the boot panics occur to frequently.  The systems are
> GNU/Linux i686 with linux-2.6.36.2 and linux-2.6.37, gcc-4.5.1, and
> glibc-2.12.1.

What is the kernel oops message?  That should be fixed first, no
userspace code should be able to crash the kernel.

thanks,

greg k-h

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Robby Workman]

On Thu, 6 Jan 2011 14:29:27 -0800
Greg KH <greg@kroah.com> wrote:

> On Thu, Jan 06, 2011 at 05:02:30PM -0500, John Stanley wrote:
> > Hello,
> > There is a problem in udev-165/extras/ata_id/ata_id.c resulting in
> > random early boot kernel panics.  As it stands, udev-165 is not
> > usable because the boot panics occur to frequently.  The systems are
> > GNU/Linux i686 with linux-2.6.36.2 and linux-2.6.37, gcc-4.5.1, and
> > glibc-2.12.1.
> 
> What is the kernel oops message?  That should be fixed first, no
> userspace code should be able to crash the kernel.


Hi Greg,

First, sorry for not posting something about this sooner - I'd
pinged Kay on IRC about it, and I *promise* I had planned to
forward it to the scsi/ati guys, but work has been hell this 
week.  Anyway, here's the initial report we got about it, along
with a lot of debugging by other folks (including the OP, who
I think is 'resonance' in that thread): 
http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/

-RW

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Ozan Çağlayan]

Cuma 07 Ocak 2011 günü (saat 00:29:27) Greg KH şunları yazmıştı:
> On Thu, Jan 06, 2011 at 05:02:30PM -0500, John Stanley wrote:
> > Hello,
> > There is a problem in udev-165/extras/ata_id/ata_id.c resulting in
> > random early boot kernel panics.  As it stands, udev-165 is not
> > usable because the boot panics occur to frequently.  The systems are
> > GNU/Linux i686 with linux-2.6.36.2 and linux-2.6.37, gcc-4.5.1, and
> > glibc-2.12.1.
> 
> What is the kernel oops message?  That should be fixed first, no
> userspace code should be able to crash the kernel.

http://www.edgrochowski.com/photos-lq/kernel-panic-12-28-10.png

I never thought that it was related to udev but one of our users posted that exact
oops screenshot to our bugzilla too. 

Udev is 165 on this installation. I'll ask him if he can consistently repeat the problem.
He thought that the oops happens when the installer scans the disks.

---
Ozan Çağlayan
TUBITAK/UEKAE - Pardus Linux
http://www.pardus.org.tr/eng

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Hannes Reinecke]

On 01/07/2011 03:13 AM, Robby Workman wrote:
> On Thu, 6 Jan 2011 14:29:27 -0800
> Greg KH <greg@kroah.com> wrote:
> 
>> On Thu, Jan 06, 2011 at 05:02:30PM -0500, John Stanley wrote:
>>> Hello,
>>> There is a problem in udev-165/extras/ata_id/ata_id.c resulting in
>>> random early boot kernel panics.  As it stands, udev-165 is not
>>> usable because the boot panics occur to frequently.  The systems are
>>> GNU/Linux i686 with linux-2.6.36.2 and linux-2.6.37, gcc-4.5.1, and
>>> glibc-2.12.1.
>>
>> What is the kernel oops message?  That should be fixed first, no
>> userspace code should be able to crash the kernel.
> 
> 
> Hi Greg,
> 
> First, sorry for not posting something about this sooner - I'd
> pinged Kay on IRC about it, and I *promise* I had planned to
> forward it to the scsi/ati guys, but work has been hell this 
> week.  Anyway, here's the initial report we got about it, along
> with a lot of debugging by other folks (including the OP, who
> I think is 'resonance' in that thread): 
> http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/
> 
It's all Tejun's fault.
kernel crashing in ata_sff_data_xfer / ioread32 ...
Looks like we're trying a read to a page which wasn't
mapped/allocated properly.

And yes, it definitely should be fixed in the kernel first.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Ozan Çağlayan]

Pazartesi 10 Ocak 2011 günü (saat 10:10:12) Hannes Reinecke şunları yazmıştı:
> On 01/07/2011 03:13 AM, Robby Workman wrote:
> > On Thu, 6 Jan 2011 14:29:27 -0800
> > Greg KH <greg@kroah.com> wrote:
> > 
> >> On Thu, Jan 06, 2011 at 05:02:30PM -0500, John Stanley wrote:
> >>> Hello,
> >>> There is a problem in udev-165/extras/ata_id/ata_id.c resulting in
> >>> random early boot kernel panics.  As it stands, udev-165 is not
> >>> usable because the boot panics occur to frequently.  The systems are
> >>> GNU/Linux i686 with linux-2.6.36.2 and linux-2.6.37, gcc-4.5.1, and
> >>> glibc-2.12.1.
> >>
> >> What is the kernel oops message?  That should be fixed first, no
> >> userspace code should be able to crash the kernel.
> > 
> > 
> > Hi Greg,
> > 
> > First, sorry for not posting something about this sooner - I'd
> > pinged Kay on IRC about it, and I *promise* I had planned to
> > forward it to the scsi/ati guys, but work has been hell this 
> > week.  Anyway, here's the initial report we got about it, along
> > with a lot of debugging by other folks (including the OP, who
> > I think is 'resonance' in that thread): 
> > http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/
> > 
> It's all Tejun's fault.
> kernel crashing in ata_sff_data_xfer / ioread32 ...
> Looks like we're trying a read to a page which wasn't
> mapped/allocated properly.

I've just had another report about random crashes (udev-165 on 2.6.37) which
crash in ioread16_rep:

http://img5.imagebanana.com/img/1ub7ghcy/DSCF5552.JPG

---
Ozan Çağlayan
TUBITAK/UEKAE - Pardus Linux
http://www.pardus.org.tr/eng

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Tejun Heo]

Hello,

On Mon, Jan 10, 2011 at 09:10:12AM +0100, Hannes Reinecke wrote:
> > First, sorry for not posting something about this sooner - I'd
> > pinged Kay on IRC about it, and I *promise* I had planned to
> > forward it to the scsi/ati guys, but work has been hell this 
> > week.  Anyway, here's the initial report we got about it, along
> > with a lot of debugging by other folks (including the OP, who
> > I think is 'resonance' in that thread): 
> > http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/
> > 
> It's all Tejun's fault.

Gees, Hannes.  That's very kind of you. :-P

> kernel crashing in ata_sff_data_xfer / ioread32 ...
> Looks like we're trying a read to a page which wasn't
> mapped/allocated properly.
> 
> And yes, it definitely should be fixed in the kernel first.

Yeah, definitely.  It isn't clear from the thread.

* Is it a regression?

* Can this be triggered by simply running ata_id or does it need any
  other condition to trigger?

I don't recall any related change in the area, at least in libata, so
it's a bit surprising.  If it's a regression, I think it's more likely
to be something between userland and libata.  The user buffer mapping
code for sg commands is quite scary after all.

Thanks.

-- 
tejun

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

* Can this be triggered by simply running ata_id or does it need any
   other condition to trigger?

Yes, once the system has booted, issuing
       lib/udev/ata_id-unpatched /dev/dvd
produces the same kernel panic nearly every time. Also, as I mentioned 
earlier, the sg__sat_identify utility  from sg3-utils-1.30 does the same 
on 1-3 attempts out of 10; i.e., issuing
      sg__sat_identify -vv -p  /dev/dvd


On 01/11/2011 08:25 AM, Tejun Heo wrote:
> Hello,
>
> On Mon, Jan 10, 2011 at 09:10:12AM +0100, Hannes Reinecke wrote:
>>> First, sorry for not posting something about this sooner - I'd
>>> pinged Kay on IRC about it, and I *promise* I had planned to
>>> forward it to the scsi/ati guys, but work has been hell this
>>> week.  Anyway, here's the initial report we got about it, along
>>> with a lot of debugging by other folks (including the OP, who
>>> I think is 'resonance' in that thread):
>>> http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/
>>>
>> It's all Tejun's fault.
> Gees, Hannes.  That's very kind of you. :-P
>
>> kernel crashing in ata_sff_data_xfer / ioread32 ...
>> Looks like we're trying a read to a page which wasn't
>> mapped/allocated properly.
>>
>> And yes, it definitely should be fixed in the kernel first.
> Yeah, definitely.  It isn't clear from the thread.
>
> * Is it a regression?
>
> * Can this be triggered by simply running ata_id or does it need any
>    other condition to trigger?
>
> I don't recall any related change in the area, at least in libata, so
> it's a bit surprising.  If it's a regression, I think it's more likely
> to be something between userland and libata.  The user buffer mapping
> code for sg commands is quite scary after all.
>
> Thanks.
>

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

On looking at this issue a bit further (please note (and be foregiving), 
I know very little about the kernel/SCSI implementation), it looks more 
and more to be very similar to another kernel-panic issue posted some 6 
months ago (http://www.spinics.net/lists/linux-scsi/msg44077.html).

The kernel-panic, which occurs at boot-time in udev/ata_id.c when 
issuing an ioctl SG_IO sg3 SCSI ATA Pass-through Identify command, 
appears to arise from DMA'ing into an incorrectly aligned user data 
buffer pointed to by sg_io_hdr.dxferp .

My guess is that in the past, use of sg3 would not involve DMA by 
default, but now, with libata ATA Pass-Through commands, it does (I also 
may be totally wrong about that, just a thought). I recall documentaion 
somewhere which emphasized that if direct I/O (DMA) is to used in sg, 
one should page-align the SCSI response data buffer.. With sg using 
indirect I/O this wouldn't be necessary, of course, but perhaps now with 
libata, it is. Just guessing here.

If I patch ata_id.c to use a page-aligned *sg_io_hdr.dxferp, the 
kernel-panic goes away.

The same kernel-panic (same traceback) also occurs in sg__sat_identify.c 
(sg3-utils-1.30) once the system is running, issuing, eg,

   sg__sat_identify -vv -p  /dev/dvd

Patching sg__sat_identify.c in the same way eliminates the kernel-panic 
here as well. The code in sg__sat_identify.c and ata_id.c implementing 
the sg3 SCSI ATA Pass-through Identify command is more or less the same.

Here's the relevant (vanilla linux-2.6.37) kernel code path, beginning in
drivers/scsi/sg.c (@NNN = at line number):

   sg_ioctl            @772  in drivers/scsi/sg.c
   sg_new_write        @801  in drivers/scsi/sg.c
   sg_common_write     @708  in drivers/scsi/sg.c
   sg_start_req        @735  in drivers/scsi/sg.c
   blk_rq_map_user     @1707 in drivers/scsi/sg.c
   __blk_rq_map_user   @142  in block/blk-map.c
   blk_rq_aligned      @57   in block/blk-map.c
   queue_dma_alignment @1060 in include/linux/blkdev.h

include/linux/blkdev.h
1057 static inline int blk_rq_aligned(struct request_queue *q, unsigned 
long addr,
1058                                  unsigned int len)
1059 {
1060         unsigned int alignment = queue_dma_alignment(q) | 
q->dma_pad_mask;
1061         return !(addr & alignment) && !(len & alignment);
1062 }

1052 static inline int queue_dma_alignment(struct request_queue *q)
1053 {
1054         return q ? q->dma_alignment : 511;
1055 }

In drivers/scsi/scsi_lib.c (request_queue @1609) we get a default 2-byte 
alignment mask (possibly overridden later by the kernel or underlying 
driver I assume), which is then used in queue_dma_alignment @1054 in 
include/linux/blkdev.h

So, for the current drive(s) at hand, as noted in 
http://www.spinics.net/lists/linux-scsi/msg44077.html),
if the driver (and/or kernel) does not override the default 2-byte 
alignment mask, and such an alignment
is inappropriate, we may have problems when DMA-ing into the specified 
user data buffer pointed to
by sg_io_hdr.dxferp .

In conclusion, then, it would seem that some cd drive/controllers either 
incorrectly specify (or don't specify) DMA buffer alignment 
requirements, or that the kernel uses inappropriate alignments in some 
cases on its own.

I'm not sure where or how to fix this in the kernel, aside from using a 
default page-alignemet in drivers/scsi/scsi_lib.c (rather than 2-byte) 
(line 1648); this, however, is probably way too 'blunt' and may impact 
other drivers and/or system performance ...:

drivers/scsi/scsi_lib.c
1609 struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
1610                                          request_fn_proc *request_fn)
1611 {
.
.
1643         /*
1644          * set a reasonable default alignment on word boundaries: the
1645          * host and device may alter it using
1646          * blk_queue_update_dma_alignment() later.
1647          */
1648         blk_queue_dma_alignment(q, 0x03); <--- Use PAGESIZE-1, 
rather than 0x03 ?
.
.

Any thoughts ?

thanks much for everyone's time,
John



On 01/11/2011 08:25 AM, Tejun Heo wrote:
> Hello,
>
> On Mon, Jan 10, 2011 at 09:10:12AM +0100, Hannes Reinecke wrote:
>>> First, sorry for not posting something about this sooner - I'd
>>> pinged Kay on IRC about it, and I *promise* I had planned to
>>> forward it to the scsi/ati guys, but work has been hell this
>>> week.  Anyway, here's the initial report we got about it, along
>>> with a lot of debugging by other folks (including the OP, who
>>> I think is 'resonance' in that thread):
>>> http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/
>>>
>> It's all Tejun's fault.
> Gees, Hannes.  That's very kind of you. :-P
>
>> kernel crashing in ata_sff_data_xfer / ioread32 ...
>> Looks like we're trying a read to a page which wasn't
>> mapped/allocated properly.
>>
>> And yes, it definitely should be fixed in the kernel first.
> Yeah, definitely.  It isn't clear from the thread.
>
> * Is it a regression?
>
> * Can this be triggered by simply running ata_id or does it need any
>    other condition to trigger?
>
> I don't recall any related change in the area, at least in libata, so
> it's a bit surprising.  If it's a regression, I think it's more likely
> to be something between userland and libata.  The user buffer mapping
> code for sg commands is quite scary after all.
>
> Thanks.
>

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

Some additional info: Several weeks ago, for one laptop which has no 
cd/dvd drives on which I installed udev-165, I got a similar 
kernel-panic (udevd tainted) seveal times, so the issue appears to not 
necessarily be tightly coupled to cd drives.
John


On 01/11/2011 08:25 AM, Tejun Heo wrote:
> Hello,
>
> On Mon, Jan 10, 2011 at 09:10:12AM +0100, Hannes Reinecke wrote:
>>> First, sorry for not posting something about this sooner - I'd
>>> pinged Kay on IRC about it, and I *promise* I had planned to
>>> forward it to the scsi/ati guys, but work has been hell this
>>> week.  Anyway, here's the initial report we got about it, along
>>> with a lot of debugging by other folks (including the OP, who
>>> I think is 'resonance' in that thread):
>>> http://www.linuxquestions.org/questions/slackware-14/current-randomly-timed-kernel-oops-on-bootup-of-two-test-boxen-852843/
>>>
>> It's all Tejun's fault.
> Gees, Hannes.  That's very kind of you. :-P
>
>> kernel crashing in ata_sff_data_xfer / ioread32 ...
>> Looks like we're trying a read to a page which wasn't
>> mapped/allocated properly.
>>
>> And yes, it definitely should be fixed in the kernel first.
> Yeah, definitely.  It isn't clear from the thread.
>
> * Is it a regression?
>
> * Can this be triggered by simply running ata_id or does it need any
>    other condition to trigger?
>
> I don't recall any related change in the area, at least in libata, so
> it's a bit surprising.  If it's a regression, I think it's more likely
> to be something between userland and libata.  The user buffer mapping
> code for sg commands is quite scary after all.
>
> Thanks.
>

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Tejun Heo]

Hello,

On Sun, Jan 16, 2011 at 11:03:06PM -0500, John Stanley wrote:
> The kernel-panic, which occurs at boot-time in udev/ata_id.c when
> issuing an ioctl SG_IO sg3 SCSI ATA Pass-through Identify command,
> appears to arise from DMA'ing into an incorrectly aligned user data
> buffer pointed to by sg_io_hdr.dxferp .

The problem is that nobody is DMA'ing in this case.  The driver in
question is ata_piix and the IO path taken is an actual PIO where the
CPU reads from the IO space and writes to the memory itself.

> My guess is that in the past, use of sg3 would not involve DMA by
> default, but now, with libata ATA Pass-Through commands, it does (I
> also may be totally wrong about that, just a thought).

No DMA in progress here.  The only (somewhat) recent related change
would be libata PIO path now using 32bit IO commands when supported by
the controller, but I fail to see how that would trigger this type of
failures.

> I recall documentaion somewhere which emphasized that if direct I/O
> (DMA) is to used in sg, one should page-align the SCSI response data
> buffer..  With sg using indirect I/O this wouldn't be necessary, of
> course, but perhaps now with libata, it is. Just guessing here.

If the buffer is not aligned, the kernel would just create a bounce
buffer and bounce the data, so it shouldn't be a problem either.  It
looks like we have an obscure bug in buffer mapping code for SG_IO.

I tried several things but can't reproduce the problem here.  Can you
please try the attached minimal test case?  It issues IDENTIFY_PACKET
and you can specify the alignment offset.  By default the buffer would
be 512byte aligned but you can offset it.  ie. specifying 1 would make
the buffer misaligned by 1 byte and so on.

Can you please see whether the problem can be reliably triggered with
it?  Also, please,

* Attach full kernel log (including boot messages) and the program
  output after triggering the problem.

* Make sure the kernel is built with debug info and frame pointer.

* Please reverse map the reported oops address to the source line.

Thanks.

-- 
tejun

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 58 bytes --]

And, of course, forgot to attach.  Here it is.

-- 
tejun

[-- Attachment #2: test-identify-packet.c --]
[-- Type: text/x-c++src, Size: 3283 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/ioctl.h>
#include <scsi/sg.h>
#include <scsi/scsi_ioctl.h>
#include <linux/bsg.h>

#define COMMAND_TIMEOUT_MSEC (30 * 1000)

static int disk_identify_packet_device_command(int	  fd,
					       void	 *buf,
					       size_t	  buf_len)
{
	struct sg_io_v4 io_v4;
	uint8_t cdb[16];
	uint8_t sense[32];
	uint8_t *desc = sense+8;
	int ret;

	/*
	 * ATA Pass-Through 16 byte command, as described in
	 *
	 *  T10 04-262r8 ATA Command Pass-Through
	 *
	 * from http://www.t10.org/ftp/t10/document.04/04-262r8.pdf
	 */
	memset(cdb, 0, sizeof(cdb));
	cdb[0] = 0x85;			/* OPERATION CODE: 16 byte pass through */
	cdb[1] = 4 << 1;		/* PROTOCOL: PIO Data-in */
	cdb[2] = 0x2e;			/* OFF_LINE=0, CK_COND=1, T_DIR=1, BYT_BLOK=1, T_LENGTH=2 */
	cdb[3] = 0;			/* FEATURES */
	cdb[4] = 0;			/* FEATURES */
	cdb[5] = 0;			/* SECTORS */
	cdb[6] = 1;			/* SECTORS */
	cdb[7] = 0;			/* LBA LOW */
	cdb[8] = 0;			/* LBA LOW */
	cdb[9] = 0;			/* LBA MID */
	cdb[10] = 0;			/* LBA MID */
	cdb[11] = 0;			/* LBA HIGH */
	cdb[12] = 0;			/* LBA HIGH */
	cdb[13] = 0;			/* DEVICE */
	cdb[14] = 0xA1;			/* Command: ATA IDENTIFY PACKET DEVICE */;
	cdb[15] = 0;			/* CONTROL */
	memset(sense, 0, sizeof(sense));

	memset(&io_v4, 0, sizeof(struct sg_io_v4));
	io_v4.guard = 'Q';
	io_v4.protocol = BSG_PROTOCOL_SCSI;
	io_v4.subprotocol = BSG_SUB_PROTOCOL_SCSI_CMD;
	io_v4.request_len = sizeof (cdb);
	io_v4.request = (uintptr_t) cdb;
	io_v4.max_response_len = sizeof (sense);
	io_v4.response = (uintptr_t) sense;
	io_v4.din_xfer_len = buf_len;
	io_v4.din_xferp = (uintptr_t) buf;
	io_v4.timeout = COMMAND_TIMEOUT_MSEC;

	ret = ioctl(fd, SG_IO, &io_v4);
	if (ret != 0) {
		/* could be that the driver doesn't do version 4, try version 3 */
		if (errno == EINVAL) {
			struct sg_io_hdr io_hdr;

			memset(&io_hdr, 0, sizeof(struct sg_io_hdr));
			io_hdr.interface_id = 'S';
			io_hdr.cmdp = (unsigned char*) cdb;
			io_hdr.cmd_len = sizeof (cdb);
			io_hdr.dxferp = buf;
			io_hdr.dxfer_len = buf_len;
			io_hdr.sbp = sense;
			io_hdr.mx_sb_len = sizeof (sense);
			io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
			io_hdr.timeout = COMMAND_TIMEOUT_MSEC;

			ret = ioctl(fd, SG_IO, &io_hdr);
			if (ret != 0)
				goto out;
		} else {
			goto out;
		}
	}

	if (!(sense[0] == 0x72 && desc[0] == 0x9 && desc[1] == 0x0c)) {
		errno = EIO;
		ret = -1;
		goto out;
	}

 out:
	return ret;
}

int main(int argc, char *argv[])
{
	char buf[2048];
	char *id;
	char *path;
	int offset = 0;
	int fd;
	FILE *fp;

	if (argc < 2) {
		fprintf(stderr,
			"Usage: test-identify-packet /dev/srX [OFFSET]\n");
		return 1;
	}

	path = argv[1];
	if (argc > 2)
		offset = atoi(argv[2]);

	if (offset < 0 || offset > 512) {
		fprintf(stderr, "offset out of range\n");
		return 1;
	}

	fd = open(path, O_RDONLY|O_NONBLOCK);
	if (fd < 0) {
		perror("open");
		return 1;
	}

	fp = popen("hexdump -Cv", "w");
	if (!fp) {
		perror("popen");
		return 1;
	}

	id = (void *)((((unsigned long)buf + 511) & ~511) + offset);
	printf("id buffer=%p\n", id);

	disk_identify_packet_device_command(fd, id, 512);

	fwrite(id, 512, 1, fp);
	fclose(fp);

	return 0;
}

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

Some additional info. If I modify  test-identify-packet.c to align on 
_SC_PAGESIZE (4096 byte) rather than 512, as shown below, then run the loop:

offset=${1:-0}
increm=${2:-1}

while [ $((offset+=$increm)) -lt  $((4096-511)) ]; do
    echo -e "+++ ./test-identify-packetpage /dev/sr0 $offset\n"
    ./test-identify-packet-page /dev/sr0 $offset
    sleep 0.5
done

no panics occur, for every offset.

512 -> pagesize modification:

--- test-identify-packet.c      2011-01-17 13:47:25.000000000 -0500
+++ test-identify-packet-page.c 2011-01-17 22:27:11.293999984 -0500
@@ -99,7 +99,8 @@

  int main(int argc, char *argv[])
  {
-       char buf[2048];
+        int pgsz = sysconf(_SC_PAGESIZE);
+       char buf[pgsz<<1];
         char *id;
         char *path;
         int offset = 0;
@@ -116,7 +117,7 @@
         if (argc > 2)
                 offset = atoi(argv[2]);

-       if (offset < 0 || offset > 512) {
+       if (offset < 0 || offset > pgsz-512) {
                 fprintf(stderr, "offset out of range\n");
                 return 1;
         }
@@ -133,7 +134,7 @@
                 return 1;
         }

-       id = (void *)((((unsigned long)buf + 511) & ~511) + offset);
+       id = (void *)((((unsigned long)buf + pgsz-1) & ~(pgsz-1)) + offset);
         printf("id buffer=%p\n", id);

         disk_identify_packet_device_command(fd, id, 512);

John


On 01/17/2011 10:27 AM, Tejun Heo wrote:
> Hello,
>
> On Sun, Jan 16, 2011 at 11:03:06PM -0500, John Stanley wrote:
>> The kernel-panic, which occurs at boot-time in udev/ata_id.c when
>> issuing an ioctl SG_IO sg3 SCSI ATA Pass-through Identify command,
>> appears to arise from DMA'ing into an incorrectly aligned user data
>> buffer pointed to by sg_io_hdr.dxferp .
> The problem is that nobody is DMA'ing in this case.  The driver in
> question is ata_piix and the IO path taken is an actual PIO where the
> CPU reads from the IO space and writes to the memory itself.
>
>> My guess is that in the past, use of sg3 would not involve DMA by
>> default, but now, with libata ATA Pass-Through commands, it does (I
>> also may be totally wrong about that, just a thought).
> No DMA in progress here.  The only (somewhat) recent related change
> would be libata PIO path now using 32bit IO commands when supported by
> the controller, but I fail to see how that would trigger this type of
> failures.
>
>> I recall documentaion somewhere which emphasized that if direct I/O
>> (DMA) is to used in sg, one should page-align the SCSI response data
>> buffer..  With sg using indirect I/O this wouldn't be necessary, of
>> course, but perhaps now with libata, it is. Just guessing here.
> If the buffer is not aligned, the kernel would just create a bounce
> buffer and bounce the data, so it shouldn't be a problem either.  It
> looks like we have an obscure bug in buffer mapping code for SG_IO.
>
> I tried several things but can't reproduce the problem here.  Can you
> please try the attached minimal test case?  It issues IDENTIFY_PACKET
> and you can specify the alignment offset.  By default the buffer would
> be 512byte aligned but you can offset it.  ie. specifying 1 would make
> the buffer misaligned by 1 byte and so on.
>
> Can you please see whether the problem can be reliably triggered with
> it?  Also, please,
>
> * Attach full kernel log (including boot messages) and the program
>    output after triggering the problem.
>
> * Make sure the kernel is built with debug info and frame pointer.
>
> * Please reverse map the reported oops address to the source line.
>
> Thanks.
>

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Tejun Heo]

Hello,

Can you please test whether the following patch fixes the problem?

Thanks.

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 5defc74..9d46731 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1099,9 +1099,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		struct request_queue *q = sdev->request_queue;
 		void *buf;
 
-		/* set the min alignment and padding */
-		blk_queue_update_dma_alignment(sdev->request_queue,
-					       ATA_DMA_PAD_SZ - 1);
+		sdev->sector_size = ATA_SECT_SIZE;
+
+		/* set DMA padding */
 		blk_queue_update_dma_pad(sdev->request_queue,
 					 ATA_DMA_PAD_SZ - 1);
 
@@ -1115,13 +1115,18 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 
 		blk_queue_dma_drain(q, atapi_drain_needed, buf, ATAPI_MAX_DRAIN);
 	} else {
-		/* ATA devices must be sector aligned */
 		sdev->sector_size = ata_id_logical_sector_size(dev->id);
-		blk_queue_update_dma_alignment(sdev->request_queue,
-					       sdev->sector_size - 1);
 		sdev->manage_start_stop = 1;
 	}
 
+	/*
+	 * ata_pio_sectors() expect sector alignment on buffers.  ATAPI
+	 * devices also need the alignment as IDENTIFY_PACKET is executed
+	 * as ATA_PROT_PIO.
+	 */
+	blk_queue_update_dma_alignment(sdev->request_queue,
+				       sdev->sector_size - 1);
+
 	if (dev->flags & ATA_DFLAG_AN)
 		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
 

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

Fantastic... looks promising, test-identify-packet is running w/o issue, 
and boots, so far, as well. I will further test with a number of warm 
and cold boots over the next several days to see if anything crops up. 
I'm very grateful for your time and efforts with this, thanks again,
John

On 01/18/2011 10:09 AM, Tejun Heo wrote:
> Hello,
>
> Can you please test whether the following patch fixes the problem?
>
> Thanks.
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 5defc74..9d46731 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1099,9 +1099,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
>   		struct request_queue *q = sdev->request_queue;
>   		void *buf;
>
> -		/* set the min alignment and padding */
> -		blk_queue_update_dma_alignment(sdev->request_queue,
> -					       ATA_DMA_PAD_SZ - 1);
> +		sdev->sector_size = ATA_SECT_SIZE;
> +
> +		/* set DMA padding */
>   		blk_queue_update_dma_pad(sdev->request_queue,
>   					 ATA_DMA_PAD_SZ - 1);
>
> @@ -1115,13 +1115,18 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
>
>   		blk_queue_dma_drain(q, atapi_drain_needed, buf, ATAPI_MAX_DRAIN);
>   	} else {
> -		/* ATA devices must be sector aligned */
>   		sdev->sector_size = ata_id_logical_sector_size(dev->id);
> -		blk_queue_update_dma_alignment(sdev->request_queue,
> -					       sdev->sector_size - 1);
>   		sdev->manage_start_stop = 1;
>   	}
>
> +	/*
> +	 * ata_pio_sectors() expect sector alignment on buffers.  ATAPI
> +	 * devices also need the alignment as IDENTIFY_PACKET is executed
> +	 * as ATA_PROT_PIO.
> +	 */
> +	blk_queue_update_dma_alignment(sdev->request_queue,
> +				       sdev->sector_size - 1);
> +
>   	if (dev->flags&  ATA_DFLAG_AN)
>   		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
>
>

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[Brad Price]

Hi, I'm also seeing this issue. I applied the libata patch and so far it 
seem to solve the issue. I've done many boots, and run "sg_sat_identify 
-p /dev/dvd" in a loop 10k times with no kernel panics. I don't think I 
could execute that command 20 times without a kernel panic before this.

Thanks!
Brad

Re: Early-boot kernel panics from udev-165/extras/ata_id/ata_id.c

[John Stanley]

Ok, looks like this fixes it. So would this patch go in 2.6.37.1 ?

John

On 01/18/2011 10:09 AM, Tejun Heo wrote:
> Hello,
>
> Can you please test whether the following patch fixes the problem?
>
> Thanks.
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 5defc74..9d46731 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1099,9 +1099,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
>   		struct request_queue *q = sdev->request_queue;
>   		void *buf;
>
> -		/* set the min alignment and padding */
> -		blk_queue_update_dma_alignment(sdev->request_queue,
> -					       ATA_DMA_PAD_SZ - 1);
> +		sdev->sector_size = ATA_SECT_SIZE;
> +
> +		/* set DMA padding */
>   		blk_queue_update_dma_pad(sdev->request_queue,
>   					 ATA_DMA_PAD_SZ - 1);
>
> @@ -1115,13 +1115,18 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
>
>   		blk_queue_dma_drain(q, atapi_drain_needed, buf, ATAPI_MAX_DRAIN);
>   	} else {
> -		/* ATA devices must be sector aligned */
>   		sdev->sector_size = ata_id_logical_sector_size(dev->id);
> -		blk_queue_update_dma_alignment(sdev->request_queue,
> -					       sdev->sector_size - 1);
>   		sdev->manage_start_stop = 1;
>   	}
>
> +	/*
> +	 * ata_pio_sectors() expect sector alignment on buffers.  ATAPI
> +	 * devices also need the alignment as IDENTIFY_PACKET is executed
> +	 * as ATA_PROT_PIO.
> +	 */
> +	blk_queue_update_dma_alignment(sdev->request_queue,
> +				       sdev->sector_size - 1);
> +
>   	if (dev->flags&  ATA_DFLAG_AN)
>   		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
>
>

[PATCH #upstream-fixes] libata: set queue DMA alignment to sector size for ATAPI too

[Tejun Heo]

ata_pio_sectors() expects buffer for each sector to be contained in a
single page; otherwise, it ends up overrunning the first page.  This
is achieved by setting queue DMA alignment.  If sector_size is smaller
than PAGE_SIZE and all buffers are sector_size aligned, buffer for
each sector is always contained in a single page.

This wasn't applied to ATAPI devices but IDENTIFY_PACKET is executed
as ATA_PROT_PIO and thus uses ata_pio_sectors().  Newer versions of
udev issue IDENTIFY_PACKET with unaligned buffer triggering the
problem and causing oops.

This patch fixes the problem by setting sdev->sector_size to
ATA_SECT_SIZE on ATATPI devices and always setting DMA alignment to
sector_size.  While at it, add a warning for the unlikely but still
possible scenario where sector_size is larger than PAGE_SIZE, in which
case the alignment wouldn't be enough.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: John Stanley <jpsinthemix@verizon.net>
Tested-by: John Stanley <jpsinthemix@verizon.net>
Cc: stable@kernel.org
---
 drivers/ata/libata-scsi.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 5defc74..600f635 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1099,9 +1099,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		struct request_queue *q = sdev->request_queue;
 		void *buf;
 
-		/* set the min alignment and padding */
-		blk_queue_update_dma_alignment(sdev->request_queue,
-					       ATA_DMA_PAD_SZ - 1);
+		sdev->sector_size = ATA_SECT_SIZE;
+
+		/* set DMA padding */
 		blk_queue_update_dma_pad(sdev->request_queue,
 					 ATA_DMA_PAD_SZ - 1);
 
@@ -1115,13 +1115,25 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 
 		blk_queue_dma_drain(q, atapi_drain_needed, buf, ATAPI_MAX_DRAIN);
 	} else {
-		/* ATA devices must be sector aligned */
 		sdev->sector_size = ata_id_logical_sector_size(dev->id);
-		blk_queue_update_dma_alignment(sdev->request_queue,
-					       sdev->sector_size - 1);
 		sdev->manage_start_stop = 1;
 	}
 
+	/*
+	 * ata_pio_sectors() expects buffer for each sector to not cross
+	 * page boundary.  Enforce it by requiring buffers to be sector
+	 * aligned, which works iff sector_size is not larger than
+	 * PAGE_SIZE.  ATAPI devices also need the alignment as
+	 * IDENTIFY_PACKET is executed as ATA_PROT_PIO.
+	 */
+	if (sdev->sector_size > PAGE_SIZE)
+		ata_dev_printk(dev, KERN_WARNING,
+			"sector_size=%u > PAGE_SIZE, PIO may malfunction\n",
+			sdev->sector_size);
+
+	blk_queue_update_dma_alignment(sdev->request_queue,
+				       sdev->sector_size - 1);
+
 	if (dev->flags & ATA_DFLAG_AN)
 		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);
 

[PATCH #upstream-fixes] libata: set queue DMA alignment to sector

[Tejun Heo]

ata_pio_sectors() expects buffer for each sector to be contained in a
single page; otherwise, it ends up overrunning the first page.  This
is achieved by setting queue DMA alignment.  If sector_size is smaller
than PAGE_SIZE and all buffers are sector_size aligned, buffer for
each sector is always contained in a single page.

This wasn't applied to ATAPI devices but IDENTIFY_PACKET is executed
as ATA_PROT_PIO and thus uses ata_pio_sectors().  Newer versions of
udev issue IDENTIFY_PACKET with unaligned buffer triggering the
problem and causing oops.

This patch fixes the problem by setting sdev->sector_size to
ATA_SECT_SIZE on ATATPI devices and always setting DMA alignment to
sector_size.  While at it, add a warning for the unlikely but still
possible scenario where sector_size is larger than PAGE_SIZE, in which
case the alignment wouldn't be enough.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: John Stanley <jpsinthemix@verizon.net>
Tested-by: John Stanley <jpsinthemix@verizon.net>
Cc: stable@kernel.org
---
 drivers/ata/libata-scsi.c |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 5defc74..600f635 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1099,9 +1099,9 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 		struct request_queue *q = sdev->request_queue;
 		void *buf;
 
-		/* set the min alignment and padding */
-		blk_queue_update_dma_alignment(sdev->request_queue,
-					       ATA_DMA_PAD_SZ - 1);
+		sdev->sector_size = ATA_SECT_SIZE;
+
+		/* set DMA padding */
 		blk_queue_update_dma_pad(sdev->request_queue,
 					 ATA_DMA_PAD_SZ - 1);
 
@@ -1115,13 +1115,25 @@ static int ata_scsi_dev_config(struct scsi_device *sdev,
 
 		blk_queue_dma_drain(q, atapi_drain_needed, buf, ATAPI_MAX_DRAIN);
 	} else {
-		/* ATA devices must be sector aligned */
 		sdev->sector_size = ata_id_logical_sector_size(dev->id);
-		blk_queue_update_dma_alignment(sdev->request_queue,
-					       sdev->sector_size - 1);
 		sdev->manage_start_stop = 1;
 	}
 
+	/*
+	 * ata_pio_sectors() expects buffer for each sector to not cross
+	 * page boundary.  Enforce it by requiring buffers to be sector
+	 * aligned, which works iff sector_size is not larger than
+	 * PAGE_SIZE.  ATAPI devices also need the alignment as
+	 * IDENTIFY_PACKET is executed as ATA_PROT_PIO.
+	 */
+	if (sdev->sector_size > PAGE_SIZE)
+		ata_dev_printk(dev, KERN_WARNING,
+			"sector_size=%u > PAGE_SIZE, PIO may malfunction\n",
+			sdev->sector_size);
+
+	blk_queue_update_dma_alignment(sdev->request_queue,
+				       sdev->sector_size - 1);
+
 	if (dev->flags & ATA_DFLAG_AN)
 		set_bit(SDEV_EVT_MEDIA_CHANGE, sdev->supported_events);