Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1627723 > unrolled thread

[PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl argument

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl argument Arnd Bergmann <arnd@arndb.de> - 2017-04-20 20:00 +0200
    Re: [PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl  argument Al Viro <viro@ZenIV.linux.org.uk> - 2017-04-20 21:30 +0200
      Re: [PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl argument Arnd Bergmann <arnd@arndb.de> - 2017-04-22 00:10 +0200
    [PATCH] scsi: pmcraid: use normal copy_from_user Arnd Bergmann <arnd@arndb.de> - 2017-04-22 00:10 +0200
      Re: [PATCH] scsi: pmcraid: use normal copy_from_user Christoph Hellwig <hch@infradead.org> - 2017-04-23 10:30 +0200

#1627723 — [PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl argument

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(-)

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:
-- 
2.9.0

[toc] | [next] | [standalone]

#1627759 — Re: [PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl argument

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...

[toc] | [prev] | [next] | [standalone]

#1628644

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

[toc] | [prev] | [next] | [standalone]

#1628640 — [PATCH] scsi: pmcraid: use normal copy_from_user

As pointed out by Al Viro for my previous series, the driver has no need
to call access_ok() and __copy_from_user()/__copy_to_user(). Changing
it to regular copy_from_user()/copy_to_user() simplifies the code without
any real downsides, making it less error-prone at best.

This patch by itself also addresses the warning about the access_ok()
macro on MIPS, but both fixes improve the code, so ideally we apply
them both.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/pmcraid.c | 40 +++++++---------------------------------
 1 file changed, 7 insertions(+), 33 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 63298f017171..2091bdf298ef 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3348,9 +3348,9 @@ static int pmcraid_copy_sglist(
 
 		kaddr = kmap(page);
 		if (direction == DMA_TO_DEVICE)
-			rc = __copy_from_user(kaddr, buffer, bsize_elem);
+			rc = copy_from_user(kaddr, buffer, bsize_elem);
 		else
-			rc = __copy_to_user(buffer, kaddr, bsize_elem);
+			rc = copy_to_user(buffer, kaddr, bsize_elem);
 
 		kunmap(page);
 
@@ -3368,9 +3368,9 @@ static int pmcraid_copy_sglist(
 		kaddr = kmap(page);
 
 		if (direction == DMA_TO_DEVICE)
-			rc = __copy_from_user(kaddr, buffer, len % bsize_elem);
+			rc = copy_from_user(kaddr, buffer, len % bsize_elem);
 		else
-			rc = __copy_to_user(buffer, kaddr, len % bsize_elem);
+			rc = copy_to_user(buffer, kaddr, len % bsize_elem);
 
 		kunmap(page);
 
@@ -3697,7 +3697,7 @@ static long pmcraid_ioctl_passthrough(
 
 	request_buffer = arg + request_offset;
 
-	rc = __copy_from_user(buffer, arg,
+	rc = copy_from_user(buffer, arg,
 			     sizeof(struct pmcraid_passthrough_ioctl_buffer));
 
 	ioasa = arg + offsetof(struct pmcraid_passthrough_ioctl_buffer, ioasa);
@@ -3718,14 +3718,7 @@ static long pmcraid_ioctl_passthrough(
 		direction = DMA_FROM_DEVICE;
 	}
 
-	if (request_size > 0) {
-		rc = access_ok(access, arg, request_offset + request_size);
-
-		if (!rc) {
-			rc = -EFAULT;
-			goto out_free_buffer;
-		}
-	} else if (request_size < 0) {
+	if (request_size < 0) {
 		rc = -EINVAL;
 		goto out_free_buffer;
 	}
@@ -3935,11 +3928,6 @@ static long pmcraid_ioctl_driver(
 {
 	int rc = -ENOSYS;
 
-	if (!access_ok(VERIFY_READ, user_buffer, _IOC_SIZE(cmd))) {
-		pmcraid_err("ioctl_driver: access fault in request buffer\n");
-		return -EFAULT;
-	}
-
 	switch (cmd) {
 	case PMCRAID_IOCTL_RESET_ADAPTER:
 		pmcraid_reset_bringup(pinstance);
@@ -3971,8 +3959,7 @@ static int pmcraid_check_ioctl_buffer(
 	struct pmcraid_ioctl_header *hdr
 )
 {
-	int rc = 0;
-	int access = VERIFY_READ;
+	int rc;
 
 	if (copy_from_user(hdr, arg, sizeof(struct pmcraid_ioctl_header))) {
 		pmcraid_err("couldn't copy ioctl header from user buffer\n");
@@ -3988,19 +3975,6 @@ static int pmcraid_check_ioctl_buffer(
 		return -EINVAL;
 	}
 
-	/* check for appropriate buffer access */
-	if ((_IOC_DIR(cmd) & _IOC_READ) == _IOC_READ)
-		access = VERIFY_WRITE;
-
-	rc = access_ok(access,
-		       (arg + sizeof(struct pmcraid_ioctl_header)),
-		       hdr->buffer_length);
-	if (!rc) {
-		pmcraid_err("access failed for user buffer of size %d\n",
-			     hdr->buffer_length);
-		return -EFAULT;
-	}
-
 	return 0;
 }
 
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1628931 — Re: [PATCH] scsi: pmcraid: use normal copy_from_user

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web