[PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 19 Aug 2016 08:50:23 +0200

Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/infiniband/hw/qib/qib_fs.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c
index fcdf3791..910f0d9 100644
--- a/drivers/infiniband/hw/qib/qib_fs.c
+++ b/drivers/infiniband/hw/qib/qib_fs.c
@@ -338,17 +338,12 @@ static ssize_t flash_write(struct file *file, const char __user *buf,
 		goto bail;
 	}
 
-	tmp = kmalloc(count, GFP_KERNEL);
-	if (!tmp) {
-		ret = -ENOMEM;
+	tmp = memdup_user(buf, count);
+	if (IS_ERR(tmp)) {
+		ret = PTR_ERR(tmp);
 		goto bail;
 	}
 
-	if (copy_from_user(tmp, buf, count)) {
-		ret = -EFAULT;
-		goto bail_tmp;
-	}
-
 	dd = private2dd(file);
 	if (qib_eeprom_write(dd, pos, tmp, count)) {
 		ret = -ENXIO;
-- 
2.9.3

RE: [PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[Marciniszyn, Mike]

> Subject: [PATCH] IB/qib: Use memdup_user() rather than duplicating its
> diff --git a/drivers/infiniband/hw/qib/qib_fs.c

I would be even more aggressive at reducing lines of code.

For example do direct returns when ok to do:
        if (pos != 0 || count != sizeof(struct qib_flash))
                return -EINVAL;

        tmp = memdup_user(buf, count);
        if (IS_ERR(tmp))
                return PTR_ERR(tmp);

The bail_tmp: label is then not needed.

Mike

Re: [PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[Leon Romanovsky]

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

On Fri, Aug 19, 2016 at 09:06:20AM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 19 Aug 2016 08:50:23 +0200
> 
> Reuse existing functionality from memdup_user() instead of keeping
> duplicate source code.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[Leon Romanovsky]

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

On Fri, Aug 19, 2016 at 03:27:20PM +0000, Marciniszyn, Mike wrote:
> > Subject: [PATCH] IB/qib: Use memdup_user() rather than duplicating its
> > diff --git a/drivers/infiniband/hw/qib/qib_fs.c
> 
> I would be even more aggressive at reducing lines of code.
> 
> For example do direct returns when ok to do:
>         if (pos != 0 || count != sizeof(struct qib_flash))
>                 return -EINVAL;
> 
>         tmp = memdup_user(buf, count);
>         if (IS_ERR(tmp))
>                 return PTR_ERR(tmp);
> 
> The bail_tmp: label is then not needed.

You still need to free tmp allocation if qib_eeprom_write failed and
this is your bail_tmp.

341         tmp = kmalloc(count, GFP_KERNEL);
342         if (!tmp) {
343                 ret = -ENOMEM;
344                 goto bail;
345         }
346
347         if (copy_from_user(tmp, buf, count)) {
348                 ret = -EFAULT;
349                 goto bail_tmp;
350         }
351
352         dd = private2dd(file);
353         if (qib_eeprom_write(dd, pos, tmp, count)) {
354                 ret = -ENXIO;
355                 qib_dev_err(dd, "failed to write to flash\n");
356                 goto bail_tmp;
357         }

> 
> Mike
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[Marciniszyn, Mike]

> >
> > The bail_tmp: label is then not needed.
> 
> You still need to free tmp allocation if qib_eeprom_write failed and this is
> your bail_tmp.
> 

Typo.   The bail: label is not needed.

Mike

Re: [PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[Leon Romanovsky]

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

On Fri, Aug 19, 2016 at 03:42:29PM +0000, Marciniszyn, Mike wrote:
> > >
> > > The bail_tmp: label is then not needed.
> > 
> > You still need to free tmp allocation if qib_eeprom_write failed and this is
> > your bail_tmp.
> > 
> 
> Typo.   The bail: label is not needed.

Yeah, it makes sense.

Thanks

> 
> Mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] IB/qib: Use memdup_user() rather than duplicating its implementation

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 699 bytes --]

On 8/19/2016 11:27 AM, Marciniszyn, Mike wrote:
>> Subject: [PATCH] IB/qib: Use memdup_user() rather than duplicating its
>> diff --git a/drivers/infiniband/hw/qib/qib_fs.c
> 
> I would be even more aggressive at reducing lines of code.
> 
> For example do direct returns when ok to do:
>         if (pos != 0 || count != sizeof(struct qib_flash))
>                 return -EINVAL;
> 
>         tmp = memdup_user(buf, count);
>         if (IS_ERR(tmp))
>                 return PTR_ERR(tmp);
> 
> The bail_tmp: label is then not needed.
> 
> Mike
> 

With Mike's additional cleanups in place, patch applied.

-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]