From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753042AbbDBKSW (ORCPT ); Thu, 2 Apr 2015 06:18:22 -0400 Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:26951 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752894AbbDBKSF (ORCPT ); Thu, 2 Apr 2015 06:18:05 -0400 X-IronPort-AV: E=Sophos;i="5.11,510,1422918000"; d="scan'208";a="108038436" Date: Thu, 2 Apr 2015 12:18:03 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: "Drokin, Oleg" cc: Julia Lawall , Greg Kroah-Hartman , "Dhere, Chaitanya (C.)" , "Dilger, Andreas" , Al Viro , "Xiong, Jinshan" , "aybuke.147@gmail.com" , "Hammond, John" , "HPDD-discuss@lists.01.org" , "" , " Mailing List" Subject: Re: [PATCH] staging: lustre: replace kzalloc with copy_from_user with memdup_user In-Reply-To: Message-ID: References: <20150331150933.GA2833@chaitanya-Ideapad-Z560> <20150331155756.GA12842@kroah.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>> --- > >>> 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