[PATCH] increase the upper limit of sg_io64.dxfer_len to make "sg_logs -p=0x0f /dev/sgN" happy

[David Howells]

Woody Zhou <zhouwoody at yahoo.com> wrote:

>   Following is a patch to remedy an "invalid argument" error while
> executing "sg_logs -p=0x0f /dev/sgN". I submit it here for your
> review. At this place /dev/sgN should be mapped to a SCSI device
> which support page 0x0f(Application client), such as IBM
> IC35L036UCDY10-0 disks.
> 
> Index: 2.4.21-15.EL/arch/ppc64/kernel/ioctl32.c
> ===================================================================
> --- arch/ppc64/kernel/ioctl32.c.orig    2004-05-25
> 15:18:46.000000000 +0800
> +++ arch/ppc64/kernel/ioctl32.c 2004-05-25 15:20:33.000000000 +0800
> @@ -1301,7 +1301,7 @@
>                         goto out;
>                 }
>         } else {
> -               if (sg_io64.dxfer_len > 4*PAGE_SIZE) {
> +               if (sg_io64.dxfer_len > 8*PAGE_SIZE) {

I'm not sure that this patch is the best way to do things. The larger the
kmalloc() call made, the harder it is for the kernel to honour it. The kernel
needs to find enough contiguous and properly aligned memory to be able to do
this, and the larger the request, the harder it will be.

Furthermore, this takes no account of the fact that PAGE_SIZE can be
changed. If it was, for example, changed to 64KB, you'd be allocating an
enormous chunk of memory, and only using less than a page in this instance.

A better way would be, perhaps, to abuse the way KERNEL_DS and USER_DS affect
pointer checking:

Rather than do this in sg_ioctl_trans():

 (1) pull parameter block into kernel space
 (2) allocate an enormous scratch buffer
 (3) copy the input data into the buffer
 (4) set FS selector to KERNEL_DS
 (5) call sys_ioctl()
 (6) restore FS selector
 (7) copy the output data from the buffer
 (8) free the buffers

You could do this:

 (1) pull parameter block into kernel space
 (2) call verify_area() on the userspace buffers
 (3) point kernel param block buffer pointers at the userspace buffers
 (4) set FS selector to KERNEL_DS
 (5) call sys_ioctl()
 (6) restore FS selector

This ought to be sufficient. Setting KERNEL_DS merely disables the address
bounds checking in access_ok() and verify_area(). These don't actually limit
kernel pointers to kernel space; so as long as the check is performed
_somewhere_ before the driver tries to access the buffers, it should be okay.

After all, verify_area() and co. don't check VMA lists or anything like that
on ppc64. The driver is reliant on the other members of asm/uaccess.h for
that. Actually, the driver is very poor in this regard; it doesn't check the
return value of the functions that actually touch userspace. It does in fact
rely on verify_area(), which it shouldn't since that doesn't tell it whether
or not there's actually a mapping there. That is, however, a different problem
entirely.

David