diff mbox series

[v3,1/2] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case

Message ID 20241001134945.798662-1-tycho@tycho.pizza (mailing list archive)
State New
Headers show
Series [v3,1/2] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY_PATH) case | expand

Commit Message

Tycho Andersen Oct. 1, 2024, 1:49 p.m. UTC 
From: Tycho Andersen <tandersen@netflix.com>

Zbigniew mentioned at Linux Plumber's that systemd is interested in
switching to execveat() for service execution, but can't, because the
contents of /proc/pid/comm are the file descriptor which was used,
instead of the path to the binary. This makes the output of tools like
top and ps useless, especially in a world where most fds are opened
CLOEXEC so the number is truly meaningless.

Change exec path to fix up /proc/pid/comm in the case where we have
allocated one of these synthetic paths in bprm_init(). This way the actual
exec machinery is unchanged, but cosmetically the comm looks reasonable to
admins investigating things.

Signed-off-by: Tycho Andersen <tandersen@netflix.com>
Suggested-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
CC: Aleksa Sarai <cyphar@cyphar.com>
Link: https://github.com/uapi-group/kernel-features#set-comm-field-before-exec
---
v2: * drop the flag, everyone :)
    * change the rendered value to f_path.dentry->d_name.name instead of
      argv[0], Eric
v3: * fix up subject line, Eric
---
 fs/exec.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)


base-commit: baeb9a7d8b60b021d907127509c44507539c15e5

Comments

Aleksa Sarai Oct. 1, 2024, 6:42 p.m. UTC | #1 
On 2024-10-01, Tycho Andersen <tycho@tycho.pizza> wrote:
> From: Tycho Andersen <tandersen@netflix.com>
> 
> Zbigniew mentioned at Linux Plumber's that systemd is interested in
> switching to execveat() for service execution, but can't, because the
> contents of /proc/pid/comm are the file descriptor which was used,
> instead of the path to the binary. This makes the output of tools like
> top and ps useless, especially in a world where most fds are opened
> CLOEXEC so the number is truly meaningless.
> 
> Change exec path to fix up /proc/pid/comm in the case where we have
> allocated one of these synthetic paths in bprm_init(). This way the actual
> exec machinery is unchanged, but cosmetically the comm looks reasonable to
> admins investigating things.

While I still think the argv[0] solution was semantically nicer, it
seems this is enough to fix the systemd problem for most cases and so we
can revisit the argv[0] discussion in another 10 years. :D

Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>

> Signed-off-by: Tycho Andersen <tandersen@netflix.com>
> Suggested-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> CC: Aleksa Sarai <cyphar@cyphar.com>
> Link: https://github.com/uapi-group/kernel-features#set-comm-field-before-exec
> ---
> v2: * drop the flag, everyone :)
>     * change the rendered value to f_path.dentry->d_name.name instead of
>       argv[0], Eric
> v3: * fix up subject line, Eric
> ---
>  fs/exec.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index dad402d55681..9520359a8dcc 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1416,7 +1416,18 @@ int begin_new_exec(struct linux_binprm * bprm)
>  		set_dumpable(current->mm, SUID_DUMP_USER);
>  
>  	perf_event_exec();
> -	__set_task_comm(me, kbasename(bprm->filename), true);
> +
> +	/*
> +	 * If fdpath was set, execveat() made up a path that will
> +	 * probably not be useful to admins running ps or similar.
> +	 * Let's fix it up to be something reasonable.
> +	 */
> +	if (bprm->fdpath) {
> +		BUILD_BUG_ON(TASK_COMM_LEN > DNAME_INLINE_LEN);
> +		__set_task_comm(me, bprm->file->f_path.dentry->d_name.name, true);
> +	} else {
> +		__set_task_comm(me, kbasename(bprm->filename), true);
> +	}
>  
>  	/* An exec changes our domain. We are no longer part of the thread
>  	   group */
> 
> base-commit: baeb9a7d8b60b021d907127509c44507539c15e5
> -- 
> 2.34.1
>
Aleksa Sarai Oct. 1, 2024, 6:48 p.m. UTC | #2 
On 2024-10-01, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2024-10-01, Tycho Andersen <tycho@tycho.pizza> wrote:
> > From: Tycho Andersen <tandersen@netflix.com>
> > 
> > Zbigniew mentioned at Linux Plumber's that systemd is interested in
> > switching to execveat() for service execution, but can't, because the
> > contents of /proc/pid/comm are the file descriptor which was used,
> > instead of the path to the binary. This makes the output of tools like
> > top and ps useless, especially in a world where most fds are opened
> > CLOEXEC so the number is truly meaningless.
> > 
> > Change exec path to fix up /proc/pid/comm in the case where we have
> > allocated one of these synthetic paths in bprm_init(). This way the actual
> > exec machinery is unchanged, but cosmetically the comm looks reasonable to
> > admins investigating things.
> 
> While I still think the argv[0] solution was semantically nicer, it
> seems this is enough to fix the systemd problem for most cases and so we
> can revisit the argv[0] discussion in another 10 years. :D

Of course, this assumes the busybox problem I mentioned really is not an
issue. But at least this option is "less wrong" than using the fd
number. I suspect we will eventually need the argv[0] thing.

> Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
> 
> > Signed-off-by: Tycho Andersen <tandersen@netflix.com>
> > Suggested-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> > CC: Aleksa Sarai <cyphar@cyphar.com>
> > Link: https://github.com/uapi-group/kernel-features#set-comm-field-before-exec
> > ---
> > v2: * drop the flag, everyone :)
> >     * change the rendered value to f_path.dentry->d_name.name instead of
> >       argv[0], Eric
> > v3: * fix up subject line, Eric
> > ---
> >  fs/exec.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/exec.c b/fs/exec.c
> > index dad402d55681..9520359a8dcc 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1416,7 +1416,18 @@ int begin_new_exec(struct linux_binprm * bprm)
> >  		set_dumpable(current->mm, SUID_DUMP_USER);
> >  
> >  	perf_event_exec();
> > -	__set_task_comm(me, kbasename(bprm->filename), true);
> > +
> > +	/*
> > +	 * If fdpath was set, execveat() made up a path that will
> > +	 * probably not be useful to admins running ps or similar.
> > +	 * Let's fix it up to be something reasonable.
> > +	 */
> > +	if (bprm->fdpath) {
> > +		BUILD_BUG_ON(TASK_COMM_LEN > DNAME_INLINE_LEN);
> > +		__set_task_comm(me, bprm->file->f_path.dentry->d_name.name, true);
> > +	} else {
> > +		__set_task_comm(me, kbasename(bprm->filename), true);
> > +	}
> >  
> >  	/* An exec changes our domain. We are no longer part of the thread
> >  	   group */
> > 
> > base-commit: baeb9a7d8b60b021d907127509c44507539c15e5
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Aleksa Sarai
> Senior Software Engineer (Containers)
> SUSE Linux GmbH
> <https://www.cyphar.com/>
Zbigniew Jędrzejewski-Szmek Oct. 2, 2024, 1:45 p.m. UTC | #3 
On Tue, Oct 01, 2024 at 08:42:56PM +0200, Aleksa Sarai wrote:
> On 2024-10-01, Tycho Andersen <tycho@tycho.pizza> wrote:
> > From: Tycho Andersen <tandersen@netflix.com>
> > 
> > Zbigniew mentioned at Linux Plumber's that systemd is interested in
> > switching to execveat() for service execution, but can't, because the
> > contents of /proc/pid/comm are the file descriptor which was used,
> > instead of the path to the binary. This makes the output of tools like
> > top and ps useless, especially in a world where most fds are opened
> > CLOEXEC so the number is truly meaningless.
> > 
> > Change exec path to fix up /proc/pid/comm in the case where we have
> > allocated one of these synthetic paths in bprm_init(). This way the actual
> > exec machinery is unchanged, but cosmetically the comm looks reasonable to
> > admins investigating things.
> 
> While I still think the argv[0] solution was semantically nicer, it
> seems this is enough to fix the systemd problem for most cases and so we
> can revisit the argv[0] discussion in another 10 years. :D

Hi Tycho and everyone else,

First, thank you so much for picking this up!
Second, sorry for being late with a reply…

Third, I tested the kernel with the patch and with systemd with the
fexecve option enabled, and it all works as expected.

Unfortunately, I don't think that the approach with
f_path.dentry->d_name.name can be used :(
As discussed previously, there are various places where symlinks
are used. Alternatives and busybox were raised. Some additional examples:
systemd (e.g. /usr/lib/systemd/systemd-udevd symlinks to /usr/bin/udevadm),
yum-builddep, yum-config-manager, yumdownloader, yum-groups-manager
all symlink to a multicall dnf-utils binary, and so on.

The question is whether "pgrep" and similar tools not getting the
expected name is a problem, and I think that the answer is,
unfortunately, "yes". Users will notice this. As a distro maintainer,
I would be _very_ wary of flipping this on in systemd, because there
certainly are scripts and other tools that use that logic to check if
things are running on the system. systemd uses cgroups and doesn't
care about COMM at all, and I expect that many other modern tools
won't either, but we have to take into account the long tail of local
admin scripts and older tools. To avoid regressions and complaints, we
really want an API that replaces the current execve invocations with
an fd-based approach but doesn't change how things otherwise look.
Arguably, the current patch would work great for 99% of cases, but
that's not enough.

(In particular, with rust being used more often for low level tools,
and the binaries being large because of the static linking, I expect
multicall binaries to become even more common. E.g. uutils-coreutils
that might become the default coreutils implementation in a few years.
It uses a multi-call binary and symlinks. Having 'coreutils' instead
of 'sleep' as COMM is not good.)

Please consider going back to the approach with argv[0].

Zbyszek

> > v2: * drop the flag, everyone :)
> >     * change the rendered value to f_path.dentry->d_name.name instead of
> >       argv[0], Eric

> > +		__set_task_comm(me, bprm->file->f_path.dentry->d_name.name, true);
Tycho Andersen Oct. 2, 2024, 2:09 p.m. UTC | #4 
On Wed, Oct 02, 2024 at 01:45:15PM +0000, Zbigniew Jędrzejewski-Szmek wrote:
> On Tue, Oct 01, 2024 at 08:42:56PM +0200, Aleksa Sarai wrote:
> > On 2024-10-01, Tycho Andersen <tycho@tycho.pizza> wrote:
> > > From: Tycho Andersen <tandersen@netflix.com>
> > > 
> > > Zbigniew mentioned at Linux Plumber's that systemd is interested in
> > > switching to execveat() for service execution, but can't, because the
> > > contents of /proc/pid/comm are the file descriptor which was used,
> > > instead of the path to the binary. This makes the output of tools like
> > > top and ps useless, especially in a world where most fds are opened
> > > CLOEXEC so the number is truly meaningless.
> > > 
> > > Change exec path to fix up /proc/pid/comm in the case where we have
> > > allocated one of these synthetic paths in bprm_init(). This way the actual
> > > exec machinery is unchanged, but cosmetically the comm looks reasonable to
> > > admins investigating things.
> > 
> > While I still think the argv[0] solution was semantically nicer, it
> > seems this is enough to fix the systemd problem for most cases and so we
> > can revisit the argv[0] discussion in another 10 years. :D
> 
...

> Unfortunately, I don't think that the approach with
> f_path.dentry->d_name.name can be used :(

hmm. Somehow earlier I had managed to convince myself that this gives
the right answer for symlinks too (instead of the original
kbasename(__d_path(file->f_path, root, buf, buflen)), but now upon
retesting it doesn't. So I agree, seems like the argv[0] hack is
needed unfortunately.

Tycho
diff mbox series

Patch

diff --git a/fs/exec.c b/fs/exec.c
index dad402d55681..9520359a8dcc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1416,7 +1416,18 @@  int begin_new_exec(struct linux_binprm * bprm)
 		set_dumpable(current->mm, SUID_DUMP_USER);
 
 	perf_event_exec();
-	__set_task_comm(me, kbasename(bprm->filename), true);
+
+	/*
+	 * If fdpath was set, execveat() made up a path that will
+	 * probably not be useful to admins running ps or similar.
+	 * Let's fix it up to be something reasonable.
+	 */
+	if (bprm->fdpath) {
+		BUILD_BUG_ON(TASK_COMM_LEN > DNAME_INLINE_LEN);
+		__set_task_comm(me, bprm->file->f_path.dentry->d_name.name, true);
+	} else {
+		__set_task_comm(me, kbasename(bprm->filename), true);
+	}
 
 	/* An exec changes our domain. We are no longer part of the thread
 	   group */