* [PATCH -next] user_namespace: Replace gotos with return statements
@ 2018-04-19 2:46 Marcos Paulo de Souza
2018-04-19 3:43 ` Eric W. Biederman
2018-05-02 14:12 ` Christian Brauner
0 siblings, 2 replies; 4+ messages in thread
From: Marcos Paulo de Souza @ 2018-04-19 2:46 UTC (permalink / raw)
To: linux-next
Cc: Marcos Paulo de Souza, Eric W. Biederman, Christian Brauner,
Mark Rutland, Ingo Molnar, Serge Hallyn, Seth Forshee,
linux-kernel
Found while inspecting the code that handles the setgroups procfs file.
Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
---
Tested locally setting up a new userns, and setting setgroups as deny and allow,
worked as before.
kernel/user_namespace.c | 20 +++++++-------------
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 246d4d4ce5c7..64a01254ac6b 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -1142,22 +1142,18 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
struct user_namespace *ns = seq->private;
char kbuf[8], *pos;
bool setgroups_allowed;
- ssize_t ret;
/* Only allow a very narrow range of strings to be written */
- ret = -EINVAL;
if ((*ppos != 0) || (count >= sizeof(kbuf)))
- goto out;
+ return -EINVAL;
/* What was written? */
- ret = -EFAULT;
if (copy_from_user(kbuf, buf, count))
- goto out;
+ return -EFAULT;
kbuf[count] = '\0';
pos = kbuf;
/* What is being requested? */
- ret = -EINVAL;
if (strncmp(pos, "allow", 5) == 0) {
pos += 5;
setgroups_allowed = true;
@@ -1167,14 +1163,13 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
setgroups_allowed = false;
}
else
- goto out;
+ return -EINVAL;
/* Verify there is not trailing junk on the line */
pos = skip_spaces(pos);
if (*pos != '\0')
- goto out;
+ return -EINVAL;
- ret = -EPERM;
mutex_lock(&userns_state_mutex);
if (setgroups_allowed) {
/* Enabling setgroups after setgroups has been disabled
@@ -1194,12 +1189,11 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
/* Report a successful write */
*ppos = count;
- ret = count;
-out:
- return ret;
+ return count;
+
out_unlock:
mutex_unlock(&userns_state_mutex);
- goto out;
+ return -EPERM;
}
bool userns_may_setgroups(const struct user_namespace *ns)
--
2.14.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH -next] user_namespace: Replace gotos with return statements
2018-04-19 2:46 [PATCH -next] user_namespace: Replace gotos with return statements Marcos Paulo de Souza
@ 2018-04-19 3:43 ` Eric W. Biederman
2018-05-02 14:12 ` Christian Brauner
1 sibling, 0 replies; 4+ messages in thread
From: Eric W. Biederman @ 2018-04-19 3:43 UTC (permalink / raw)
To: Marcos Paulo de Souza
Cc: linux-next, Christian Brauner, Mark Rutland, Ingo Molnar,
Serge Hallyn, Seth Forshee, linux-kernel
Marcos Paulo de Souza <marcos.souza.org@gmail.com> writes:
> Found while inspecting the code that handles the setgroups procfs
> file.
What perchance might be the advantage of introducing multiple exits
into proc_setgroups_write?
I strongly suspect that if you look at the generated code it will
be worse after your patch.
Eric
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
> Tested locally setting up a new userns, and setting setgroups as deny and allow,
> worked as before.
>
> kernel/user_namespace.c | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 246d4d4ce5c7..64a01254ac6b 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1142,22 +1142,18 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> struct user_namespace *ns = seq->private;
> char kbuf[8], *pos;
> bool setgroups_allowed;
> - ssize_t ret;
>
> /* Only allow a very narrow range of strings to be written */
> - ret = -EINVAL;
> if ((*ppos != 0) || (count >= sizeof(kbuf)))
> - goto out;
> + return -EINVAL;
>
> /* What was written? */
> - ret = -EFAULT;
> if (copy_from_user(kbuf, buf, count))
> - goto out;
> + return -EFAULT;
> kbuf[count] = '\0';
> pos = kbuf;
>
> /* What is being requested? */
> - ret = -EINVAL;
> if (strncmp(pos, "allow", 5) == 0) {
> pos += 5;
> setgroups_allowed = true;
> @@ -1167,14 +1163,13 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> setgroups_allowed = false;
> }
> else
> - goto out;
> + return -EINVAL;
>
> /* Verify there is not trailing junk on the line */
> pos = skip_spaces(pos);
> if (*pos != '\0')
> - goto out;
> + return -EINVAL;
>
> - ret = -EPERM;
> mutex_lock(&userns_state_mutex);
> if (setgroups_allowed) {
> /* Enabling setgroups after setgroups has been disabled
> @@ -1194,12 +1189,11 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
>
> /* Report a successful write */
> *ppos = count;
> - ret = count;
> -out:
> - return ret;
> + return count;
> +
> out_unlock:
> mutex_unlock(&userns_state_mutex);
> - goto out;
> + return -EPERM;
> }
>
> bool userns_may_setgroups(const struct user_namespace *ns)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH -next] user_namespace: Replace gotos with return statements
2018-04-19 2:46 [PATCH -next] user_namespace: Replace gotos with return statements Marcos Paulo de Souza
2018-04-19 3:43 ` Eric W. Biederman
@ 2018-05-02 14:12 ` Christian Brauner
2018-05-04 1:23 ` Marcos Paulo de Souza
1 sibling, 1 reply; 4+ messages in thread
From: Christian Brauner @ 2018-05-02 14:12 UTC (permalink / raw)
To: Marcos Paulo de Souza
Cc: linux-next, Eric W. Biederman, Mark Rutland, Ingo Molnar,
Serge Hallyn, Seth Forshee, Linux Kernel Mailing List
On Wed, Apr 18, 2018 at 11:46:38PM -0300, Marcos Paulo de Souza wrote:
> Found while inspecting the code that handles the setgroups procfs file.
This is not really improving anything so I unfortunately don't really
see why we should take this.
Christian
>
> Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> ---
> Tested locally setting up a new userns, and setting setgroups as deny and allow,
> worked as before.
>
> kernel/user_namespace.c | 20 +++++++-------------
> 1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index 246d4d4ce5c7..64a01254ac6b 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -1142,22 +1142,18 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> struct user_namespace *ns = seq->private;
> char kbuf[8], *pos;
> bool setgroups_allowed;
> - ssize_t ret;
>
> /* Only allow a very narrow range of strings to be written */
> - ret = -EINVAL;
> if ((*ppos != 0) || (count >= sizeof(kbuf)))
> - goto out;
> + return -EINVAL;
>
> /* What was written? */
> - ret = -EFAULT;
> if (copy_from_user(kbuf, buf, count))
> - goto out;
> + return -EFAULT;
> kbuf[count] = '\0';
> pos = kbuf;
>
> /* What is being requested? */
> - ret = -EINVAL;
> if (strncmp(pos, "allow", 5) == 0) {
> pos += 5;
> setgroups_allowed = true;
> @@ -1167,14 +1163,13 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> setgroups_allowed = false;
> }
> else
> - goto out;
> + return -EINVAL;
>
> /* Verify there is not trailing junk on the line */
> pos = skip_spaces(pos);
> if (*pos != '\0')
> - goto out;
> + return -EINVAL;
>
> - ret = -EPERM;
> mutex_lock(&userns_state_mutex);
> if (setgroups_allowed) {
> /* Enabling setgroups after setgroups has been disabled
> @@ -1194,12 +1189,11 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
>
> /* Report a successful write */
> *ppos = count;
> - ret = count;
> -out:
> - return ret;
> + return count;
> +
> out_unlock:
> mutex_unlock(&userns_state_mutex);
> - goto out;
> + return -EPERM;
> }
>
> bool userns_may_setgroups(const struct user_namespace *ns)
> --
> 2.14.3
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH -next] user_namespace: Replace gotos with return statements
2018-05-02 14:12 ` Christian Brauner
@ 2018-05-04 1:23 ` Marcos Paulo de Souza
0 siblings, 0 replies; 4+ messages in thread
From: Marcos Paulo de Souza @ 2018-05-04 1:23 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-next, Eric W. Biederman, Mark Rutland, Ingo Molnar,
Serge Hallyn, Seth Forshee, Linux Kernel Mailing List
On Wed, May 02, 2018 at 04:12:18PM +0200, Christian Brauner wrote:
> On Wed, Apr 18, 2018 at 11:46:38PM -0300, Marcos Paulo de Souza wrote:
> > Found while inspecting the code that handles the setgroups procfs file.
>
> This is not really improving anything so I unfortunately don't really
> see why we should take this.
Sorry for the late reply. My idea was to make the code more straightforward,
never happened to me this would led to a worse code being generated. So, please
discard the patch.
Thanks,
>
> Christian
>
> >
> > Signed-off-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
> > ---
> > Tested locally setting up a new userns, and setting setgroups as deny and allow,
> > worked as before.
> >
> > kernel/user_namespace.c | 20 +++++++-------------
> > 1 file changed, 7 insertions(+), 13 deletions(-)
> >
> > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > index 246d4d4ce5c7..64a01254ac6b 100644
> > --- a/kernel/user_namespace.c
> > +++ b/kernel/user_namespace.c
> > @@ -1142,22 +1142,18 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> > struct user_namespace *ns = seq->private;
> > char kbuf[8], *pos;
> > bool setgroups_allowed;
> > - ssize_t ret;
> >
> > /* Only allow a very narrow range of strings to be written */
> > - ret = -EINVAL;
> > if ((*ppos != 0) || (count >= sizeof(kbuf)))
> > - goto out;
> > + return -EINVAL;
> >
> > /* What was written? */
> > - ret = -EFAULT;
> > if (copy_from_user(kbuf, buf, count))
> > - goto out;
> > + return -EFAULT;
> > kbuf[count] = '\0';
> > pos = kbuf;
> >
> > /* What is being requested? */
> > - ret = -EINVAL;
> > if (strncmp(pos, "allow", 5) == 0) {
> > pos += 5;
> > setgroups_allowed = true;
> > @@ -1167,14 +1163,13 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> > setgroups_allowed = false;
> > }
> > else
> > - goto out;
> > + return -EINVAL;
> >
> > /* Verify there is not trailing junk on the line */
> > pos = skip_spaces(pos);
> > if (*pos != '\0')
> > - goto out;
> > + return -EINVAL;
> >
> > - ret = -EPERM;
> > mutex_lock(&userns_state_mutex);
> > if (setgroups_allowed) {
> > /* Enabling setgroups after setgroups has been disabled
> > @@ -1194,12 +1189,11 @@ ssize_t proc_setgroups_write(struct file *file, const char __user *buf,
> >
> > /* Report a successful write */
> > *ppos = count;
> > - ret = count;
> > -out:
> > - return ret;
> > + return count;
> > +
> > out_unlock:
> > mutex_unlock(&userns_state_mutex);
> > - goto out;
> > + return -EPERM;
> > }
> >
> > bool userns_may_setgroups(const struct user_namespace *ns)
> > --
> > 2.14.3
> >
--
Thanks,
Marcos
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-04 1:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19 2:46 [PATCH -next] user_namespace: Replace gotos with return statements Marcos Paulo de Souza
2018-04-19 3:43 ` Eric W. Biederman
2018-05-02 14:12 ` Christian Brauner
2018-05-04 1:23 ` Marcos Paulo de Souza
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.