Re: [RFC PATCH 1/2] fs: name_to_handle_at() support for connectable file handles

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 20, 2024 at 9:36 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> On Fri, 2024-09-20 at 09:13 +0200, Jeff Layton wrote:
> > On Thu, 2024-09-19 at 16:06 +0200, Amir Goldstein wrote:
> > > nfsd encodes "connectable" file handles for the subtree_check feature.
> > > So far, userspace nfs server could not make use of this functionality.
> > >
> > > Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
> > > When used, the encoded file handle is "connectable".
> > >
> > > Note that decoding a "connectable" file handle with open_by_handle_at(2)
> > > is not guarandteed to return a "connected" fd (i.e. fd with known path).
> > > A new opt-in API would be needed to guarantee a "connected" fd.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > ---
> > >  fs/fhandle.c               | 24 ++++++++++++++++++++----
> > >  include/uapi/linux/fcntl.h |  1 +
> > >  2 files changed, 21 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > index 8cb665629f4a..956d9b25d4f7 100644
> > > --- a/fs/fhandle.c
> > > +++ b/fs/fhandle.c
> > > @@ -31,6 +31,11 @@ static long do_sys_name_to_handle(const struct path *path,
> > >     if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
> > >             return -EOPNOTSUPP;
> > >
> > > +   /* Do not encode a connectable handle for a disconnected dentry */
> > > +   if (fh_flags & EXPORT_FH_CONNECTABLE &&
> > > +       path->dentry->d_flags & DCACHE_DISCONNECTED)
> > > +           return -EACCES;
> > > +
> >
> > I'm not sure about EACCES here. That implies that if you had the right
> > creds then this would work. DCACHE_DISCONNECTED has nothing to do with
> > permissions though. Maybe -EINVAL instead since getting a disconnected
> > dentry here would imply that @path is somehow bogus?
> >
> > Given how this function is used, will we ever see a disconnected dentry
> > here? The path comes from userland in this case, so I don't think it
> > can ever be disconnected. Maybe a WARN_ON_ONCE or pr_warn would be
> > appropriate in this case too?
> >

Yes, I agree (with some additions below...)

>
> Oh, I guess you can get a disconnected dentry here.
>
> You could call open_by_handle_at() for a directory fh, and then pass
> that into name_to_handle_at().

Aha, but a disconnected directory dentry is fine, because it is
still "connectable", so we should not fail on it.

>
> Either way, this API scares me since it seems like it can just randomly
> fail, depending on the state of the dcache. That's the worst-case
> scenario for an API.
>
> If you want to go through with this, you'll need to carefully document
> what's required to make this work properly, as this has some
> significant footguns.
>

Agreed.

The correct statement is that we should not get a disconnected
non-dir dentry, as long as we do not allow AT_EMPTY_PATH,
so if we return EINVAL for the flag combination
AT_EMPTY_PATH | AT_HANDLE_CONNECTABLE
we should be back to a deterministic API.

As you wrote in the first email, we should never expect to
resolve a path to a dentry that is not "connectable". Right?

Thanks,
Amir.





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux