Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753358AbcKKKC3 (ORCPT ); Fri, 11 Nov 2016 05:02:29 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:52674 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751161AbcKKKC0 (ORCPT ); Fri, 11 Nov 2016 05:02:26 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 83CE0622E0 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=vivek.gautam@codeaurora.org MIME-Version: 1.0 In-Reply-To: <1478855235-26233-1-git-send-email-sachin.s5@samsung.com> References: <1478855235-26233-1-git-send-email-sachin.s5@samsung.com> From: Vivek Gautam Date: Fri, 11 Nov 2016 15:32:23 +0530 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails. 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 Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2516 Lines: 71 On Fri, Nov 11, 2016 at 2:37 PM, 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. > > Signed-off-by: Sachin Shukla > --- > 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); > } you may as well just move kfree() to a new error label, and let 'out' do only mutex_unlock() and return ret. Also remove the initialization of ret variable at the time of its declaration. That doesn't make sense, since ret is initialized to new values in this function anyways. Thanks Vivek -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project