* Re: [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails.
2016-11-11 9:07 [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails Sachin Shukla
@ 2016-11-11 10:02 ` Vivek Gautam
2016-11-11 15:40 ` Serge E. Hallyn
2016-11-11 16:55 ` Eric W. Biederman
2 siblings, 0 replies; 4+ messages in thread
From: Vivek Gautam @ 2016-11-11 10:02 UTC (permalink / raw)
To: Sachin Shukla
Cc: Eric W. Biederman, Kees Cook, Serge Hallyn, Andrey Vagin,
linux-kernel@vger.kernel.org, sachiniiitm, ravikant.s2,
p.shailesh, ashish.kalra, vidushi.koul
On Fri, Nov 11, 2016 at 2:37 PM, Sachin Shukla <sachin.s5@samsung.com> wrote:
> From: "Sachin Shukla" <sachin.s5@samsung.com>
>
> 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 <sachin.s5@samsung.com>
> ---
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails.
2016-11-11 9:07 [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails Sachin Shukla
2016-11-11 10:02 ` Vivek Gautam
@ 2016-11-11 15:40 ` Serge E. Hallyn
2016-11-11 16:55 ` Eric W. Biederman
2 siblings, 0 replies; 4+ messages in thread
From: Serge E. Hallyn @ 2016-11-11 15:40 UTC (permalink / raw)
To: Sachin Shukla
Cc: Eric W. Biederman, Kees Cook, Serge Hallyn, Andrey Vagin,
linux-kernel, sachiniiitm, ravikant.s2, p.shailesh, ashish.kalra,
vidushi.koul
On Fri, Nov 11, 2016 at 02:37:15PM +0530, Sachin Shukla wrote:
> From: "Sachin Shukla" <sachin.s5@samsung.com>
>
> 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 <sachin.s5@samsung.com>
I do appreciate the work, and I recognize these things can be subjective,
but I would say
Nacked-by: Serge Hallyn <serge@hallyn.com>
> ---
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails.
2016-11-11 9:07 [PATCH] Kernel: Improvement in code readability when memdup_user_nul() fails Sachin Shukla
2016-11-11 10:02 ` Vivek Gautam
2016-11-11 15:40 ` Serge E. Hallyn
@ 2016-11-11 16:55 ` Eric W. Biederman
2 siblings, 0 replies; 4+ messages in thread
From: Eric W. Biederman @ 2016-11-11 16:55 UTC (permalink / raw)
To: Sachin Shukla
Cc: Kees Cook, Serge Hallyn, Andrey Vagin, linux-kernel, sachiniiitm,
ravikant.s2, p.shailesh, ashish.kalra, vidushi.koul
Sachin Shukla <sachin.s5@samsung.com> writes:
> From: "Sachin Shukla" <sachin.s5@samsung.com>
>
> 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.
Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
This is stupid, unnecesary gratituous change that makes the maintenance
of the code worse.
>
> Signed-off-by: Sachin Shukla <sachin.s5@samsung.com>
> ---
> 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 */
^ permalink raw reply [flat|nested] 4+ messages in thread