[PATCH v4 0/3] API for exporting connectable file handles to userspace

[PATCH v4 0/3] API for exporting connectable file handles to userspace

[Amir Goldstein]

Christian,

These patches bring the NFS connectable file handles feature to
userspace servers.

They rely on your and Aleksa's changes recently merged to v6.12.

This v4 incorporates the review comments on Jeff and Jan (thanks!)
and there does not seem to be any objection for this new API, so
I think it is ready for staging.

The API I chose for encoding conenctable file handles is pretty
conventional (AT_HANDLE_CONNECTABLE).

open_by_handle_at(2) does not have AT_ flags argument, but also, I find
it more useful API that encoding a connectable file handle can mandate
the resolving of a connected fd, without having to opt-in for a
connected fd independently.

I chose to implemnent this by using upper bits in the handle type field
It may be that out-of-tree filesystems return a handle type with upper
bits set, but AFAIK, no in-tree filesystem does that.
I added some warnings just in case we encouter that.

I have written an fstest [4] and a man page draft [5] for the feature.

Thanks,
Amir.

Changes since v3 [3]:
- Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
- Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
- Add explicit check for negative type values (Jan)
- Added fstest and man-page draft

Changes since v2 [2]:
- Use bit arithmetics instead of bitfileds (Jeff)
- Add assertions about use of high type bits

Changes since v1 [1]:
- Assert on encode for disconnected path (Jeff)
- Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
- Drop the O_PATH mount_fd API hack (Jeff)
- Encode an explicit "connectable" flag in handle type

[1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
[2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
[3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
[4] https://github.com/amir73il/xfstests/commits/connectable-fh/
[5] https://github.com/amir73il/man-pages/commits/connectable-fh/

Amir Goldstein (3):
  fs: prepare for "explicit connectable" file handles
  fs: name_to_handle_at() support for "explicit connectable" file
    handles
  fs: open_by_handle_at() support for decoding "explicit connectable"
    file handles

 fs/exportfs/expfs.c        | 17 ++++++++-
 fs/fhandle.c               | 75 +++++++++++++++++++++++++++++++++++---
 include/linux/exportfs.h   | 13 +++++++
 include/uapi/linux/fcntl.h |  1 +
 4 files changed, 98 insertions(+), 8 deletions(-)

-- 
2.34.1

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

[Amir Goldstein]

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      | 17 +++++++++++++++--
 fs/fhandle.c             |  7 +++++++
 include/linux/exportfs.h | 11 +++++++++++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index 4f2dd4ab4486..0c899cfba578 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -382,14 +382,24 @@ 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 (type > 0 && FILEID_USER_FLAGS(type)) {
+		pr_warn_once("%s: unexpected fh type value 0x%x from fstype %s.\n",
+			     __func__, type, inode->i_sb->s_type->name);
+		return -EINVAL;
+	}
+
+	return type;
 
-	return nop->encode_fh(inode, fid->raw, max_len, parent);
 }
 EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
 
@@ -436,6 +446,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
 	char nbuf[NAME_MAX+1];
 	int err;
 
+	if (fileid_type < 0 || FILEID_USER_FLAGS(fileid_type))
+		return ERR_PTR(-EINVAL);
+
 	/*
 	 * 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..218511f38cbb 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -307,6 +307,11 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		retval = -EINVAL;
 		goto out_path;
 	}
+	if (f_handle.handle_type < 0 ||
+	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS) {
+		retval = -EINVAL;
+		goto out_path;
+	}
 	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
 			 GFP_KERNEL);
 	if (!handle) {
@@ -322,6 +327,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..5e14d4500a75 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -160,6 +160,17 @@ 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)
+
 /**
  * struct export_operations - for nfsd to communicate with file systems
  * @encode_fh:      encode a file handle fragment from a dentry
-- 
2.34.1

[PATCH v4 2/3] fs: name_to_handle_at() support for "explicit connectable" file handles

[Amir Goldstein]

nfsd encodes "connectable" file handles for the subtree_check feature,
which can be resolved to an open file with a connected path.
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 "explicitly connectable".

The "explicitly connectable" file handle sets bits in the high 16bit of
the handle_type field, so open_by_handle_at(2) will know that it needs
to open a file with a connected path.

old kernels will now recognize the handle_type with high bits set,
so "explicitly connectable" file handles cannot be decoded by
open_by_handle_at(2) on old kernels.

The flag AT_HANDLE_CONNECTABLE is not allowed together with either
AT_HANDLE_FID or AT_EMPTY_PATH.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
 include/linux/exportfs.h   |  2 ++
 include/uapi/linux/fcntl.h |  1 +
 3 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 218511f38cbb..8339a1041025 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -31,6 +31,14 @@ 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;
 
+	/*
+	 * A request to encode a connectable handle for a disconnected dentry
+	 * is unexpected since AT_EMPTY_PATH is not allowed.
+	 */
+	if (fh_flags & EXPORT_FH_CONNECTABLE &&
+	    WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED))
+		return -EINVAL;
+
 	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
 		return -EFAULT;
 
@@ -45,7 +53,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);
@@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
 		 * non variable part of the file_handle
 		 */
 		handle_bytes = 0;
-	} else
+	} else {
+		/*
+		 * When asked to encode a connectable file handle, encode this
+		 * property in the file handle itself, so that we later know
+		 * how to decode it.
+		 * For sanity, also encode in the file handle if the encoded
+		 * object is a directory and verify this during decode, because
+		 * decoding directory file handles is quite different than
+		 * decoding connectable non-directory file handles.
+		 */
+		if (fh_flags & EXPORT_FH_CONNECTABLE) {
+			handle->handle_type |= FILEID_IS_CONNECTABLE;
+			if (d_is_dir(path->dentry))
+				fh_flags |= FILEID_IS_DIR;
+		}
 		retval = 0;
+	}
 	/* copy the mount id */
 	if (unique_mntid) {
 		if (put_user(real_mount(path->mnt)->mnt_id_unique,
@@ -109,15 +132,30 @@ 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.
+	 * AT_EMPTY_PATH could be used along with a dfd that refers to a
+	 * disconnected non-directory, which cannot be used to encode a
+	 * connectable file handle, because its parent is unknown.
+	 */
+	if (flag & AT_HANDLE_CONNECTABLE &&
+	    flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
 		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/linux/exportfs.h b/include/linux/exportfs.h
index 5e14d4500a75..4ee42b2cf4ab 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -169,6 +169,8 @@ struct fid {
 #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
 
 /* Flags supported in encoded handle_type that is exported to user */
+#define FILEID_IS_CONNECTABLE	0x10000
+#define FILEID_IS_DIR		0x20000
 #define FILEID_VALID_USER_FLAGS	(0)
 
 /**
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
-- 
2.34.1

[PATCH v4 3/3] fs: open_by_handle_at() support for decoding "explicit connectable" file handles

[Amir Goldstein]

Teach open_by_handle_at(2) about the type format of "explicit connectable"
file handles that were created using the AT_HANDLE_CONNECTABLE flag to
name_to_handle_at(2).

When decoding an "explicit connectable" file handles, name_to_handle_at(2)
should fail if it cannot open a "connected" fd with known path, which is
accessible (to capable user) from mount fd path.

Note that this does not check if the path is accessible to the calling
user, just that it is accessible wrt the mount namesapce, so if there
is no "connected" alias, or if parts of the path are hidden in the
mount namespace, open_by_handle_at(2) will return -ESTALE.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fhandle.c             | 20 +++++++++++++++++++-
 include/linux/exportfs.h |  2 +-
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8339a1041025..75cfd190cd69 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -246,7 +246,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
 
 	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
 		retval = 1;
-	WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
+	/*
+	 * exportfs_decode_fh_raw() does not call acceptable() callback with
+	 * a disconnected directory dentry, so we should have reached either
+	 * mount fd directory or sb root.
+	 */
+	if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
+		WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
 	dput(d);
 	return retval;
 }
@@ -350,6 +356,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		retval = -EINVAL;
 		goto out_path;
 	}
+
 	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
 			 GFP_KERNEL);
 	if (!handle) {
@@ -365,6 +372,17 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		goto out_handle;
 	}
 
+	/*
+	 * If handle was encoded with AT_HANDLE_CONNECTABLE, verify that we
+	 * are decoding an fd with connected path, which is accessible from
+	 * the mount fd path.
+	 */
+	if (f_handle.handle_type & FILEID_IS_CONNECTABLE) {
+		ctx.fh_flags |= EXPORT_FH_CONNECTABLE;
+		ctx.flags |= HANDLE_CHECK_SUBTREE;
+	}
+	if (f_handle.handle_type & FILEID_IS_DIR)
+		ctx.fh_flags |= EXPORT_FH_DIR_ONLY;
 	/* Filesystem code should not be exposed to user flags */
 	handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
 	retval = do_handle_to_path(handle, path, &ctx);
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 4ee42b2cf4ab..fcab6ab1d38a 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -171,7 +171,7 @@ struct fid {
 /* Flags supported in encoded handle_type that is exported to user */
 #define FILEID_IS_CONNECTABLE	0x10000
 #define FILEID_IS_DIR		0x20000
-#define FILEID_VALID_USER_FLAGS	(0)
+#define FILEID_VALID_USER_FLAGS	(FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
 
 /**
  * struct export_operations - for nfsd to communicate with file systems
-- 
2.34.1

Re: [PATCH v4 2/3] fs: name_to_handle_at() support for "explicit connectable" file handles

[Jeff Layton]

On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote:
> nfsd encodes "connectable" file handles for the subtree_check feature,
> which can be resolved to an open file with a connected path.
> 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 "explicitly connectable".
> 
> The "explicitly connectable" file handle sets bits in the high 16bit of
> the handle_type field, so open_by_handle_at(2) will know that it needs
> to open a file with a connected path.
> 
> old kernels will now recognize the handle_type with high bits set,
> so "explicitly connectable" file handles cannot be decoded by
> open_by_handle_at(2) on old kernels.
> 
> The flag AT_HANDLE_CONNECTABLE is not allowed together with either
> AT_HANDLE_FID or AT_EMPTY_PATH.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
>  include/linux/exportfs.h   |  2 ++
>  include/uapi/linux/fcntl.h |  1 +
>  3 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 218511f38cbb..8339a1041025 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -31,6 +31,14 @@ 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;
>  
> +	/*
> +	 * A request to encode a connectable handle for a disconnected dentry
> +	 * is unexpected since AT_EMPTY_PATH is not allowed.
> +	 */
> +	if (fh_flags & EXPORT_FH_CONNECTABLE &&
> +	    WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED))

Is this even possible? The dentry in this case will have been reached
by pathwalk. Oh, but I guess the dfd could point to a disconnected
dentry and then you pass in AT_EMPTY_PATH.

I'm not sure we want to warn in that case though, since this is a
situation that an unprivileged user could be able to arrange. Maybe we
should just return a more distinct error code in this case?

Since the scenario involves a dfd that is disconnected, how about:

    #define EBADFD          77      /* File descriptor in bad state */



> +		return -EINVAL;
> +
>  	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
>  		return -EFAULT;
>  
> @@ -45,7 +53,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);
> @@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
>  		 * non variable part of the file_handle
>  		 */
>  		handle_bytes = 0;
> -	} else
> +	} else {
> +		/*
> +		 * When asked to encode a connectable file handle, encode this
> +		 * property in the file handle itself, so that we later know
> +		 * how to decode it.
> +		 * For sanity, also encode in the file handle if the encoded
> +		 * object is a directory and verify this during decode, because
> +		 * decoding directory file handles is quite different than
> +		 * decoding connectable non-directory file handles.
> +		 */
> +		if (fh_flags & EXPORT_FH_CONNECTABLE) {
> +			handle->handle_type |= FILEID_IS_CONNECTABLE;
> +			if (d_is_dir(path->dentry))
> +				fh_flags |= FILEID_IS_DIR;
> +		}
>  		retval = 0;
> +	}
>  	/* copy the mount id */
>  	if (unique_mntid) {
>  		if (put_user(real_mount(path->mnt)->mnt_id_unique,
> @@ -109,15 +132,30 @@ 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.
> +	 * AT_EMPTY_PATH could be used along with a dfd that refers to a
> +	 * disconnected non-directory, which cannot be used to encode a
> +	 * connectable file handle, because its parent is unknown.
> +	 */
> +	if (flag & AT_HANDLE_CONNECTABLE &&

nit: might need parenthesis around the above & check.

> +	    flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
>  		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/linux/exportfs.h b/include/linux/exportfs.h
> index 5e14d4500a75..4ee42b2cf4ab 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -169,6 +169,8 @@ struct fid {
>  #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
>  
>  /* Flags supported in encoded handle_type that is exported to user */
> +#define FILEID_IS_CONNECTABLE	0x10000
> +#define FILEID_IS_DIR		0x20000
>  #define FILEID_VALID_USER_FLAGS	(0)
>  
>  /**
> 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>

Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

[Jeff Layton]

On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote:
> Christian,
> 
> These patches bring the NFS connectable file handles feature to
> userspace servers.
> 
> They rely on your and Aleksa's changes recently merged to v6.12.
> 
> This v4 incorporates the review comments on Jeff and Jan (thanks!)
> and there does not seem to be any objection for this new API, so
> I think it is ready for staging.
> 
> The API I chose for encoding conenctable file handles is pretty
> conventional (AT_HANDLE_CONNECTABLE).
> 
> open_by_handle_at(2) does not have AT_ flags argument, but also, I find
> it more useful API that encoding a connectable file handle can mandate
> the resolving of a connected fd, without having to opt-in for a
> connected fd independently.
> 
> I chose to implemnent this by using upper bits in the handle type field
> It may be that out-of-tree filesystems return a handle type with upper
> bits set, but AFAIK, no in-tree filesystem does that.
> I added some warnings just in case we encouter that.
> 
> I have written an fstest [4] and a man page draft [5] for the feature.
> 
> Thanks,
> Amir.
> 
> Changes since v3 [3]:
> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
> - Add explicit check for negative type values (Jan)
> - Added fstest and man-page draft
> 
> Changes since v2 [2]:
> - Use bit arithmetics instead of bitfileds (Jeff)
> - Add assertions about use of high type bits
> 
> Changes since v1 [1]:
> - Assert on encode for disconnected path (Jeff)
> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
> - Drop the O_PATH mount_fd API hack (Jeff)
> - Encode an explicit "connectable" flag in handle type
> 
> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/
> 
> Amir Goldstein (3):
>   fs: prepare for "explicit connectable" file handles
>   fs: name_to_handle_at() support for "explicit connectable" file
>     handles
>   fs: open_by_handle_at() support for decoding "explicit connectable"
>     file handles
> 
>  fs/exportfs/expfs.c        | 17 ++++++++-
>  fs/fhandle.c               | 75 +++++++++++++++++++++++++++++++++++---
>  include/linux/exportfs.h   | 13 +++++++
>  include/uapi/linux/fcntl.h |  1 +
>  4 files changed, 98 insertions(+), 8 deletions(-)
> 

Aside from the relatively small stuff I noted in patch #2, this looks
fine to me.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v4 2/3] fs: name_to_handle_at() support for "explicit connectable" file handles

[Amir Goldstein]

On Fri, Oct 11, 2024 at 4:00 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote:
> > nfsd encodes "connectable" file handles for the subtree_check feature,
> > which can be resolved to an open file with a connected path.
> > 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 "explicitly connectable".
> >
> > The "explicitly connectable" file handle sets bits in the high 16bit of
> > the handle_type field, so open_by_handle_at(2) will know that it needs
> > to open a file with a connected path.
> >
> > old kernels will now recognize the handle_type with high bits set,
> > so "explicitly connectable" file handles cannot be decoded by
> > open_by_handle_at(2) on old kernels.
> >
> > The flag AT_HANDLE_CONNECTABLE is not allowed together with either
> > AT_HANDLE_FID or AT_EMPTY_PATH.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
> >  include/linux/exportfs.h   |  2 ++
> >  include/uapi/linux/fcntl.h |  1 +
> >  3 files changed, 46 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 218511f38cbb..8339a1041025 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -31,6 +31,14 @@ 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;
> >
> > +     /*
> > +      * A request to encode a connectable handle for a disconnected dentry
> > +      * is unexpected since AT_EMPTY_PATH is not allowed.
> > +      */
> > +     if (fh_flags & EXPORT_FH_CONNECTABLE &&
> > +         WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED))
>
> Is this even possible? The dentry in this case will have been reached
> by pathwalk. Oh, but I guess the dfd could point to a disconnected
> dentry and then you pass in AT_EMPTY_PATH.

But see comment above "...is unexpected since AT_EMPTY_PATH is not allowed."

and see below

+        * AT_EMPTY_PATH could be used along with a dfd that refers to a
+        * disconnected non-directory, which cannot be used to encode a
+        * connectable file handle, because its parent is unknown.
+        */
+       if (flag & AT_HANDLE_CONNECTABLE &&
+           flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
                return -EINVAL;

The code/API should not allow this also for a malicious user,
unless I missed something, hence, the assertion.

>
> I'm not sure we want to warn in that case though, since this is a
> situation that an unprivileged user could be able to arrange. Maybe we
> should just return a more distinct error code in this case?
>
> Since the scenario involves a dfd that is disconnected, how about:
>
>     #define EBADFD          77      /* File descriptor in bad state */
>

To me it does not look like a good fit, but let's see what others think.
In the end, it is a rare condition that should never happen
(hence assert), so I don't think the error value matters that much?

> > +             return -EINVAL;
> > +
> >       if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> >               return -EFAULT;
> >
> > @@ -45,7 +53,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);
> > @@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
> >                * non variable part of the file_handle
> >                */
> >               handle_bytes = 0;
> > -     } else
> > +     } else {
> > +             /*
> > +              * When asked to encode a connectable file handle, encode this
> > +              * property in the file handle itself, so that we later know
> > +              * how to decode it.
> > +              * For sanity, also encode in the file handle if the encoded
> > +              * object is a directory and verify this during decode, because
> > +              * decoding directory file handles is quite different than
> > +              * decoding connectable non-directory file handles.
> > +              */
> > +             if (fh_flags & EXPORT_FH_CONNECTABLE) {
> > +                     handle->handle_type |= FILEID_IS_CONNECTABLE;
> > +                     if (d_is_dir(path->dentry))
> > +                             fh_flags |= FILEID_IS_DIR;
> > +             }
> >               retval = 0;
> > +     }
> >       /* copy the mount id */
> >       if (unique_mntid) {
> >               if (put_user(real_mount(path->mnt)->mnt_id_unique,
> > @@ -109,15 +132,30 @@ 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.
> > +      * AT_EMPTY_PATH could be used along with a dfd that refers to a
> > +      * disconnected non-directory, which cannot be used to encode a
> > +      * connectable file handle, because its parent is unknown.
> > +      */
> > +     if (flag & AT_HANDLE_CONNECTABLE &&
>
> nit: might need parenthesis around the above & check.
>
> > +         flag & (AT_HANDLE_FID | AT_EMPTY_PATH))

I don't think it is needed, but for readability I don't mind adding them.
I am having a hard time remembering the operation precedence  myself,
but this one is clear to me so I don't bother with ().

Thanks,
Amir.

Re: [PATCH v4 2/3] fs: name_to_handle_at() support for "explicit connectable" file handles

[Jeff Layton]

On Fri, 2024-10-11 at 16:14 +0200, Amir Goldstein wrote:
> On Fri, Oct 11, 2024 at 4:00 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote:
> > > nfsd encodes "connectable" file handles for the subtree_check feature,
> > > which can be resolved to an open file with a connected path.
> > > 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 "explicitly connectable".
> > > 
> > > The "explicitly connectable" file handle sets bits in the high 16bit of
> > > the handle_type field, so open_by_handle_at(2) will know that it needs
> > > to open a file with a connected path.
> > > 
> > > old kernels will now recognize the handle_type with high bits set,
> > > so "explicitly connectable" file handles cannot be decoded by
> > > open_by_handle_at(2) on old kernels.
> > > 
> > > The flag AT_HANDLE_CONNECTABLE is not allowed together with either
> > > AT_HANDLE_FID or AT_EMPTY_PATH.
> > > 
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
> > >  include/linux/exportfs.h   |  2 ++
> > >  include/uapi/linux/fcntl.h |  1 +
> > >  3 files changed, 46 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > index 218511f38cbb..8339a1041025 100644
> > > --- a/fs/fhandle.c
> > > +++ b/fs/fhandle.c
> > > @@ -31,6 +31,14 @@ 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;
> > > 
> > > +     /*
> > > +      * A request to encode a connectable handle for a disconnected dentry
> > > +      * is unexpected since AT_EMPTY_PATH is not allowed.
> > > +      */
> > > +     if (fh_flags & EXPORT_FH_CONNECTABLE &&
> > > +         WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED))
> > 
> > Is this even possible? The dentry in this case will have been reached
> > by pathwalk. Oh, but I guess the dfd could point to a disconnected
> > dentry and then you pass in AT_EMPTY_PATH.
> 
> But see comment above "...is unexpected since AT_EMPTY_PATH is not allowed."
> 
> and see below
> 
> +        * AT_EMPTY_PATH could be used along with a dfd that refers to a
> +        * disconnected non-directory, which cannot be used to encode a
> +        * connectable file handle, because its parent is unknown.
> +        */
> +       if (flag & AT_HANDLE_CONNECTABLE &&
> +           flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
>                 return -EINVAL;
> 
> The code/API should not allow this also for a malicious user,
> unless I missed something, hence, the assertion.
> 

Ok. If that's the case, I'm fine with this as-is then. If that ever
fires then I guess we'll know that something is wrong.

> > 
> > I'm not sure we want to warn in that case though, since this is a
> > situation that an unprivileged user could be able to arrange. Maybe we
> > should just return a more distinct error code in this case?
> > 
> > Since the scenario involves a dfd that is disconnected, how about:
> > 
> >     #define EBADFD          77      /* File descriptor in bad state */
> > 
> 
> To me it does not look like a good fit, but let's see what others think.
> In the end, it is a rare condition that should never happen
> (hence assert), so I don't think the error value matters that much?
> 

Agreed.

> > > +             return -EINVAL;
> > > +
> > >       if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> > >               return -EFAULT;
> > > 
> > > @@ -45,7 +53,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);
> > > @@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
> > >                * non variable part of the file_handle
> > >                */
> > >               handle_bytes = 0;
> > > -     } else
> > > +     } else {
> > > +             /*
> > > +              * When asked to encode a connectable file handle, encode this
> > > +              * property in the file handle itself, so that we later know
> > > +              * how to decode it.
> > > +              * For sanity, also encode in the file handle if the encoded
> > > +              * object is a directory and verify this during decode, because
> > > +              * decoding directory file handles is quite different than
> > > +              * decoding connectable non-directory file handles.
> > > +              */
> > > +             if (fh_flags & EXPORT_FH_CONNECTABLE) {
> > > +                     handle->handle_type |= FILEID_IS_CONNECTABLE;
> > > +                     if (d_is_dir(path->dentry))
> > > +                             fh_flags |= FILEID_IS_DIR;
> > > +             }
> > >               retval = 0;
> > > +     }
> > >       /* copy the mount id */
> > >       if (unique_mntid) {
> > >               if (put_user(real_mount(path->mnt)->mnt_id_unique,
> > > @@ -109,15 +132,30 @@ 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.
> > > +      * AT_EMPTY_PATH could be used along with a dfd that refers to a
> > > +      * disconnected non-directory, which cannot be used to encode a
> > > +      * connectable file handle, because its parent is unknown.
> > > +      */
> > > +     if (flag & AT_HANDLE_CONNECTABLE &&
> > 
> > nit: might need parenthesis around the above & check.
> > 
> > > +         flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
> 
> I don't think it is needed, but for readability I don't mind adding them.
> I am having a hard time remembering the operation precedence  myself,
> but this one is clear to me so I don't bother with ().

I (lately) get warnings from the compiler with W=1 even when the
precedence is fine. If you're not seeing that then this is OK too.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

[Chuck Lever III]

> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> Christian,
> 
> These patches bring the NFS connectable file handles feature to
> userspace servers.
> 
> They rely on your and Aleksa's changes recently merged to v6.12.
> 
> This v4 incorporates the review comments on Jeff and Jan (thanks!)
> and there does not seem to be any objection for this new API, so
> I think it is ready for staging.
> 
> The API I chose for encoding conenctable file handles is pretty
> conventional (AT_HANDLE_CONNECTABLE).
> 
> open_by_handle_at(2) does not have AT_ flags argument, but also, I find
> it more useful API that encoding a connectable file handle can mandate
> the resolving of a connected fd, without having to opt-in for a
> connected fd independently.
> 
> I chose to implemnent this by using upper bits in the handle type field
> It may be that out-of-tree filesystems return a handle type with upper
> bits set, but AFAIK, no in-tree filesystem does that.
> I added some warnings just in case we encouter that.
> 
> I have written an fstest [4] and a man page draft [5] for the feature.
> 
> Thanks,
> Amir.
> 
> Changes since v3 [3]:
> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
> - Add explicit check for negative type values (Jan)
> - Added fstest and man-page draft
> 
> Changes since v2 [2]:
> - Use bit arithmetics instead of bitfileds (Jeff)
> - Add assertions about use of high type bits
> 
> Changes since v1 [1]:
> - Assert on encode for disconnected path (Jeff)
> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
> - Drop the O_PATH mount_fd API hack (Jeff)
> - Encode an explicit "connectable" flag in handle type
> 
> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/
> 
> Amir Goldstein (3):
>  fs: prepare for "explicit connectable" file handles
>  fs: name_to_handle_at() support for "explicit connectable" file
>    handles
>  fs: open_by_handle_at() support for decoding "explicit connectable"
>    file handles
> 
> fs/exportfs/expfs.c        | 17 ++++++++-
> fs/fhandle.c               | 75 +++++++++++++++++++++++++++++++++++---
> include/linux/exportfs.h   | 13 +++++++
> include/uapi/linux/fcntl.h |  1 +
> 4 files changed, 98 insertions(+), 8 deletions(-)
> 
> -- 
> 2.34.1
> 

Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>

Assuming this is going directly to Christian's tree.

I'm a little concerned about how this new facility might be
abused to get access to parts of the file system that a user
is not authorized to access. But follow-up comments from Amir
suggest that (with the current code) it is difficult or
impossible to do.

Are there self-tests or unit-tests for exportfs?


--
Chuck Lever

Re: [PATCH v4 2/3] fs: name_to_handle_at() support for "explicit connectable" file handles

[Amir Goldstein]

On Fri, Oct 11, 2024 at 4:18 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2024-10-11 at 16:14 +0200, Amir Goldstein wrote:
> > On Fri, Oct 11, 2024 at 4:00 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote:
> > > > nfsd encodes "connectable" file handles for the subtree_check feature,
> > > > which can be resolved to an open file with a connected path.
> > > > 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 "explicitly connectable".
> > > >
> > > > The "explicitly connectable" file handle sets bits in the high 16bit of
> > > > the handle_type field, so open_by_handle_at(2) will know that it needs
> > > > to open a file with a connected path.
> > > >
> > > > old kernels will now recognize the handle_type with high bits set,
> > > > so "explicitly connectable" file handles cannot be decoded by
> > > > open_by_handle_at(2) on old kernels.
> > > >
> > > > The flag AT_HANDLE_CONNECTABLE is not allowed together with either
> > > > AT_HANDLE_FID or AT_EMPTY_PATH.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > >  fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
> > > >  include/linux/exportfs.h   |  2 ++
> > > >  include/uapi/linux/fcntl.h |  1 +
> > > >  3 files changed, 46 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > > > index 218511f38cbb..8339a1041025 100644
> > > > --- a/fs/fhandle.c
> > > > +++ b/fs/fhandle.c
> > > > @@ -31,6 +31,14 @@ 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;
> > > >
> > > > +     /*
> > > > +      * A request to encode a connectable handle for a disconnected dentry
> > > > +      * is unexpected since AT_EMPTY_PATH is not allowed.
> > > > +      */
> > > > +     if (fh_flags & EXPORT_FH_CONNECTABLE &&
> > > > +         WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED))
> > >
> > > Is this even possible? The dentry in this case will have been reached
> > > by pathwalk. Oh, but I guess the dfd could point to a disconnected
> > > dentry and then you pass in AT_EMPTY_PATH.
> >
> > But see comment above "...is unexpected since AT_EMPTY_PATH is not allowed."
> >
> > and see below
> >
> > +        * AT_EMPTY_PATH could be used along with a dfd that refers to a
> > +        * disconnected non-directory, which cannot be used to encode a
> > +        * connectable file handle, because its parent is unknown.
> > +        */
> > +       if (flag & AT_HANDLE_CONNECTABLE &&
> > +           flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
> >                 return -EINVAL;
> >
> > The code/API should not allow this also for a malicious user,
> > unless I missed something, hence, the assertion.
> >
>
> Ok. If that's the case, I'm fine with this as-is then. If that ever
> fires then I guess we'll know that something is wrong.
>
> > >
> > > I'm not sure we want to warn in that case though, since this is a
> > > situation that an unprivileged user could be able to arrange. Maybe we
> > > should just return a more distinct error code in this case?
> > >
> > > Since the scenario involves a dfd that is disconnected, how about:
> > >
> > >     #define EBADFD          77      /* File descriptor in bad state */
> > >
> >
> > To me it does not look like a good fit, but let's see what others think.
> > In the end, it is a rare condition that should never happen
> > (hence assert), so I don't think the error value matters that much?
> >
>
> Agreed.
>
> > > > +             return -EINVAL;
> > > > +
> > > >       if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> > > >               return -EFAULT;
> > > >
> > > > @@ -45,7 +53,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);
> > > > @@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
> > > >                * non variable part of the file_handle
> > > >                */
> > > >               handle_bytes = 0;
> > > > -     } else
> > > > +     } else {
> > > > +             /*
> > > > +              * When asked to encode a connectable file handle, encode this
> > > > +              * property in the file handle itself, so that we later know
> > > > +              * how to decode it.
> > > > +              * For sanity, also encode in the file handle if the encoded
> > > > +              * object is a directory and verify this during decode, because
> > > > +              * decoding directory file handles is quite different than
> > > > +              * decoding connectable non-directory file handles.
> > > > +              */
> > > > +             if (fh_flags & EXPORT_FH_CONNECTABLE) {
> > > > +                     handle->handle_type |= FILEID_IS_CONNECTABLE;
> > > > +                     if (d_is_dir(path->dentry))
> > > > +                             fh_flags |= FILEID_IS_DIR;
> > > > +             }
> > > >               retval = 0;
> > > > +     }
> > > >       /* copy the mount id */
> > > >       if (unique_mntid) {
> > > >               if (put_user(real_mount(path->mnt)->mnt_id_unique,
> > > > @@ -109,15 +132,30 @@ 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.
> > > > +      * AT_EMPTY_PATH could be used along with a dfd that refers to a
> > > > +      * disconnected non-directory, which cannot be used to encode a
> > > > +      * connectable file handle, because its parent is unknown.
> > > > +      */
> > > > +     if (flag & AT_HANDLE_CONNECTABLE &&
> > >
> > > nit: might need parenthesis around the above & check.
> > >
> > > > +         flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
> >
> > I don't think it is needed, but for readability I don't mind adding them.
> > I am having a hard time remembering the operation precedence  myself,
> > but this one is clear to me so I don't bother with ().
>
> I (lately) get warnings from the compiler with W=1 even when the
> precedence is fine. If you're not seeing that then this is OK too.
>

Did not get any warnings, but if Christian wants to add the () on
commit I have no quarrel with that :)

Thanks for the review!
Amir.

Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Christian,
> >
> > These patches bring the NFS connectable file handles feature to
> > userspace servers.
> >
> > They rely on your and Aleksa's changes recently merged to v6.12.
> >
> > This v4 incorporates the review comments on Jeff and Jan (thanks!)
> > and there does not seem to be any objection for this new API, so
> > I think it is ready for staging.
> >
> > The API I chose for encoding conenctable file handles is pretty
> > conventional (AT_HANDLE_CONNECTABLE).
> >
> > open_by_handle_at(2) does not have AT_ flags argument, but also, I find
> > it more useful API that encoding a connectable file handle can mandate
> > the resolving of a connected fd, without having to opt-in for a
> > connected fd independently.
> >
> > I chose to implemnent this by using upper bits in the handle type field
> > It may be that out-of-tree filesystems return a handle type with upper
> > bits set, but AFAIK, no in-tree filesystem does that.
> > I added some warnings just in case we encouter that.
> >
> > I have written an fstest [4] and a man page draft [5] for the feature.
> >
> > Thanks,
> > Amir.
> >
> > Changes since v3 [3]:
> > - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
> > - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
> > - Add explicit check for negative type values (Jan)
> > - Added fstest and man-page draft
> >
> > Changes since v2 [2]:
> > - Use bit arithmetics instead of bitfileds (Jeff)
> > - Add assertions about use of high type bits
> >
> > Changes since v1 [1]:
> > - Assert on encode for disconnected path (Jeff)
> > - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
> > - Drop the O_PATH mount_fd API hack (Jeff)
> > - Encode an explicit "connectable" flag in handle type
> >
> > [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
> > [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
> > [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
> > [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
> > [5] https://github.com/amir73il/man-pages/commits/connectable-fh/
> >
> > Amir Goldstein (3):
> >  fs: prepare for "explicit connectable" file handles
> >  fs: name_to_handle_at() support for "explicit connectable" file
> >    handles
> >  fs: open_by_handle_at() support for decoding "explicit connectable"
> >    file handles
> >
> > fs/exportfs/expfs.c        | 17 ++++++++-
> > fs/fhandle.c               | 75 +++++++++++++++++++++++++++++++++++---
> > include/linux/exportfs.h   | 13 +++++++
> > include/uapi/linux/fcntl.h |  1 +
> > 4 files changed, 98 insertions(+), 8 deletions(-)
> >
> > --
> > 2.34.1
> >
>
> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>
> Assuming this is going directly to Christian's tree.
>
> I'm a little concerned about how this new facility might be
> abused to get access to parts of the file system that a user
> is not authorized to access.

That's exactly the sort of thing I would like to be reviewed,
but what makes you feel concerned?

Are you concerned about handcrafted file handles?
Correct me if I am wrong, but I think that any parts of the filesystem
that could be accessed (by user with CAP_DAC_READ_SEARCH)
using a handcrafted connectable file handle, could have also been
accessed by the parent fid part before, so I do not see how connectable
file handles create new ways to get access?

> But follow-up comments from Amir
> suggest that (with the current code) it is difficult or
> impossible to do.
>
> Are there self-tests or unit-tests for exportfs?

There are fstests, particularly, the "exportfs" test group
and I added this one for connectable file handles:

[4] https://github.com/amir73il/xfstests/commits/connectable-fh/

Did you mean another form of unit tests?

Thanks,
Amir.

Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

[Chuck Lever III]

> On Oct 11, 2024, at 2:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>> 
>> 
>> 
>>> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> 
>>> Christian,
>>> 
>>> These patches bring the NFS connectable file handles feature to
>>> userspace servers.
>>> 
>>> They rely on your and Aleksa's changes recently merged to v6.12.
>>> 
>>> This v4 incorporates the review comments on Jeff and Jan (thanks!)
>>> and there does not seem to be any objection for this new API, so
>>> I think it is ready for staging.
>>> 
>>> The API I chose for encoding conenctable file handles is pretty
>>> conventional (AT_HANDLE_CONNECTABLE).
>>> 
>>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find
>>> it more useful API that encoding a connectable file handle can mandate
>>> the resolving of a connected fd, without having to opt-in for a
>>> connected fd independently.
>>> 
>>> I chose to implemnent this by using upper bits in the handle type field
>>> It may be that out-of-tree filesystems return a handle type with upper
>>> bits set, but AFAIK, no in-tree filesystem does that.
>>> I added some warnings just in case we encouter that.
>>> 
>>> I have written an fstest [4] and a man page draft [5] for the feature.
>>> 
>>> Thanks,
>>> Amir.
>>> 
>>> Changes since v3 [3]:
>>> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
>>> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
>>> - Add explicit check for negative type values (Jan)
>>> - Added fstest and man-page draft
>>> 
>>> Changes since v2 [2]:
>>> - Use bit arithmetics instead of bitfileds (Jeff)
>>> - Add assertions about use of high type bits
>>> 
>>> Changes since v1 [1]:
>>> - Assert on encode for disconnected path (Jeff)
>>> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
>>> - Drop the O_PATH mount_fd API hack (Jeff)
>>> - Encode an explicit "connectable" flag in handle type
>>> 
>>> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
>>> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
>>> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
>>> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
>>> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/
>>> 
>>> Amir Goldstein (3):
>>> fs: prepare for "explicit connectable" file handles
>>> fs: name_to_handle_at() support for "explicit connectable" file
>>>   handles
>>> fs: open_by_handle_at() support for decoding "explicit connectable"
>>>   file handles
>>> 
>>> fs/exportfs/expfs.c        | 17 ++++++++-
>>> fs/fhandle.c               | 75 +++++++++++++++++++++++++++++++++++---
>>> include/linux/exportfs.h   | 13 +++++++
>>> include/uapi/linux/fcntl.h |  1 +
>>> 4 files changed, 98 insertions(+), 8 deletions(-)
>>> 
>>> --
>>> 2.34.1
>>> 
>> 
>> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>> 
>> Assuming this is going directly to Christian's tree.
>> 
>> I'm a little concerned about how this new facility might be
>> abused to get access to parts of the file system that a user
>> is not authorized to access.
> 
> That's exactly the sort of thing I would like to be reviewed,
> but what makes you feel concerned?
> 
> Are you concerned about handcrafted file handles?

Yes; a user could construct a file handle that could bypass
the usual authorization checks when it gets connected. It's
a little hare-brained and hand-wavy because this is a new
area for me.


> Correct me if I am wrong, but I think that any parts of the filesystem
> that could be accessed (by user with CAP_DAC_READ_SEARCH)
> using a handcrafted connectable file handle, could have also been
> accessed by the parent fid part before, so I do not see how connectable
> file handles create new ways to get access?
> 
>> But follow-up comments from Amir
>> suggest that (with the current code) it is difficult or
>> impossible to do.
>> 
>> Are there self-tests or unit-tests for exportfs?
> 
> There are fstests, particularly, the "exportfs" test group
> and I added this one for connectable file handles:
> 
> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
> 
> Did you mean another form of unit tests?

No, actually; fstests wfm. That's something I could set
up via kdevops and run on a regular basis.


--
Chuck Lever

Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 5147 bytes --]

On Fri, Oct 11, 2024 at 8:40 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 11, 2024, at 2:22 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Oct 11, 2024 at 4:24 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
> >>
> >>
> >>
> >>> On Oct 11, 2024, at 5:00 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> >>>
> >>> Christian,
> >>>
> >>> These patches bring the NFS connectable file handles feature to
> >>> userspace servers.
> >>>
> >>> They rely on your and Aleksa's changes recently merged to v6.12.
> >>>
> >>> This v4 incorporates the review comments on Jeff and Jan (thanks!)
> >>> and there does not seem to be any objection for this new API, so
> >>> I think it is ready for staging.
> >>>
> >>> The API I chose for encoding conenctable file handles is pretty
> >>> conventional (AT_HANDLE_CONNECTABLE).
> >>>
> >>> open_by_handle_at(2) does not have AT_ flags argument, but also, I find
> >>> it more useful API that encoding a connectable file handle can mandate
> >>> the resolving of a connected fd, without having to opt-in for a
> >>> connected fd independently.
> >>>
> >>> I chose to implemnent this by using upper bits in the handle type field
> >>> It may be that out-of-tree filesystems return a handle type with upper
> >>> bits set, but AFAIK, no in-tree filesystem does that.
> >>> I added some warnings just in case we encouter that.
> >>>
> >>> I have written an fstest [4] and a man page draft [5] for the feature.
> >>>
> >>> Thanks,
> >>> Amir.
> >>>
> >>> Changes since v3 [3]:
> >>> - Relax WARN_ON in decode and replace with pr_warn in encode (Jeff)
> >>> - Loose the macro FILEID_USER_TYPE_IS_VALID() (Jan)
> >>> - Add explicit check for negative type values (Jan)
> >>> - Added fstest and man-page draft
> >>>
> >>> Changes since v2 [2]:
> >>> - Use bit arithmetics instead of bitfileds (Jeff)
> >>> - Add assertions about use of high type bits
> >>>
> >>> Changes since v1 [1]:
> >>> - Assert on encode for disconnected path (Jeff)
> >>> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
> >>> - Drop the O_PATH mount_fd API hack (Jeff)
> >>> - Encode an explicit "connectable" flag in handle type
> >>>
> >>> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
> >>> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
> >>> [3] https://lore.kernel.org/linux-fsdevel/20241008152118.453724-1-amir73il@gmail.com/
> >>> [4] https://github.com/amir73il/xfstests/commits/connectable-fh/
> >>> [5] https://github.com/amir73il/man-pages/commits/connectable-fh/
> >>>
> >>> Amir Goldstein (3):
> >>> fs: prepare for "explicit connectable" file handles
> >>> fs: name_to_handle_at() support for "explicit connectable" file
> >>>   handles
> >>> fs: open_by_handle_at() support for decoding "explicit connectable"
> >>>   file handles
> >>>
> >>> fs/exportfs/expfs.c        | 17 ++++++++-
> >>> fs/fhandle.c               | 75 +++++++++++++++++++++++++++++++++++---
> >>> include/linux/exportfs.h   | 13 +++++++
> >>> include/uapi/linux/fcntl.h |  1 +
> >>> 4 files changed, 98 insertions(+), 8 deletions(-)
> >>>
> >>> --
> >>> 2.34.1
> >>>
> >>
> >> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
> >>
> >> Assuming this is going directly to Christian's tree.
> >>
> >> I'm a little concerned about how this new facility might be
> >> abused to get access to parts of the file system that a user
> >> is not authorized to access.
> >
> > That's exactly the sort of thing I would like to be reviewed,
> > but what makes you feel concerned?
> >
> > Are you concerned about handcrafted file handles?
>
> Yes; a user could construct a file handle that could bypass
> the usual authorization checks when it gets connected. It's
> a little hare-brained and hand-wavy because this is a new
> area for me.
>

A malformed file handle is indeed a concern - it has always been,
but in order to exploit one, an attacker would actually need to have
a filesystem exported to nfs (to the attacking client machine).

With commit 620c266f3949 ("fhandle: relax open_by_handle_at()
permission checks"), attackers that have non-root access to a machine
could also try to exploit filesystem bugs with malformed file handles.

By adding support for connectable file handles, attackers could try
to exploit bugs in ->fh_to_parent() implementations - bugs that would
not have been exploitable so far unless filesystem is exported to nfs with
subtree_check, which is quite rare IIUC.

So I did an audit of the in-tree ->fh_to_{dentry,parent}() implementations.
AFAICT all implementations properly check buffer length before trying
to decode the handle... except for ceph.

It looks to me like __snapfh_to_dentry() does not check buffer length
before assuming that ceph_nfs_snapfh can be accessed.

Ilya,

Do you agree with my analysis?
Please see the attached fix patch.
Let me know if you want me to post it on ceph list or if that is sufficient.

Thanks,
Amir.

[-- Attachment #2: 0001-ceph-fix-bounds-check-for-decoding-fh-of-snapshot-fi.patch --]
[-- Type: text/x-patch, Size: 1233 bytes --]

From 229c0b6663eef65436031b782f858d02d5dec938 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Mon, 14 Oct 2024 10:46:20 +0200
Subject: [PATCH] ceph: fix bounds check for decoding fh of snapshot file

Prevent attackers from using malformed ceph file handle with type
FILEID_BTRFS_WITH_PARENT to cause out of bounds access.

Fixes: 570df4e9c23f ("ceph: snapshot nfs re-export")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ceph/export.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 44451749c544..4a8d00e9910a 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -302,6 +302,8 @@ static struct dentry *ceph_fh_to_dentry(struct super_block *sb,
 
 	if (fh_type == FILEID_BTRFS_WITH_PARENT) {
 		struct ceph_nfs_snapfh *sfh = (void *)fid->raw;
+		if (fh_len < sizeof(*sfh) / 4)
+			return NULL;
 		return __snapfh_to_dentry(sb, sfh, false);
 	}
 
@@ -422,6 +424,8 @@ static struct dentry *ceph_fh_to_parent(struct super_block *sb,
 
 	if (fh_type == FILEID_BTRFS_WITH_PARENT) {
 		struct ceph_nfs_snapfh *sfh = (void *)fid->raw;
+		if (fh_len < sizeof(*sfh) / 4)
+			return NULL;
 		return __snapfh_to_dentry(sb, sfh, true);
 	}
 
-- 
2.34.1

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

[Jan Kara]

On Fri 11-10-24 11:00:21, 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>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/exportfs/expfs.c      | 17 +++++++++++++++--
>  fs/fhandle.c             |  7 +++++++
>  include/linux/exportfs.h | 11 +++++++++++
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index 4f2dd4ab4486..0c899cfba578 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -382,14 +382,24 @@ 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 (type > 0 && FILEID_USER_FLAGS(type)) {
> +		pr_warn_once("%s: unexpected fh type value 0x%x from fstype %s.\n",
> +			     __func__, type, inode->i_sb->s_type->name);
> +		return -EINVAL;
> +	}
> +
> +	return type;
>  
> -	return nop->encode_fh(inode, fid->raw, max_len, parent);
>  }
>  EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
>  
> @@ -436,6 +446,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
>  	char nbuf[NAME_MAX+1];
>  	int err;
>  
> +	if (fileid_type < 0 || FILEID_USER_FLAGS(fileid_type))
> +		return ERR_PTR(-EINVAL);
> +
>  	/*
>  	 * 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..218511f38cbb 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -307,6 +307,11 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		retval = -EINVAL;
>  		goto out_path;
>  	}
> +	if (f_handle.handle_type < 0 ||
> +	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS) {
> +		retval = -EINVAL;
> +		goto out_path;
> +	}
>  	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
>  			 GFP_KERNEL);
>  	if (!handle) {
> @@ -322,6 +327,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..5e14d4500a75 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -160,6 +160,17 @@ 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)
> +
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
>   * @encode_fh:      encode a file handle fragment from a dentry
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 0/3] API for exporting connectable file handles to userspace

[Christian Brauner]

On Fri, 11 Oct 2024 11:00:20 +0200, Amir Goldstein wrote:
> Christian,
> 
> These patches bring the NFS connectable file handles feature to
> userspace servers.
> 
> They rely on your and Aleksa's changes recently merged to v6.12.
> 
> [...]

Applied to the vfs.exportfs branch of the vfs/vfs.git tree.
Patches in the vfs.exportfs branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.exportfs

[1/3] fs: prepare for "explicit connectable" file handles
      https://git.kernel.org/vfs/vfs/c/7f0e6b304c6c
[2/3] fs: name_to_handle_at() support for "explicit connectable" file handles
      https://git.kernel.org/vfs/vfs/c/4142418cafc9
[3/3] fs: open_by_handle_at() support for decoding "explicit connectable" file handles
      https://git.kernel.org/vfs/vfs/c/81667fcf9b82

Re: [PATCH v4 3/3] fs: open_by_handle_at() support for decoding "explicit connectable" file handles

[Jeff Layton]

On Fri, 2024-10-11 at 11:00 +0200, Amir Goldstein wrote:
> Teach open_by_handle_at(2) about the type format of "explicit connectable"
> file handles that were created using the AT_HANDLE_CONNECTABLE flag to
> name_to_handle_at(2).
> 
> When decoding an "explicit connectable" file handles, name_to_handle_at(2)
> should fail if it cannot open a "connected" fd with known path, which is
> accessible (to capable user) from mount fd path.
> 
> Note that this does not check if the path is accessible to the calling
> user, just that it is accessible wrt the mount namesapce, so if there
> is no "connected" alias, or if parts of the path are hidden in the
> mount namespace, open_by_handle_at(2) will return -ESTALE.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c             | 20 +++++++++++++++++++-
>  include/linux/exportfs.h |  2 +-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 8339a1041025..75cfd190cd69 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -246,7 +246,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  
>  	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
>  		retval = 1;
> -	WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> +	/*
> +	 * exportfs_decode_fh_raw() does not call acceptable() callback with
> +	 * a disconnected directory dentry, so we should have reached either
> +	 * mount fd directory or sb root.
> +	 */
> +	if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> +		WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
>  	dput(d);
>  	return retval;
>  }
> @@ -350,6 +356,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		retval = -EINVAL;
>  		goto out_path;
>  	}
> +
>  	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
>  			 GFP_KERNEL);
>  	if (!handle) {
> @@ -365,6 +372,17 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		goto out_handle;
>  	}
>  
> +	/*
> +	 * If handle was encoded with AT_HANDLE_CONNECTABLE, verify that we
> +	 * are decoding an fd with connected path, which is accessible from
> +	 * the mount fd path.
> +	 */
> +	if (f_handle.handle_type & FILEID_IS_CONNECTABLE) {
> +		ctx.fh_flags |= EXPORT_FH_CONNECTABLE;
> +		ctx.flags |= HANDLE_CHECK_SUBTREE;
> +	}
> +	if (f_handle.handle_type & FILEID_IS_DIR)
> +		ctx.fh_flags |= EXPORT_FH_DIR_ONLY;
>  	/* Filesystem code should not be exposed to user flags */
>  	handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
>  	retval = do_handle_to_path(handle, path, &ctx);
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 4ee42b2cf4ab..fcab6ab1d38a 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -171,7 +171,7 @@ struct fid {
>  /* Flags supported in encoded handle_type that is exported to user */
>  #define FILEID_IS_CONNECTABLE	0x10000
>  #define FILEID_IS_DIR		0x20000
> -#define FILEID_VALID_USER_FLAGS	(0)
> +#define FILEID_VALID_USER_FLAGS	(FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
>  
>  /**
>   * struct export_operations - for nfsd to communicate with file systems

Thought I had sent this last week, but I got sidetracked:

Reviewed-by: Jeff Layton <jlayton@kernel.org>