LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
@ 2015-03-31 15:10 Dhere, Chaitanya (C.)
  2015-03-31 15:15 ` Julia Lawall
  0 siblings, 1 reply; 7+ messages in thread
From: Dhere, Chaitanya (C.) @ 2015-03-31 15:10 UTC (permalink / raw)
  To: oleg.drokin, andreas.dilger, gregkh, viro, jinshan.xiong,
	aybuke.147, john.hammond, Julia.Lawall
  Cc: HPDD-discuss@lists.01.org, devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
  2015-03-31 15:10 [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user Dhere, Chaitanya (C.)
@ 2015-03-31 15:15 ` Julia Lawall
  2015-03-31 15:57   ` gregkh
  0 siblings, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2015-03-31 15:15 UTC (permalink / raw)
  To: Dhere, Chaitanya (C.)
  Cc: oleg.drokin, andreas.dilger, gregkh, viro, jinshan.xiong,
	aybuke.147, john.hammond, Julia.Lawall,
	HPDD-discuss@lists.01.org, devel, linux-kernel

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
  2015-03-31 15:15 ` Julia Lawall
@ 2015-03-31 15:57   ` gregkh
  2015-03-31 22:45     ` Drokin, Oleg
  0 siblings, 1 reply; 7+ messages in thread
From: gregkh @ 2015-03-31 15:57 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Dhere, Chaitanya (C.),
	oleg.drokin, andreas.dilger, viro, jinshan.xiong, aybuke.147,
	john.hammond, HPDD-discuss@lists.01.org, devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
  2015-03-31 15:57   ` gregkh
@ 2015-03-31 22:45     ` Drokin, Oleg
  2015-04-02 10:18       ` Julia Lawall
  0 siblings, 1 reply; 7+ messages in thread
From: Drokin, Oleg @ 2015-03-31 22:45 UTC (permalink / raw)
  To: Julia Lawall, Greg Kroah-Hartman
  Cc: Dhere, Chaitanya (C.),
	Dilger, Andreas, Al Viro, Xiong, Jinshan, aybuke.147, Hammond,
	John, HPDD-discuss@lists.01.org,
	<devel@driverdev.osuosl.org>,
	<linux-kernel@vger.kernel.org> Mailing List


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
  2015-03-31 22:45     ` Drokin, Oleg
@ 2015-04-02 10:18       ` Julia Lawall
  2015-04-03 18:56         ` Drokin, Oleg
  0 siblings, 1 reply; 7+ messages in thread
From: Julia Lawall @ 2015-04-02 10:18 UTC (permalink / raw)
  To: Drokin, Oleg
  Cc: Julia Lawall, Greg Kroah-Hartman, Dhere, Chaitanya (C.),
	Dilger, Andreas, Al Viro, Xiong, Jinshan, aybuke.147, Hammond,
	John, HPDD-discuss@lists.01.org,
	<devel@driverdev.osuosl.org>,
	<linux-kernel@vger.kernel.org> Mailing List



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
  2015-04-02 10:18       ` Julia Lawall
@ 2015-04-03 18:56         ` Drokin, Oleg
  2015-04-03 19:51           ` Julia Lawall
  0 siblings, 1 reply; 7+ messages in thread
From: Drokin, Oleg @ 2015-04-03 18:56 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Greg Kroah-Hartman, Dhere, Chaitanya (C.),
	Dilger, Andreas, Al Viro, Xiong, Jinshan, aybuke.147, Hammond,
	John, HPDD-discuss@lists.01.org,
	<devel@driverdev.osuosl.org>,
	<linux-kernel@vger.kernel.org> Mailing List

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user
  2015-04-03 18:56         ` Drokin, Oleg
@ 2015-04-03 19:51           ` Julia Lawall
  0 siblings, 0 replies; 7+ messages in thread
From: Julia Lawall @ 2015-04-03 19:51 UTC (permalink / raw)
  To: Drokin, Oleg
  Cc: Julia Lawall, Greg Kroah-Hartman, Dhere, Chaitanya (C.),
	Dilger, Andreas, Al Viro, Xiong, Jinshan, aybuke.147, Hammond,
	John, HPDD-discuss@lists.01.org,
	<devel@driverdev.osuosl.org>,
	<linux-kernel@vger.kernel.org> Mailing List



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-04-03 19:51 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-31 15:10 [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user Dhere, Chaitanya (C.)
2015-03-31 15:15 ` Julia Lawall
2015-03-31 15:57   ` gregkh
2015-03-31 22:45     ` Drokin, Oleg
2015-04-02 10:18       ` Julia Lawall
2015-04-03 18:56         ` Drokin, Oleg
2015-04-03 19:51           ` Julia Lawall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).