Re: [PATCH v3 1/3] fs: prepare for "explicit connectable" file handles

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> We would like to use the high 16bit of the handle_type field to encode
> file handle traits, such as "connectable".
> 
> In preparation for this change, make sure that filesystems do not return
> a handle_type value with upper bits set and that the open_by_handle_at(2)
> syscall rejects these handle types.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/exportfs/expfs.c      | 14 ++++++++++++--
>  fs/fhandle.c             |  6 ++++++
>  include/linux/exportfs.h | 14 ++++++++++++++
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 4f2dd4ab4486..c8eb660fdde4 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  			     int *max_len, struct inode *parent, int flags)
>  {
>  	const struct export_operations *nop = inode->i_sb->s_export_op;
> +	enum fid_type type;
>  
>  	if (!exportfs_can_encode_fh(nop, flags))
>  		return -EOPNOTSUPP;
>  
>  	if (!nop && (flags & EXPORT_FH_FID))
> -		return exportfs_encode_ino64_fid(inode, fid, max_len);
> +		type = exportfs_encode_ino64_fid(inode, fid, max_len);
> +	else
> +		type = nop->encode_fh(inode, fid->raw, max_len, parent);
> +
> +	if (WARN_ON_ONCE(FILEID_USER_FLAGS(type)))
> +		return -EINVAL;
> +

The stack trace won't be very useful here. Rather than a WARN, it might
be better to dump out some info about the fstype (and maybe other
info?) that returned the bogus type value here. I'm pretty sure most
in-kernel fs's don't do this, but who knows what 3rd party fs's might
do.

> +	return type;
>  
> -	return nop->encode_fh(inode, fid->raw, max_len, parent);
>  }
>  EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
>  
> @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
>  	char nbuf[NAME_MAX+1];
>  	int err;
>  
> +	if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
> +		return -EINVAL;
> +


This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which
means that this fh comes from userland or from an NFS client. I don't
think we want to WARN because someone crafted a bogus fh and passed it
to us.


>  	/*
>  	 * Try to get any dentry for the given file handle from the filesystem.
>  	 */
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 82df28d45cd7..c5792cf3c6e9 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -307,6 +307,10 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		retval = -EINVAL;
>  		goto out_path;
>  	}
> +	if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) {
> +		retval = -EINVAL;
> +		goto out_path;
> +	}
>  	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
>  			 GFP_KERNEL);
>  	if (!handle) {
> @@ -322,6 +326,8 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		goto out_handle;
>  	}
>  
> +	/* Filesystem code should not be exposed to user flags */
> +	handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
>  	retval = do_handle_to_path(handle, path, &ctx);
>  
>  out_handle:
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 893a1d21dc1c..76a3050b3593 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -160,6 +160,20 @@ struct fid {
>  #define EXPORT_FH_FID		0x2 /* File handle may be non-decodeable */
>  #define EXPORT_FH_DIR_ONLY	0x4 /* Only decode file handle for a directory */
>  
> +/*
> + * Filesystems use only lower 8 bits of file_handle type for fid_type.
> + * name_to_handle_at() uses upper 16 bits of type as user flags to be
> + * interpreted by open_by_handle_at().
> + */
> +#define FILEID_USER_FLAGS_MASK	0xffff0000
> +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> +
> +/* Flags supported in encoded handle_type that is exported to user */
> +#define FILEID_VALID_USER_FLAGS	(0)
> +
> +#define FILEID_USER_TYPE_IS_VALID(type) \
> +	(!(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS))
> +
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
>   * @encode_fh:      encode a file handle fragment from a dentry

The rest looks reasonable.
-- 
Jeff Layton <jlayton@kernel.org>