Message-ID: <Zv1OayMEmLP2kjhj@kawka3.in.waw.pl>
Date: Wed, 2 Oct 2024 13:45:15 +0000
From: Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>
To: Tycho Andersen <tycho@...ho.pizza>
Cc: Aleksa Sarai <cyphar@...har.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>,
	linux-fsdevel@...r.kernel.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
	Tycho Andersen <tandersen@...flix.com>
Subject: Re: [PATCH v3 1/2] exec: fix up /proc/pid/comm in the
 execveat(AT_EMPTY_PATH) case

On Tue, Oct 01, 2024 at 08:42:56PM +0200, Aleksa Sarai wrote:
> On 2024-10-01, Tycho Andersen <tycho@...ho.pizza> wrote:
> > From: Tycho Andersen <tandersen@...flix.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);

Powered by blists - more mailing lists