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

[Julia Lawall <julia.lawall@lip6.fr>]

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