Re: [Outreachy kernel] [PATCH] Staging: rtl8712: Use mem_dup() instead of copy_from_user()

[Cristina Opriceana <cristina.opriceana@gmail.com>]

On Du, 2015-03-22 at 21:04 +0100, Julia Lawall wrote:
> 
> On Sun, 22 Mar 2015, Cristina Opriceana wrote:
> 
> > Use mem_dup() instead of its duplicated implementation in order
> > to simplify code. Found with coccinelle.
> > 
> > Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> > ---
> >  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > index 81f39c3..c39d031 100644
> > --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > @@ -1912,13 +1912,9 @@ static int r871x_mp_ioctl_hdl(struct net_device *dev,
> >  	bset = (u8)(p->flags & 0xFFFF);
> >  	len = p->length;
> >  	pparmbuf = NULL;
> > -	pparmbuf = kmalloc(len, GFP_ATOMIC);
> > -	if (pparmbuf == NULL) {
> > -		ret = -ENOMEM;
> > -		goto _r871x_mp_ioctl_hdl_exit;
> > -	}
> > -	if (copy_from_user(pparmbuf, p->pointer, len)) {
> > -		ret = -EFAULT;
> > +	pparmbuf = memdup_user(p->pointer, len);
> 
> There is a potential problem with this.  memdup_user uses GFP_KERNEL 
> (blocking allowed), while the original code used GFP_ATOMIC (blocking not 
> allowed).  You could check whether any locks can be held on the way to 
> this code, which would require GFP_ATOMIC.  On the other hand, it may be 
> that copy_from_user can only be used when blocking is allowed.
> 
> julia
> 
As far as i understand, since copy_from_user() may sleep, it cannot be
used while holding a lock. So, the use of GFP_ATOMIC might be useless in
this context.
Either that or copy_from_user shouldn't be used, which is not the case
because the purpose of this function is to receive a command from the
userspace and call its associated handler.

Cristina