Message-ID: <2025-08-08.1754667211-eroded-secure-codex-earthworm-EK9kTK@cyphar.com>
Date: Sat, 9 Aug 2025 01:51:44 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Askar Safin <safinaskar@...omail.com>, amir73il@...il.com, 
	corbet@....net, jack@...e.cz, linux-api@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, luto@...capital.net, shuah@...nel.org, viro@...iv.linux.org.uk
Subject: Re: [PATCH v4 2/4] procfs: add "pidns" mount option

On 2025-08-08, Christian Brauner <brauner@...nel.org> wrote:
> On Thu, Aug 07, 2025 at 05:17:56PM +1000, Aleksa Sarai wrote:
> > On 2025-08-07, Aleksa Sarai <cyphar@...har.com> wrote:
> > > On 2025-08-06, Askar Safin <safinaskar@...omail.com> wrote:
> > > > > I just realised that we probably also want to support FSCONFIG_SET_PATH
> > > > 
> > > > I just checked kernel code. Indeed nobody uses FSCONFIG_SET_PATH.
> > > > Moreover, fsparam_path macro is present since 5.1. And for all this
> > > > time nobody used it. So, let's just remove FSCONFIG_SET_PATH. Nobody
> > > > used it, so this will not break anything.
> > > > 
> > > > If you okay with that, I can submit patch, removing it.
> > > 
> > > I would prefer you didn't -- "*at()" semantics are very useful to a lot
> > > of programs (*especially* AT_EMPTY_PATH). I would like the pidns= stuff
> > > to support it, and probably also overlayfs...
> > > 
> > > I suspect the primary issue is that when migrating to the new mount API,
> > > filesystem devs just went with the easiest thing to use
> > > (FSCONFIG_SET_STRING) even though FSCONFIG_SET_PATH would be better. I
> > > suspect the lack of documentation around fsconfig(2) played a part too.
> > > 
> > > My impression is that interest in the minutia about fsconfig(2) is quite
> > > low on the list of priorities for most filesystem devs, and so the neat
> > > aspects of fsconfig(2) haven't been fully utilised. (In LPC last year,
> > > we struggled to come to an agreement on how filesystems should use the
> > > read(2)-based error interface.)
> > > 
> > > We can very easily move fsparam_string() or fsparam_file_or_string()
> > > parameters to fsparam_path() and a future fsparam_file_or_path(). I
> > > would much prefer that as a user.
> > 
> > Actually, fsparam_bdev() accepts FSCONFIG_SET_PATH in a very roundabout
> > way (and the checker doesn't verify anything...?). So there is at least
> > one user (ext4's "journal_path"), it's just not well-documented (which
> > I'm trying to fix ;]).
> > 
> > My plan is to update fs_lookup_param() to be more useful for the (fairly
> > common) use-case of wanting to support paths and file descriptors, and
> > going through to clean up some of these unused fsparam_* helpers (or
> > fsparam_* helpers being abused to implement stuff that the fs_parser
> > core already supports).
> > 
> > At the very least, overlayfs, ext4, and this procfs patchset can make
> > use of it.
> 
> I've never bothered with actually iplementing FSCONFIG_SET_PATH
> semantics because I think it's really weird to allow *at semantics when
> setting filesystem parameters. I always thought it's better to force
> userspace to provide a file descriptor for the final destination instead
> of doing some arcane lookup variant for mount configuration. But I'm
> happy to be convinced of its usefulness...

I do think it's useful, and here's my thought process...

Most filesystems have to take string path parameters in order to support
mount(2) and work with mount(8). Yes, fsparam_fd() will accept
FSCONFIG_SET_STRING by parsing it as a decimal string, but there are
only two users of fsparam_fd() and honestly I'm not convinced this is a
particularly sane API for anything other than strict backcompat reasons
(the API only makes sense as a file descriptor and you want mount(8) to
be able to use it).

So you end up with most parameters supporting paths set using
FSCONFIG_SET_STRING anyway, meaning in-kernel lookups can't be taken off
the table. And if we accept paths for lookup, then (for the same reason
we have *at(2) syscalls) it is preferable to allow specifying dirfds. So
FSCONFIG_SET_PATH should also be supported.

And as there is no infrastructure to block FSCONFIG_SET_PATH_EMPTY
arguments (yes, you can do it manually, but the *only* user of
fs_lookup_param() doesn't), then anything that accepts FSCONFIG_SET_PATH
currently also accepts FSCONFIG_SET_PATH_EMPTY which is "morally
equivalent" to FSCONFIG_SET_FD. So unless you block
FSCONFIG_SET_PATH_EMPTY then FSCONFIG_SET_FD should probably also be
supported (there is the re-opening distinction, of course, but that is
not relevant if you use filename_lookup() -- which is what filesystems
will do in practice).

So my impression is that most users (if they had an fsconfig(2) man page
to read...) would expect parameters that accept paths to either:

* Work with FSCONFIG_SET_STRING and FSCONFIG_SET_PATH only; or
* Work with FSCONFIG_SET_STRING, FSCONFIG_SET_PATH,
  FSCONFIG_SET_PATH_EMPTY, and FSCONFIG_SET_FD.

Currently, none of our parameters work that way.

 * ext4's journal_path takes FSCONFIG_SET_STRING, FSCONFIG_SET_PATH, and
   FSCONFIG_SET_PATH_EMPTY.
 * overlayfs takes FSCONFIG_SET_FD and FSCONFIG_SET_STRING.

I only fully realised how inconsistent this is while working on the
fsconfig(2) man pages -- at the moment I have a very long paragraph
explaining that there is this distinction in-kernel, but this really
doesn't seem intentional to me. I would be very confused as a user that
FSCONFIG_SET_PATH is useless for most filesystem *path* parameters, even
though the filesystem accepts them as FSCONFIG_SET_STRING.

As for practical uses, it would be nice to not have to open 500 files in
order to create a 500-layer overlayfs.

-- 
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