On Tue, Jun 24, 2025 at 09:23:36PM +0200, Amir Goldstein wrote:
> On Tue, Jun 24, 2025 at 5:23 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > On Tue, Jun 24, 2025 at 05:07:59PM +0200, Amir Goldstein wrote:
> > > On Tue, Jun 24, 2025 at 4:51 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Jun 24, 2025 at 04:28:50PM +0200, Amir Goldstein wrote:
> > > > > On Tue, Jun 24, 2025 at 12:53 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > > > > >
> > > > > > On Tue, Jun 24, 2025 at 11:30 AM Jan Kara <jack@xxxxxxx> wrote:
> > > > > > >
> > > > > > > On Tue 24-06-25 10:29:13, Christian Brauner wrote:
> > > > > > > > Various filesystems such as pidfs (and likely drm in the future) have a
> > > > > > > > use-case to support opening files purely based on the handle without
> > > > > > > > having to require a file descriptor to another object. That's especially
> > > > > > > > the case for filesystems that don't do any lookup whatsoever and there's
> > > > > > > > zero relationship between the objects. Such filesystems are also
> > > > > > > > singletons that stay around for the lifetime of the system meaning that
> > > > > > > > they can be uniquely identified and accessed purely based on the file
> > > > > > > > handle type. Enable that so that userspace doesn't have to allocate an
> > > > > > > > object needlessly especially if they can't do that for whatever reason.
> > > > > > > >
> > > > > > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > fs/fhandle.c | 22 ++++++++++++++++++++--
> > > > > > > > fs/pidfs.c | 5 ++++-
> > > > > > > > 2 files changed, 24 insertions(+), 3 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > > > > > > index ab4891925b52..54081e19f594 100644
> > > > > > > > --- a/fs/fhandle.c
> > > > > > > > +++ b/fs/fhandle.c
> > > > > > > > @@ -173,7 +173,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> > > > > > > > return err;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static int get_path_anchor(int fd, struct path *root)
> > > > > > > > +static int get_path_anchor(int fd, struct path *root, int handle_type)
> > > > > > > > {
> > > > > > > > if (fd >= 0) {
> > > > > > > > CLASS(fd, f)(fd);
> > > > > > > > @@ -193,6 +193,24 @@ static int get_path_anchor(int fd, struct path *root)
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > >
> > > > > > > > + /*
> > > > > > > > + * Only autonomous handles can be decoded without a file
> > > > > > > > + * descriptor.
> > > > > > > > + */
> > > > > > > > + if (!(handle_type & FILEID_IS_AUTONOMOUS))
> > > > > > > > + return -EOPNOTSUPP;
> > > > > > >
> > > > > > > This somewhat ties to my comment to patch 5 that if someone passed invalid
> > > > > > > fd < 0 before, we'd be returning -EBADF and now we'd be returning -EINVAL
> > > > > > > or -EOPNOTSUPP based on FILEID_IS_AUTONOMOUS setting. I don't care that
> > > > > > > much about it so feel free to ignore me but I think the following might be
> > > > > > > more sensible error codes:
> > > > > > >
> > > > > > > if (!(handle_type & FILEID_IS_AUTONOMOUS)) {
> > > > > > > if (fd == FD_INVALID)
> > > > > > > return -EOPNOTSUPP;
> > > > > > > return -EBADF;
> > > > > > > }
> > > > > > >
> > > > > > > if (fd != FD_INVALID)
> > > > > > > return -EBADF; (or -EINVAL no strong preference here)
> > > > > >
> > > > > > FWIW, I like -EBADF better.
> > > > > > it makes the error more descriptive and keeps the flow simple:
> > > > > >
> > > > > > + /*
> > > > > > + * Only autonomous handles can be decoded without a file
> > > > > > + * descriptor and only when FD_INVALID is provided.
> > > > > > + */
> > > > > > + if (fd != FD_INVALID)
> > > > > > + return -EBADF;
> > > > > > +
> > > > > > + if (!(handle_type & FILEID_IS_AUTONOMOUS))
> > > > > > + return -EOPNOTSUPP;
> > > > > >
> > > > >
> > > > > Thinking about it some more, as I am trying to address your concerns
> > > > > about crafting autonomous file handles by systemd, as you already
> > > > > decided to define a range for kernel reserved values for fd, why not,
> > > > > instead of requiring FD_INVALID for autonomous file handle, that we
> > > > > actually define a kernel fd value that translates to "the root of pidfs":
> > > > >
> > > > > + /*
> > > > > + * Autonomous handles can be decoded with a special file
> > > > > + * descriptor value that describes the filesystem.
> > > > > + */
> > > > > + switch (fd) {
> > > > > + case FD_PIDFS_ROOT:
> > > > > + pidfs_get_root(root);
> > > > > + break;
> > > > > + default:
> > > > > + return -EBADF;
> > > > > + }
> > > > > +
> > > > >
> > > > > Then you can toss all my old ideas, including FILEID_IS_AUTONOMOUS,
> > > > > and EXPORT_OP_AUTONOMOUS_HANDLES and you do not even need
> > > > > to define FILEID_PIDFS anymore, just keep exporting FILEID_KERNFS
> > > > > as before (you can also keep the existing systemd code) and when you want
> > > > > to open file by handle you just go
> > > > > open_by_handle_at(FD_PIDFS, &handle, 0)
> > > > > and that's it.
> > > > >
> > > > > In the end, my one and only concern with autonomous file handles is that
> > > > > there should be a user opt-in to request them.
> > > > >
> > > > > Sorry for taking the long road to get to this simpler design.
> > > > > WDYT?
> > > >
> > > > And simply place FD_PIDFS_ROOT into the -10000 range?
> > > > Sounds good to me.
> > >
> > > Yes.
> > >
> > > I mean I don't expect there will be a flood of those singleton
> > > filesystems, but generally speaking, a unique fd constant
> > > to describe the root of a singleton filesystem makes sense IMO.
> >
> > Agreed. See the appended updated patches. I'm not resending completely.
> > I just dropped other patches.
>
> For those can also add:
> Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
>
> But I see that FD_INVALID is still there.
> Are you planning to keep the FD_INVALID patch without any
> users for FD_INVALID?
Yes, even if just for userspace to get used to.
