Message-ID: <20241106.141100-patchy.noises.kissable.cannons-37UAyhH88iH@cyphar.com>
Date: Thu, 7 Nov 2024 02:09:19 +1100
From: Aleksa Sarai <cyphar@...har.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Christian Brauner <brauner@...nel.org>, 
	Miklos Szeredi <miklos@...redi.hu>, Jan Kara <jack@...e.cz>, linux-unionfs@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Karel Zak <kzak@...hat.com>, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] overlayfs: port all superblock creation logging to
 fsopen logs

On 2024-11-06, Amir Goldstein <amir73il@...il.com> wrote:
> On Wed, Nov 6, 2024 at 12:00 PM Amir Goldstein <amir73il@...il.com> wrote:
> >
> > On Wed, Nov 6, 2024 at 10:59 AM Christian Brauner <brauner@...nel.org> wrote:
> > >
> > > On Wed, Nov 06, 2024 at 02:09:58PM +1100, Aleksa Sarai wrote:
> > > > overlayfs helpfully provides a lot of of information when setting up a
> > > > mount, but unfortunately when using the fsopen(2) API, a lot of this
> > > > information is mixed in with the general kernel log.
> > > >
> > > > In addition, some of the logs can become a source of spam if programs
> > > > are creating many internal overlayfs mounts (in runc we use an internal
> > > > overlayfs mount to protect the runc binary against container breakout
> > > > attacks like CVE-2019-5736, and xino_auto=on caused a lot of spam in
> > > > dmesg because we didn't explicitly disable xino[1]).
> > > >
> > > > By logging to the fs_context, userspace can get more accurate
> > > > information when using fsopen(2) and there is less dmesg spam for
> > > > systems where a lot of programs are using fsopen("overlay"). Legacy
> > > > mount(2) users will still see the same errors in dmesg as they did
> > > > before (though the prefix of the log messages will now be "overlay"
> > > > rather than "overlayfs").
> >
> > I am not sure about the level of risk in this format change.
> > Miklos, WDYT?
> >
> > > >
> > > > [1]: https://bbs.archlinux.org/viewtopic.php?pid=2206551
> > > >
> > > > Signed-off-by: Aleksa Sarai <cyphar@...har.com>
> > > > ---
> > >
> > > To me this sounds inherently useful! So I'm all for it.
> > >
> >
> > [CC: Karel]
> >
> > I am quite concerned about this.
> > I have a memory that Christian suggested to make this change back in
> > the original conversion to new mount API, but back then mount tool
> > did not print out the errors to users properly and even if it does
> > print out errors, some script could very well be ignoring them.

I think Christian mentioned this at LSF/MM (or maybe LPC), but it seems
that util-linux does provide the log information now in the case of
fsconfig(2) errors:

	% strace -e fsopen,fsconfig mount -t overlay -o userxattr=str x /tmp/a
	fsopen("overlay", FSOPEN_CLOEXEC)       = 3
	fsconfig(3, FSCONFIG_SET_STRING, "source", "foo", 0) = 0
	fsconfig(3, FSCONFIG_SET_STRING, "userxattr", "str", 0) = -1 EINVAL (Invalid argument)
	mount: /tmp/a: fsconfig system call failed: overlay: Unexpected value for 'userxattr'.
		   dmesg(1) may have more information after failed mount system call.

(Using the current HEAD of util-linux -- openSUSE's util-linux isn't
compiled with support for fsopen apparently.)

However, it doesn't output any of the info-level ancillary information
if there were no errors. So there will definitely be some loss of
information for pr_* logs that don't cause an actual error (which is a
little unfortunate, since that is the exact dmesg spam that caused me to
write this patch).

I could take a look at sending a patch to get libmount to output that
information, but that won't help with the immediate issue, and this
doesn't help with the possible concern with some script that scrapes
dmesg. (Though I think it goes without saying that such scripts are kind
of broken by design -- since unprivileged users can create overlayfs
mounts and thus spam the kernel log with any message, there is no
practical way for a script to correctly get the right log information
without using the new mount API's logging facilities.)

I can adjust this patch to only include the log+return-an-error cases,
but that doesn't really address your primary concern, I guess.

> > My strong feeling is that suppressing legacy errors to kmsg should be opt-in
> > via the new mount API and that it should not be the default for libmount.
> > IMO, it is certainly NOT enough that new mount API is used by userspace
> > as an indication for the kernel to suppress errors to kmsg.

I can see an argument for some kind of MS_SILENT analogue for
fsconfig(), though it will make the spam problem worse until programs
migrate to setting this new flag.

Also, as this is already an issue ever since libmount added support for
the new API (so since 2.39 I believe?), I think it would make just as
much sense for this flag to be opt-in -- so libmount could set the
"verbose" or "kmsglog" flag by default but most normal programs would
not get the spammy behaviour by default. If you're writing a custom
program using the new mount API, you almost certainly *don't* want dmesg
to get a bunch of messages that you would have no plan to ever parse.

> > I have no problem with reporting errors to both userspace and kmsg
> > without opt-in from usersapce.
> >
> > Furthermore, looking at the existing invalfc() calls in overlayfs, I see that
> > a few legacy pr_err() were converted to invalfc() with this commit
> > (signed off by myself):
> > 819829f0319a ovl: refactor layer parsing helpers
> >
> > I am not really sure if the discussion about suppressing the kmsg errors was
> > resolved or dismissed or maybe it only happened in my head??
> >
> > Thanks,
> > Amir.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists