ide-scsi oops with ide tape drive

ide-scsi oops with ide tape drive

[Stuart_Hayes]

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


Hello!

We're seeing a null pointer dereference with certain IDE tape drives on
2.6.11 when we use it with ide-scsi (i686 architecture).  The problem is
that the scatter-gather pages aren't mapped to kernel virtual address
space in idescsi_output_buffers()/idescsi_input_buffers(), so, if these
pages are in high memory, page_address() returns a null pointer.

This patch fixes the problem.  I'll attach it as a file, too, just in
case it gets mangled.  Please let me know if there are any problems with
or questions regarding this patch.

Again, this patch is against 2.6.11.

Thanks!
Stuart Hayes
stuart_hayes@dell.com



--- ide-scsi.c.orig	2005-03-08 13:44:38.000000000 -0500
+++ ide-scsi.c	2005-03-08 14:02:43.000000000 -0500
@@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
 			return;
 		}
 		count = min(pc->sg->length - pc->b_count, bcount);
-		buf = page_address(pc->sg->page) + pc->sg->offset;
+		buf = kmap_atomic(pc->sg->page, KM_USER0) +
pc->sg->offset;
 		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
count);
+		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
 			return;
 		}
 		count = min(pc->sg->length - pc->b_count, bcount);
-		buf = page_address(pc->sg->page) + pc->sg->offset;
+		buf = kmap_atomic(pc->sg->page, KM_USER0) +
pc->sg->offset;
 		drive->hwif->atapi_output_bytes(drive, buf +
pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;

[-- Attachment #2: ide-scsi.2.6.11.patch --]
[-- Type: application/octet-stream, Size: 1034 bytes --]

--- ide-scsi.c.orig	2005-03-08 13:44:38.000000000 -0500
+++ ide-scsi.c	2005-03-08 14:02:43.000000000 -0500
@@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
 			return;
 		}
 		count = min(pc->sg->length - pc->b_count, bcount);
-		buf = page_address(pc->sg->page) + pc->sg->offset;
+		buf = kmap_atomic(pc->sg->page, KM_USER0) + pc->sg->offset;
 		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;
@@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
 			return;
 		}
 		count = min(pc->sg->length - pc->b_count, bcount);
-		buf = page_address(pc->sg->page) + pc->sg->offset;
+		buf = kmap_atomic(pc->sg->page, KM_USER0) + pc->sg->offset;
 		drive->hwif->atapi_output_bytes(drive, buf + pc->b_count, count);
+		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
 		bcount -= count; pc->b_count += count;
 		if (pc->b_count == pc->sg->length) {
 			pc->sg++;

Re: ide-scsi oops with ide tape drive

[Jens Axboe]

On Tue, Mar 08 2005, Stuart_Hayes@Dell.com wrote:
> 
> Hello!
> 
> We're seeing a null pointer dereference with certain IDE tape drives on
> 2.6.11 when we use it with ide-scsi (i686 architecture).  The problem is
> that the scatter-gather pages aren't mapped to kernel virtual address
> space in idescsi_output_buffers()/idescsi_input_buffers(), so, if these
> pages are in high memory, page_address() returns a null pointer.
> 
> This patch fixes the problem.  I'll attach it as a file, too, just in
> case it gets mangled.  Please let me know if there are any problems with
> or questions regarding this patch.
> 
> Again, this patch is against 2.6.11.
> 
> Thanks!
> Stuart Hayes
> stuart_hayes@dell.com
> 
> 
> 
> --- ide-scsi.c.orig	2005-03-08 13:44:38.000000000 -0500
> +++ ide-scsi.c	2005-03-08 14:02:43.000000000 -0500
> @@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
>  			return;
>  		}
>  		count = min(pc->sg->length - pc->b_count, bcount);
> -		buf = page_address(pc->sg->page) + pc->sg->offset;
> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
> pc->sg->offset;
>  		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
> count);
> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>  		bcount -= count; pc->b_count += count;
>  		if (pc->b_count == pc->sg->length) {
>  			pc->sg++;

You need a local_irq_save(flags); ... local_irq_restore(flags); around
the kmap(atomic), transfer, and kunmap_atomic() for this to be safe.
Interrupts may not be disabled at this point, depends on drive settings.

> @@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
>  			return;
>  		}
>  		count = min(pc->sg->length - pc->b_count, bcount);
> -		buf = page_address(pc->sg->page) + pc->sg->offset;
> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
> pc->sg->offset;
>  		drive->hwif->atapi_output_bytes(drive, buf +
> pc->b_count, count);
> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>  		bcount -= count; pc->b_count += count;
>  		if (pc->b_count == pc->sg->length) {
>  			pc->sg++;

Ditto.

-- 
Jens Axboe

RE: ide-scsi oops with ide tape drive

[Stuart_Hayes]

Jens Axboe wrote:
> On Tue, Mar 08 2005, Stuart_Hayes@Dell.com wrote:
>> 
>> Hello!
>> 
>> We're seeing a null pointer dereference with certain IDE tape drives
>> on 
>> 2.6.11 when we use it with ide-scsi (i686 architecture).  The
>> problem is that the scatter-gather pages aren't mapped to kernel
>> virtual address space in
>> idescsi_output_buffers()/idescsi_input_buffers(), so, if these pages
>> are in high memory, page_address() returns a null pointer. 
>> 
>> This patch fixes the problem.  I'll attach it as a file, too, just in
>> case it gets mangled.  Please let me know if there are any problems
>> with or questions regarding this patch.
>> 
>> Again, this patch is against 2.6.11.
>> 
>> Thanks!
>> Stuart Hayes
>> stuart_hayes@dell.com
>> 
>> 
>> 
>> --- ide-scsi.c.orig	2005-03-08 13:44:38.000000000 -0500
>> +++ ide-scsi.c	2005-03-08 14:02:43.000000000 -0500
>> @@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d 
>>  		return; }
>>  		count = min(pc->sg->length - pc->b_count, bcount);
>> -		buf = page_address(pc->sg->page) + pc->sg->offset;
>> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
>> pc->sg->offset;
>>  		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
>> count);
>> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>>  		bcount -= count; pc->b_count += count;
>>  		if (pc->b_count == pc->sg->length) {
>>  			pc->sg++;
> 
> You need a local_irq_save(flags); ... local_irq_restore(flags); around
> the kmap(atomic), transfer, and kunmap_atomic() for this to be safe.
> Interrupts may not be disabled at this point, depends on drive
> settings. 
> 

Thanks for the quick response!  I'll look into this more carefully.
--Stuart

>> @@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_ 
>>  		return; }
>>  		count = min(pc->sg->length - pc->b_count, bcount);
>> -		buf = page_address(pc->sg->page) + pc->sg->offset;
>> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
>> pc->sg->offset;
>>  		drive->hwif->atapi_output_bytes(drive, buf +
>> pc->b_count, count);
>> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>>  		bcount -= count; pc->b_count += count;
>>  		if (pc->b_count == pc->sg->length) {
>>  			pc->sg++;
> 
> Ditto.

RE: ide-scsi oops with ide tape drive

[Stuart_Hayes]

Hayes, Stuart wrote:
> Jens Axboe wrote:
>> On Tue, Mar 08 2005, Stuart_Hayes@Dell.com wrote:
>>> 
>>> Hello!
>>> 
>>> We're seeing a null pointer dereference with certain IDE tape
>>> drives on 
>>> 2.6.11 when we use it with ide-scsi (i686 architecture).  The
>>> problem is that the scatter-gather pages aren't mapped to kernel
>>> virtual address space in
>>> idescsi_output_buffers()/idescsi_input_buffers(), so, if these
>>> pages are in high memory, page_address() returns a null pointer. 
>>> 
>>> This patch fixes the problem.  I'll attach it as a file, too, just
>>> in case it gets mangled.  Please let me know if there are any
>>> problems with or questions regarding this patch.
>>> 
>>> Again, this patch is against 2.6.11.
>>> 
>>> Thanks!
>>> Stuart Hayes
>>> stuart_hayes@dell.com
>>> 
>>> 
>>> 
>>> --- ide-scsi.c.orig	2005-03-08 13:44:38.000000000 -0500
>>> +++ ide-scsi.c	2005-03-08 14:02:43.000000000 -0500
>>> @@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d 
>>>  		return; } count = min(pc->sg->length - pc->b_count,
bcount);
>>> -		buf = page_address(pc->sg->page) + pc->sg->offset;
>>> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
>>> pc->sg->offset;
>>>  		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
count);
>>> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>>>  		bcount -= count; pc->b_count += count;
>>>  		if (pc->b_count == pc->sg->length) {
>>>  			pc->sg++;
>> 
>> You need a local_irq_save(flags); ... local_irq_restore(flags);
>> around the kmap(atomic), transfer, and kunmap_atomic() for this to
>> be safe. Interrupts may not be disabled at this point, depends on
>> drive settings. 
>> 
> 
> Thanks for the quick response!  I'll look into this more carefully.
> --Stuart
> 

Oh, yeah, unmaskirq...

Do you see any problem with masking local IRQs during the transfer?  If
unmask is set, it could be because the device is a slow PIO device, so
transferring a whole page could take over 2.5ms.

I could check, and if unmask is set && the page is in high memory, I
could copy the page down to low memory and allow the transfer to the
drive to proceed with irqs unmasked.  Do you think it would be worth the
extra code complexity?

Thanks
Stuart


>>> @@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_ 
>>>  		return; } count = min(pc->sg->length - pc->b_count,
bcount);
>>> -		buf = page_address(pc->sg->page) + pc->sg->offset;
>>> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
>>> pc->sg->offset;
>>>  		drive->hwif->atapi_output_bytes(drive, buf +
>>> pc->b_count, count);
>>> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>>>  		bcount -= count; pc->b_count += count;
>>>  		if (pc->b_count == pc->sg->length) {
>>>  			pc->sg++;
>> 
>> Ditto.

RE: ide-scsi oops with ide tape drive

[Stuart_Hayes]

> Hayes, Stuart wrote:
>> Jens Axboe wrote:
>>> On Tue, Mar 08 2005, Stuart_Hayes@Dell.com wrote:
>>>> 
>>>> Hello!
>>>> 
>>>> We're seeing a null pointer dereference with certain IDE tape
>>>> drives on 
>>>> 2.6.11 when we use it with ide-scsi (i686 architecture).  The
>>>> problem is that the scatter-gather pages aren't mapped to kernel
>>>> virtual address space in
>>>> idescsi_output_buffers()/idescsi_input_buffers(), so, if these
>>>> pages are in high memory, page_address() returns a null pointer. 
>>>> 
>>>> This patch fixes the problem.  I'll attach it as a file, too, just
>>>> in case it gets mangled.  Please let me know if there are any
>>>> problems with or questions regarding this patch.
>>>> 
>>>> Again, this patch is against 2.6.11.
>>>> 
>>>> Thanks!
>>>> Stuart Hayes
>>>> stuart_hayes@dell.com
>>>> 
>>>> 
>>>> 
>>>> --- ide-scsi.c.orig	2005-03-08 13:44:38.000000000 -0500
>>>> +++ ide-scsi.c	2005-03-08 14:02:43.000000000 -0500
>>>> @@ -151,8 +151,9 @@ static void idescsi_input_buffers (ide_d
>>>>  		return; } count = min(pc->sg->length - pc->b_count,
bcount);
>>>> -		buf = page_address(pc->sg->page) + pc->sg->offset;
>>>> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
>>>> pc->sg->offset;
>>>>  		drive->hwif->atapi_input_bytes(drive, buf + pc->b_count,
count);
>>>> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>>>>  		bcount -= count; pc->b_count += count;
>>>>  		if (pc->b_count == pc->sg->length) {
>>>>  			pc->sg++;
>>> 
>>> You need a local_irq_save(flags); ... local_irq_restore(flags);
>>> around the kmap(atomic), transfer, and kunmap_atomic() for this to
>>> be safe. Interrupts may not be disabled at this point, depends on
>>> drive settings. 
>>> 
>> 
>> Thanks for the quick response!  I'll look into this more carefully.
>> --Stuart 
>> 
> 
> Oh, yeah, unmaskirq...
> 
> Do you see any problem with masking local IRQs during the transfer? 
> If unmask is set, it could be because the device is a slow PIO
> device, so transferring a whole page could take over 2.5ms.  
> 
> I could check, and if unmask is set && the page is in high memory, I
> could copy the page down to low memory and allow the transfer to the
> drive to proceed with irqs unmasked.  Do you think it would be worth
> the extra code complexity?   
> 
> Thanks
> Stuart
> 
> 

Oops, it's a worst case of ~1.2ms, not 2.5ms.  I guess that would be OK.

I'll rework the patch and send it again in a bit.


>>>> @@ -173,8 +174,9 @@ static void idescsi_output_buffers (ide_
>>>>  		return; } count = min(pc->sg->length - pc->b_count,
bcount);
>>>> -		buf = page_address(pc->sg->page) + pc->sg->offset;
>>>> +		buf = kmap_atomic(pc->sg->page, KM_USER0) +
>>>> pc->sg->offset;
>>>>  		drive->hwif->atapi_output_bytes(drive, buf +
>>>> pc->b_count, count);
>>>> +		kunmap_atomic(buf - pc->sg->offset, KM_USER0);
>>>>  		bcount -= count; pc->b_count += count;
>>>>  		if (pc->b_count == pc->sg->length) {
>>>>  			pc->sg++;
>>> 
>>> Ditto.