[PATCH 12/27] drivers/ieee1394: Use memdup_user

[PATCH 12/27] drivers/ieee1394: Use memdup_user

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Use memdup_user when user data is immediately copied into the
allocated region.

The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression from,to,size,flag;
position p;
identifier l1,l2;
@@

-  to = \(kmalloc@p\|kzalloc@p\)(size,flag);
+  to = memdup_user(from,size);
   if (
-      to==NULL
+      IS_ERR(to)
                 || ...) {
   <+... when != goto l1;
-  -ENOMEM
+  PTR_ERR(to)
   ...+>
   }
-  if (copy_from_user(to, from, size) != 0) {
-    <+... when != goto l2;
-    -EFAULT
-    ...+>
-  }
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/ieee1394/video1394.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/ieee1394/video1394.c b/drivers/ieee1394/video1394.c
index a42bd68..a77483b 100644
--- a/drivers/ieee1394/video1394.c
+++ b/drivers/ieee1394/video1394.c
@@ -1045,14 +1045,9 @@ static long video1394_ioctl(struct file *file,
 			if (get_user(qv, &p->packet_sizes))
 				return -EFAULT;
 
-			psizes = kmalloc(buf_size, GFP_KERNEL);
-			if (!psizes)
-				return -ENOMEM;
-
-			if (copy_from_user(psizes, qv, buf_size)) {
-				kfree(psizes);
-				return -EFAULT;
-			}
+			psizes = memdup_user(qv, buf_size);
+			if (IS_ERR(psizes))
+				return PTR_ERR(psizes);
 		}
 
 		spin_lock_irqsave(&d->lock,flags);

Re: [PATCH 12/27] drivers/ieee1394: Use memdup_user

[Stefan Richter]

Julia Lawall wrote:
> Use memdup_user when user data is immediately copied into the
> allocated region.

Looks nice.  We won't apply janitorial updates to drivers/ieee1394
anymore though, since it is made obsolete by drivers/firewire and to be
removed sooner than later.  (I will post a proposed removal schedule
today to make this better known.)
-- 
Stefan Richter
-=====-==-=- -=-= =-==-
http://arcgraph.de/sr/

Re: [PATCH 12/27] drivers/ieee1394: Use memdup_user

[Andrew Morton]

On Sat, 22 May 2010 11:25:47 +0200
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Julia Lawall wrote:
> > Use memdup_user when user data is immediately copied into the
> > allocated region.
> 
> Looks nice.  We won't apply janitorial updates to drivers/ieee1394
> anymore though, since it is made obsolete by drivers/firewire and to be
> removed sooner than later.  (I will post a proposed removal schedule
> today to make this better known.)

I don't see much point in declaring a moratorium against
drivers/ieee1394.  Perhaps that removal will never happen.  Perhaps
third parties will find reason to revert that removal and to continue
to maintain drivers/ieee1394.  Perhaps people will take code snippets
from drivers/ieee1394 and will move them into drivers/firewire.

The bottom line is that the patch improves the code.  And there ain't
nothing wrong with having better code.

Re: [PATCH 12/27] drivers/ieee1394: Use memdup_user

[Stefan Richter]

Andrew Morton wrote:
> On Sat, 22 May 2010 11:25:47 +0200
> Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
>> Julia Lawall wrote:
>>> Use memdup_user when user data is immediately copied into the
>>> allocated region.
>> Looks nice.  We won't apply janitorial updates to drivers/ieee1394
>> anymore though, since it is made obsolete by drivers/firewire and to be
>> removed sooner than later.  (I will post a proposed removal schedule
>> today to make this better known.)
> 
> I don't see much point in declaring a moratorium against
> drivers/ieee1394.  Perhaps that removal will never happen.

That's a wrong impression due to us driver developers dragging the
coexistence of two driver stacks out for too long.

> Perhaps
> third parties will find reason to revert that removal and to continue
> to maintain drivers/ieee1394.  Perhaps people will take code snippets
> from drivers/ieee1394 and will move them into drivers/firewire.
> 
> The bottom line is that the patch improves the code.  And there ain't
> nothing wrong with having better code.

This code is as good as it needs to be.

Spending time on things that can be done is fun, but spending time on
things that need to be done can't hurt either.
-- 
Stefan Richter
-=====-==-=- -=-= ==-=-
http://arcgraph.de/sr/

Re: [PATCH 12/27] drivers/ieee1394: Use memdup_user

[Stefan Richter]

Julia Lawall wrote on 2010-05-22:
...
> --- a/drivers/ieee1394/video1394.c
> +++ b/drivers/ieee1394/video1394.c
> @@ -1045,14 +1045,9 @@ static long video1394_ioctl(struct file *file,
>  			if (get_user(qv, &p->packet_sizes))
>  				return -EFAULT;
>  
> -			psizes = kmalloc(buf_size, GFP_KERNEL);
> -			if (!psizes)
> -				return -ENOMEM;
> -
> -			if (copy_from_user(psizes, qv, buf_size)) {
> -				kfree(psizes);
> -				return -EFAULT;
> -			}
> +			psizes = memdup_user(qv, buf_size);
> +			if (IS_ERR(psizes))
> +				return PTR_ERR(psizes);
>  		}
>  
>  		spin_lock_irqsave(&d->lock,flags);

Committed to linux1394-2.6.git now, thanks.  (Because akpm said that it
is worth it regardless of video1394 being at its end of life.)
-- 
Stefan Richter
-=====-==-=- -=-= =====
http://arcgraph.de/sr/