Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756819AbcKKPj4 (ORCPT ); Fri, 11 Nov 2016 10:39:56 -0500 Received: from h2.hallyn.com ([78.46.35.8]:49282 "EHLO h2.hallyn.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754181AbcKKPjz (ORCPT ); Fri, 11 Nov 2016 10:39:55 -0500 Date: Fri, 11 Nov 2016 09:40:00 -0600 From: "Serge E. Hallyn" To: Sachin Shukla Cc: "Eric W. Biederman" , Kees Cook , Serge Hallyn , Andrey Vagin , linux-kernel@vger.kernel.org, sachiniiitm@gmail.com, ravikant.s2@samsung.com, p.shailesh@samsung.com, ashish.kalra@samsung.com, vidushi.koul@samsung.com Subject: Re: [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails. Message-ID: <20161111154000.GA30539@mail.hallyn.com> References: <1478855235-26233-1-git-send-email-sachin.s5@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1478855235-26233-1-git-send-email-sachin.s5@samsung.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2269 Lines: 74 On Fri, Nov 11, 2016 at 02:37:15PM +0530, Sachin Shukla wrote: > From: "Sachin Shukla" > > There is no need to call kfree() if memdup_user_nul() fails, as no memory > was allocated and the error in the error-valued pointer should be returned. Hi, in general, having a common exit path is considered more readable, more easily reviewable, than having more exit paths. To this end, initializing pointers to NULL and kfree()ing them at common exit paths even when they may not have been alloc()ed yet is also often seen as more readable. > Signed-off-by: Sachin Shukla I do appreciate the work, and I recognize these things can be subjective, but I would say Nacked-by: Serge Hallyn > --- > kernel/user_namespace.c | 25 ++++++++++++++----------- > 1 file changed, 14 insertions(+), 11 deletions(-) > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > index 86b7854..a0ffbf0 100644 > --- a/kernel/user_namespace.c > +++ b/kernel/user_namespace.c > @@ -672,28 +672,31 @@ static ssize_t map_write(struct file *file, const char __user *buf, > */ > mutex_lock(&userns_state_mutex); > > - ret = -EPERM; > /* Only allow one successful write to the map */ > - if (map->nr_extents != 0) > - goto out; > + if (map->nr_extents != 0) { > + mutex_unlock(&userns_state_mutex); > + return -EPERM; > + } > > /* > * Adjusting namespace settings requires capabilities on the target. > */ > - if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN)) > - goto out; > + if (cap_valid(cap_setid) && !file_ns_capable(file, ns, CAP_SYS_ADMIN)) { > + mutex_unlock(&userns_state_mutex); > + return -EPERM; > + } > > /* Only allow < page size writes at the beginning of the file */ > - ret = -EINVAL; > - if ((*ppos != 0) || (count >= PAGE_SIZE)) > - goto out; > + if ((*ppos != 0) || (count >= PAGE_SIZE)) { > + mutex_unlock(&userns_state_mutex); > + return -EINVAL; > + } > > /* Slurp in the user data */ > kbuf = memdup_user_nul(buf, count); > if (IS_ERR(kbuf)) { > - ret = PTR_ERR(kbuf); > - kbuf = NULL; > - goto out; > + mutex_unlock(&userns_state_mutex); > + return PTR_ERR(kbuf); > } > > /* Parse the user data */ > -- > 1.7.9.5