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

[Jeff Layton <jlayton@kernel.org>]

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@gmail.com>
> > ---
> >  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?
> 

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().

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.

 
> >  	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> >  		return -EFAULT;
> >  
> > @@ -45,7 +50,7 @@ static long do_sys_name_to_handle(const struct path *path,
> >  	/* convert handle size to multiple of sizeof(u32) */
> >  	handle_dwords = f_handle.handle_bytes >> 2;
> >  
> > -	/* we ask for a non connectable maybe decodeable file handle */
> > +	/* Encode a possibly decodeable/connectable file handle */
> >  	retval = exportfs_encode_fh(path->dentry,
> >  				    (struct fid *)handle->f_handle,
> >  				    &handle_dwords, fh_flags);
> > @@ -109,15 +114,26 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> >  {
> >  	struct path path;
> >  	int lookup_flags;
> > -	int fh_flags;
> > +	int fh_flags = 0;
> >  	int err;
> >  
> >  	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> > -		     AT_HANDLE_MNT_ID_UNIQUE))
> > +		     AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * AT_HANDLE_FID means there is no intention to decode file handle
> > +	 * AT_HANDLE_CONNECTABLE means there is an intention to decode a
> > +	 * connected fd (with known path), so these flags are conflicting.
> > +	 */
> > +	if (flag & AT_HANDLE_CONNECTABLE && flag & AT_HANDLE_FID)
> >  		return -EINVAL;
> > +	else if (flag & AT_HANDLE_FID)
> > +		fh_flags |= EXPORT_FH_FID;
> > +	else if (flag & AT_HANDLE_CONNECTABLE)
> > +		fh_flags |= EXPORT_FH_CONNECTABLE;
> >  
> >  	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> > -	fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
> >  	if (flag & AT_EMPTY_PATH)
> >  		lookup_flags |= LOOKUP_EMPTY;
> >  	err = user_path_at(dfd, name, lookup_flags, &path);
> > diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> > index 87e2dec79fea..56ff2100e021 100644
> > --- a/include/uapi/linux/fcntl.h
> > +++ b/include/uapi/linux/fcntl.h
> > @@ -153,6 +153,7 @@
> >  					   object identity and may not be
> >  					   usable with open_by_handle_at(2). */
> >  #define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
> > +#define AT_HANDLE_CONNECTABLE	0x002	/* Request a connectable file handle */
> >  
> >  #if defined(__KERNEL__)
> >  #define AT_GETATTR_NOSEC	0x80000000
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 

-- 
Jeff Layton <jlayton@kernel.org>