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.
