| Message ID | 20170420175549.3435196-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Apr 20, 2017 at 07:54:45PM +0200, Arnd Bergmann wrote: > kernelci.org reports a new compile warning for old code in the pmcraid > driver: > > arch/mips/include/asm/uaccess.h:138:21: warning: passing argument 1 of '__access_ok' makes pointer from integer without a cast [-Wint-conversion] > > The warning got introduced by a cleanup to the access_ok() helper > that requires the argument to be a pointer, where the old version > silently accepts 'unsigned long' arguments as it still does on most > other architectures. > > The new behavior in MIPS however seems absolutely sensible, and so far I > could only find one other file with the same issue, so the best solution > seems to be to clean up the pmcraid driver. > > This makes the driver consistently use 'void __iomem *' pointers for > passing around the address of the user space ioctl arguments, which gets > rid of the kernelci warning as well as several sparse warnings. Is there any point in keeping that access_ok() in the first place, rather than just switching to copy_from_user()/copy_to_user() in there? AFAICS, it's only for the sake of the loop in pmcraid_copy_sglist(): for (i = 0; i < (len / bsize_elem); i++, buffer += bsize_elem) { struct page *page = sg_page(&scatterlist[i]); kaddr = kmap(page); if (direction == DMA_TO_DEVICE) rc = __copy_from_user(kaddr, (void *)buffer, bsize_elem); else rc = __copy_to_user((void *)buffer, kaddr, bsize_elem); kunmap(page); if (rc) { pmcraid_err("failed to copy user data into sg list\n"); return -EFAULT; } scatterlist[i].length = bsize_elem; } and seeing that each of those calls copies is at least a full page... If there is an architecture where a single access_ok() costs a noticable fraction of the time it takes to copy a full page, we have a much worse problem than overhead in obscure ioctl...
On Thu, Apr 20, 2017 at 9:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Thu, Apr 20, 2017 at 07:54:45PM +0200, Arnd Bergmann wrote: >> kernelci.org reports a new compile warning for old code in the pmcraid >> driver: >> >> arch/mips/include/asm/uaccess.h:138:21: warning: passing argument 1 of '__access_ok' makes pointer from integer without a cast [-Wint-conversion] >> >> The warning got introduced by a cleanup to the access_ok() helper >> that requires the argument to be a pointer, where the old version >> silently accepts 'unsigned long' arguments as it still does on most >> other architectures. >> >> The new behavior in MIPS however seems absolutely sensible, and so far I >> could only find one other file with the same issue, so the best solution >> seems to be to clean up the pmcraid driver. >> >> This makes the driver consistently use 'void __iomem *' pointers for >> passing around the address of the user space ioctl arguments, which gets >> rid of the kernelci warning as well as several sparse warnings. > > Is there any point in keeping that access_ok() in the first place, rather > than just switching to copy_from_user()/copy_to_user() in there? AFAICS, > it's only for the sake of the loop in pmcraid_copy_sglist(): > for (i = 0; i < (len / bsize_elem); i++, buffer += bsize_elem) { > struct page *page = sg_page(&scatterlist[i]); > > kaddr = kmap(page); > if (direction == DMA_TO_DEVICE) > rc = __copy_from_user(kaddr, > (void *)buffer, > bsize_elem); > else > rc = __copy_to_user((void *)buffer, kaddr, bsize_elem); > > kunmap(page); > > if (rc) { > pmcraid_err("failed to copy user data into sg list\n"); > return -EFAULT; > } > > scatterlist[i].length = bsize_elem; > } > and seeing that each of those calls copies is at least a full page... If > there is an architecture where a single access_ok() costs a noticable fraction > of the time it takes to copy a full page, we have a much worse problem than > overhead in obscure ioctl... Right, that would also fix the warning. I think we should just do both fixes, as they are each a worthwhile cleanup. I can do this as another patch on top of the series. I've done that second patch now and given it a spin on the randconfig test builds. Arnd
diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c index 49e70a383afa..096c704ca39a 100644 --- a/drivers/scsi/pmcraid.c +++ b/drivers/scsi/pmcraid.c @@ -3325,7 +3325,7 @@ static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen) */ static int pmcraid_copy_sglist( struct pmcraid_sglist *sglist, - unsigned long buffer, + void __user *buffer, u32 len, int direction ) @@ -3346,11 +3346,9 @@ static int pmcraid_copy_sglist( kaddr = kmap(page); if (direction == DMA_TO_DEVICE) - rc = __copy_from_user(kaddr, - (void *)buffer, - bsize_elem); + rc = __copy_from_user(kaddr, buffer, bsize_elem); else - rc = __copy_to_user((void *)buffer, kaddr, bsize_elem); + rc = __copy_to_user(buffer, kaddr, bsize_elem); kunmap(page); @@ -3368,13 +3366,9 @@ static int pmcraid_copy_sglist( kaddr = kmap(page); if (direction == DMA_TO_DEVICE) - rc = __copy_from_user(kaddr, - (void *)buffer, - len % bsize_elem); + rc = __copy_from_user(kaddr, buffer, len % bsize_elem); else - rc = __copy_to_user((void *)buffer, - kaddr, - len % bsize_elem); + rc = __copy_to_user(buffer, kaddr, len % bsize_elem); kunmap(page); @@ -3652,17 +3646,17 @@ static long pmcraid_ioctl_passthrough( struct pmcraid_instance *pinstance, unsigned int ioctl_cmd, unsigned int buflen, - unsigned long arg + void __user *arg ) { struct pmcraid_passthrough_ioctl_buffer *buffer; struct pmcraid_ioarcb *ioarcb; struct pmcraid_cmd *cmd; struct pmcraid_cmd *cancel_cmd; - unsigned long request_buffer; + void __user *request_buffer; unsigned long request_offset; unsigned long lock_flags; - void *ioasa; + void __user *ioasa; u32 ioasc; int request_size; int buffer_size; @@ -3701,13 +3695,10 @@ static long pmcraid_ioctl_passthrough( request_buffer = arg + request_offset; - rc = __copy_from_user(buffer, - (struct pmcraid_passthrough_ioctl_buffer *) arg, + rc = __copy_from_user(buffer, arg, sizeof(struct pmcraid_passthrough_ioctl_buffer)); - ioasa = - (void *)(arg + - offsetof(struct pmcraid_passthrough_ioctl_buffer, ioasa)); + ioasa = arg + offsetof(struct pmcraid_passthrough_ioctl_buffer, ioasa); if (rc) { pmcraid_err("ioctl: can't copy passthrough buffer\n"); @@ -4021,6 +4012,7 @@ static long pmcraid_chr_ioctl( { struct pmcraid_instance *pinstance = NULL; struct pmcraid_ioctl_header *hdr = NULL; + void __user *argp = (void __user *)arg; int retval = -ENOTTY; hdr = kmalloc(sizeof(struct pmcraid_ioctl_header), GFP_KERNEL); @@ -4030,7 +4022,7 @@ static long pmcraid_chr_ioctl( return -ENOMEM; } - retval = pmcraid_check_ioctl_buffer(cmd, (void *)arg, hdr); + retval = pmcraid_check_ioctl_buffer(cmd, argp, hdr); if (retval) { pmcraid_info("chr_ioctl: header check failed\n"); @@ -4055,10 +4047,8 @@ static long pmcraid_chr_ioctl( if (cmd == PMCRAID_IOCTL_DOWNLOAD_MICROCODE) scsi_block_requests(pinstance->host); - retval = pmcraid_ioctl_passthrough(pinstance, - cmd, - hdr->buffer_length, - arg); + retval = pmcraid_ioctl_passthrough(pinstance, cmd, + hdr->buffer_length, argp); if (cmd == PMCRAID_IOCTL_DOWNLOAD_MICROCODE) scsi_unblock_requests(pinstance->host); @@ -4066,10 +4056,8 @@ static long pmcraid_chr_ioctl( case PMCRAID_DRIVER_IOCTL: arg += sizeof(struct pmcraid_ioctl_header); - retval = pmcraid_ioctl_driver(pinstance, - cmd, - hdr->buffer_length, - (void __user *)arg); + retval = pmcraid_ioctl_driver(pinstance, cmd, + hdr->buffer_length, argp); break; default:
kernelci.org reports a new compile warning for old code in the pmcraid driver: arch/mips/include/asm/uaccess.h:138:21: warning: passing argument 1 of '__access_ok' makes pointer from integer without a cast [-Wint-conversion] The warning got introduced by a cleanup to the access_ok() helper that requires the argument to be a pointer, where the old version silently accepts 'unsigned long' arguments as it still does on most other architectures. The new behavior in MIPS however seems absolutely sensible, and so far I could only find one other file with the same issue, so the best solution seems to be to clean up the pmcraid driver. This makes the driver consistently use 'void __iomem *' pointers for passing around the address of the user space ioctl arguments, which gets rid of the kernelci warning as well as several sparse warnings. Fixes: f0a955f4eeec ("mips: sanitize __access_ok()") Cc: Alexander Viro <viro@zeniv.linux.org.uk> Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- I wanted to be sure that I get all the __iomem annotations right, so I ended up fixing all other sparse warnings as well, see the three follow-up patches. --- drivers/scsi/pmcraid.c | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-)