[PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[Dhere, Chaitanya (C.)]

This patch replaces kzalloc and copy_from_user with memdup_user call
This change was detected with coccinelle tool

Signed-off-by: Chaitanya Dhere <cvijaydh@visteon.com>
---
 drivers/staging/lustre/lustre/llite/file.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index 85e74d1..85b5567 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -2368,14 +2368,9 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		struct hsm_state_set	*hss;
 		int			 rc;
 
-		hss = kzalloc(sizeof(*hss), GFP_NOFS);
-		if (!hss)
-			return -ENOMEM;
-
-		if (copy_from_user(hss, (char *)arg, sizeof(*hss))) {
-			OBD_FREE_PTR(hss);
-			return -EFAULT;
-		}
+		hss = memdup_user((char *)arg, sizeof(*hss));
+		if (IS_ERR(hss))
+			return PTR_ERR(hss);
 
 		rc = ll_hsm_state_set(inode, hss);
 
-- 
1.7.9.5

Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[Julia Lawall]

On Tue, 31 Mar 2015, Dhere, Chaitanya (C.) wrote:

> This patch replaces kzalloc and copy_from_user with memdup_user call
> This change was detected with coccinelle tool
>
> Signed-off-by: Chaitanya Dhere <cvijaydh@visteon.com>
> ---
>  drivers/staging/lustre/lustre/llite/file.c |   11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> index 85e74d1..85b5567 100644
> --- a/drivers/staging/lustre/lustre/llite/file.c
> +++ b/drivers/staging/lustre/lustre/llite/file.c
> @@ -2368,14 +2368,9 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  		struct hsm_state_set	*hss;
>  		int			 rc;
>
> -		hss = kzalloc(sizeof(*hss), GFP_NOFS);
> -		if (!hss)
> -			return -ENOMEM;
> -
> -		if (copy_from_user(hss, (char *)arg, sizeof(*hss))) {
> -			OBD_FREE_PTR(hss);
> -			return -EFAULT;
> -		}
> +		hss = memdup_user((char *)arg, sizeof(*hss));

memdup_user will use the flag GFP_KERNEL, ie (__GFP_WAIT | __GFP_IO |
__GFP_FS), rather than the flag GFP_NOFS, ie (__GFP_WAIT | __GFP_IO), that
is specified.  I don't know if this is a problem here.

julia

> +		if (IS_ERR(hss))
> +			return PTR_ERR(hss);
>
>  		rc = ll_hsm_state_set(inode, hss);
>
> --
> 1.7.9.5
>

Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[gregkh]

On Tue, Mar 31, 2015 at 05:15:23PM +0200, Julia Lawall wrote:
> On Tue, 31 Mar 2015, Dhere, Chaitanya (C.) wrote:
> 
> > This patch replaces kzalloc and copy_from_user with memdup_user call
> > This change was detected with coccinelle tool
> >
> > Signed-off-by: Chaitanya Dhere <cvijaydh@visteon.com>
> > ---
> >  drivers/staging/lustre/lustre/llite/file.c |   11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> > index 85e74d1..85b5567 100644
> > --- a/drivers/staging/lustre/lustre/llite/file.c
> > +++ b/drivers/staging/lustre/lustre/llite/file.c
> > @@ -2368,14 +2368,9 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >  		struct hsm_state_set	*hss;
> >  		int			 rc;
> >
> > -		hss = kzalloc(sizeof(*hss), GFP_NOFS);
> > -		if (!hss)
> > -			return -ENOMEM;
> > -
> > -		if (copy_from_user(hss, (char *)arg, sizeof(*hss))) {
> > -			OBD_FREE_PTR(hss);
> > -			return -EFAULT;
> > -		}
> > +		hss = memdup_user((char *)arg, sizeof(*hss));
> 
> memdup_user will use the flag GFP_KERNEL, ie (__GFP_WAIT | __GFP_IO |
> __GFP_FS), rather than the flag GFP_NOFS, ie (__GFP_WAIT | __GFP_IO), that
> is specified.  I don't know if this is a problem here.

Yes, this is a filesystem, so this can't be changed, as we can't have
the allocation go out and ask for more filesystem accesses in the middle
of trying to do a filesystem access :)

thanks,

greg k-h

Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[Drokin, Oleg]

On Mar 31, 2015, at 11:57 AM, gregkh@linuxfoundation.org wrote:

> On Tue, Mar 31, 2015 at 05:15:23PM +0200, Julia Lawall wrote:
>> On Tue, 31 Mar 2015, Dhere, Chaitanya (C.) wrote:
>> 
>>> This patch replaces kzalloc and copy_from_user with memdup_user call
>>> This change was detected with coccinelle tool
>>> 
>>> Signed-off-by: Chaitanya Dhere <cvijaydh@visteon.com>
>>> ---
>>> drivers/staging/lustre/lustre/llite/file.c |   11 +++--------
>>> 1 file changed, 3 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
>>> index 85e74d1..85b5567 100644
>>> --- a/drivers/staging/lustre/lustre/llite/file.c
>>> +++ b/drivers/staging/lustre/lustre/llite/file.c
>>> @@ -2368,14 +2368,9 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>> 		struct hsm_state_set	*hss;
>>> 		int			 rc;
>>> 
>>> -		hss = kzalloc(sizeof(*hss), GFP_NOFS);
>>> -		if (!hss)
>>> -			return -ENOMEM;
>>> -
>>> -		if (copy_from_user(hss, (char *)arg, sizeof(*hss))) {
>>> -			OBD_FREE_PTR(hss);
>>> -			return -EFAULT;
>>> -		}
>>> +		hss = memdup_user((char *)arg, sizeof(*hss));
>> 
>> memdup_user will use the flag GFP_KERNEL, ie (__GFP_WAIT | __GFP_IO |
>> __GFP_FS), rather than the flag GFP_NOFS, ie (__GFP_WAIT | __GFP_IO), that
>> is specified.  I don't know if this is a problem here.
> 
> Yes, this is a filesystem, so this can't be changed, as we can't have
> the allocation go out and ask for more filesystem accesses in the middle
> of trying to do a filesystem access :)

Technically in this place we are not really holding any locks or anything else of value to cause a deadlock,
so we might be fine here.
More importantly, I totally missed this OBD_ALLOC replacement with kzalloc when it happened.
In theory all OBD_ALLOC() calls add up all allocated memory in a counter and then OBD_FREE() calls
subtract freed memory (for a poor man's memory leak detection and tracing).
Now since it's out of match, there should have been tons of very loud warnings about it, but I don't see
any in my logs and I wonder why.

Julia, I wonder if you happen to have a bunch of other patches to get rid of the rest of OBD_ALLOC and OBD_FREE stuff by any chance?

Bye,
    Oleg

Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[Julia Lawall]

On Tue, 31 Mar 2015, Drokin, Oleg wrote:

>
> On Mar 31, 2015, at 11:57 AM, gregkh@linuxfoundation.org wrote:
>
> > On Tue, Mar 31, 2015 at 05:15:23PM +0200, Julia Lawall wrote:
> >> On Tue, 31 Mar 2015, Dhere, Chaitanya (C.) wrote:
> >>
> >>> This patch replaces kzalloc and copy_from_user with memdup_user call
> >>> This change was detected with coccinelle tool
> >>>
> >>> Signed-off-by: Chaitanya Dhere <cvijaydh@visteon.com>
> >>> ---
> >>> drivers/staging/lustre/lustre/llite/file.c |   11 +++--------
> >>> 1 file changed, 3 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
> >>> index 85e74d1..85b5567 100644
> >>> --- a/drivers/staging/lustre/lustre/llite/file.c
> >>> +++ b/drivers/staging/lustre/lustre/llite/file.c
> >>> @@ -2368,14 +2368,9 @@ ll_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >>> 		struct hsm_state_set	*hss;
> >>> 		int			 rc;
> >>>
> >>> -		hss = kzalloc(sizeof(*hss), GFP_NOFS);
> >>> -		if (!hss)
> >>> -			return -ENOMEM;
> >>> -
> >>> -		if (copy_from_user(hss, (char *)arg, sizeof(*hss))) {
> >>> -			OBD_FREE_PTR(hss);
> >>> -			return -EFAULT;
> >>> -		}
> >>> +		hss = memdup_user((char *)arg, sizeof(*hss));
> >>
> >> memdup_user will use the flag GFP_KERNEL, ie (__GFP_WAIT | __GFP_IO |
> >> __GFP_FS), rather than the flag GFP_NOFS, ie (__GFP_WAIT | __GFP_IO), that
> >> is specified.  I don't know if this is a problem here.
> >
> > Yes, this is a filesystem, so this can't be changed, as we can't have
> > the allocation go out and ask for more filesystem accesses in the middle
> > of trying to do a filesystem access :)
>
> Technically in this place we are not really holding any locks or anything else of value to cause a deadlock,
> so we might be fine here.
> More importantly, I totally missed this OBD_ALLOC replacement with kzalloc when it happened.
> In theory all OBD_ALLOC() calls add up all allocated memory in a counter and then OBD_FREE() calls
> subtract freed memory (for a poor man's memory leak detection and tracing).
> Now since it's out of match, there should have been tons of very loud warnings about it, but I don't see
> any in my logs and I wonder why.
>
> Julia, I wonder if you happen to have a bunch of other patches to get rid of the rest of OBD_ALLOC and OBD_FREE stuff by any chance?

I can generate them again, but I wasn't clear on what was wanted.  I would
really prefer something where it is explicit at the call site that an
assignment is taking place.  If we can have x = obd_alloc(...) and
obd_free(x,...) (I don't have time to look up the exact arguments at the
moment), then I can take care of that).  I still think it is too bad that
this code won't benefit from rules written for more generic memory
allocation functions, but if the extra debugging facility provided by
these functions is useful, then I guess it is reasonable to keep it.

julia

Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[Drokin, Oleg]

Hello!

On Apr 2, 2015, at 6:18 AM, Julia Lawall wrote:

>> Julia, I wonder if you happen to have a bunch of other patches to get rid of the rest of OBD_ALLOC and OBD_FREE stuff by any chance?
> I can generate them again, but I wasn't clear on what was wanted.  I would
> really prefer something where it is explicit at the call site that an
> assignment is taking place.  If we can have x = obd_alloc(...) and
> obd_free(x,...) (I don't have time to look up the exact arguments at the
> moment), then I can take care of that).  I still think it is too bad that
> this code won't benefit from rules written for more generic memory
> allocation functions, but if the extra debugging facility provided by
> these functions is useful, then I guess it is reasonable to keep it.

Like I mentioned sometime last year - it's now pretty easy to replace the memleak
detection with other in-kernel mechanisms some of which are in fact even better
than what we have. And considering our mechanisms are totally broken now by the mixup of
wrapped vs nonwrapped allocation/freeing - there's no point in holding to it remaining at all.
The only last bit of useful functionality left, I imagine, is the ability to redirect allocation
to regular kmalloc or to vmalloc based on the allocation size (there's kvfree already for the
freeing part of it).
Other than that the wrappers could go away at any time now, I think.

Bye,
    Oleg

Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user

[Julia Lawall]

On Fri, 3 Apr 2015, Drokin, Oleg wrote:

> Hello!
> 
> On Apr 2, 2015, at 6:18 AM, Julia Lawall wrote:
> 
> >> Julia, I wonder if you happen to have a bunch of other patches to get rid of the rest of OBD_ALLOC and OBD_FREE stuff by any chance?
> > I can generate them again, but I wasn't clear on what was wanted.  I would
> > really prefer something where it is explicit at the call site that an
> > assignment is taking place.  If we can have x = obd_alloc(...) and
> > obd_free(x,...) (I don't have time to look up the exact arguments at the
> > moment), then I can take care of that).  I still think it is too bad that
> > this code won't benefit from rules written for more generic memory
> > allocation functions, but if the extra debugging facility provided by
> > these functions is useful, then I guess it is reasonable to keep it.
> 
> Like I mentioned sometime last year - it's now pretty easy to replace the memleak
> detection with other in-kernel mechanisms some of which are in fact even better
> than what we have. And considering our mechanisms are totally broken now by the mixup of
> wrapped vs nonwrapped allocation/freeing - there's no point in holding to it remaining at all.
> The only last bit of useful functionality left, I imagine, is the ability to redirect allocation
> to regular kmalloc or to vmalloc based on the allocation size (there's kvfree already for the
> freeing part of it).
> Other than that the wrappers could go away at any time now, I think.

OK, thanks.

julia