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

[Jeff Layton <jlayton@kernel.org>]

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?

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