[PATCH v2 0/2] API for exporting connectable file handles to userspace

[PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

Jeff,

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

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

I am aware of the usability implications with connectable file handles,
which are not consistent throughout the inode lifetime (i.e. when moved
to another parent), but the nfsd feature does exists and some users (me)
are interested in exporting this feature to userspace.

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.

Therefore, the whacky API from RFC was replaced with an explicit
connectable flag in the unused (*) upper bits of the handle_type.

(*) It may be valid for filesystems to return a handle type with upper
bits set, but AFAIK, no filesystem does that.

I chose to implemnent this by re-farmatting struct file_handle using bit
feilds.  While using bit fields in UAPI is a questionable practice,
file_handle is not actually in the UAPI and the legacy struct
file_handle which is described in the man page, is binary compatible
with the modified kernel definition with bit fields.
If this is a problem, I can add (and strip) the connectable bit using
plain arithmetics.

Thought and flames are welcome.

Thanks,
Amir.

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/

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

 fs/fhandle.c               | 70 ++++++++++++++++++++++++++++++++++----
 include/linux/exportfs.h   |  2 ++
 include/linux/fs.h         |  3 +-
 include/uapi/linux/fcntl.h |  1 +
 4 files changed, 69 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] 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               | 53 ++++++++++++++++++++++++++++++++++----
 include/linux/exportfs.h   |  2 ++
 include/linux/fs.h         |  3 ++-
 include/uapi/linux/fcntl.h |  1 +
 4 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 8cb665629f4a..6c87f1764235 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);
@@ -54,6 +62,7 @@ static long do_sys_name_to_handle(const struct path *path,
 	handle_bytes = handle_dwords * sizeof(u32);
 	handle->handle_bytes = handle_bytes;
 	if ((handle->handle_bytes > f_handle.handle_bytes) ||
+	    WARN_ON_ONCE(retval > FILEID_INVALID) ||
 	    (retval == FILEID_INVALID) || (retval < 0)) {
 		/* As per old exportfs_encode_fh documentation
 		 * we could return ENOSPC to indicate overflow
@@ -67,8 +76,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) {
+			if (d_is_dir(path->dentry))
+				fh_flags |= EXPORT_FH_DIR_ONLY;
+			handle->handle_flags = fh_flags;
+		}
 		retval = 0;
+	}
 	/* copy the mount id */
 	if (unique_mntid) {
 		if (put_user(real_mount(path->mnt)->mnt_id_unique,
@@ -109,15 +133,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);
@@ -307,6 +346,10 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		retval = -EINVAL;
 		goto out_path;
 	}
+	if (f_handle.handle_flags) {
+		retval = -EINVAL;
+		goto out_path;
+	}
 	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
 			 GFP_KERNEL);
 	if (!handle) {
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 893a1d21dc1c..96b62e502f71 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -159,6 +159,8 @@ struct fid {
 #define EXPORT_FH_CONNECTABLE	0x1 /* Encode file handle with parent */
 #define EXPORT_FH_FID		0x2 /* File handle may be non-decodeable */
 #define EXPORT_FH_DIR_ONLY	0x4 /* Only decode file handle for a directory */
+/* Flags allowed in encoded handle_flags that is exported to user */
+#define EXPORT_FH_USER_FLAGS	(EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
 
 /**
  * struct export_operations - for nfsd to communicate with file systems
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0df3e5f0dd2b..bcf8f750b309 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1071,7 +1071,8 @@ struct file {
 
 struct file_handle {
 	__u32 handle_bytes;
-	int handle_type;
+	int handle_type:16;
+	int handle_flags:16;
 	/* file identifier */
 	unsigned char f_handle[] __counted_by(handle_bytes);
 };
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 v2 2/2] 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 | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 6c87f1764235..68e59141f67b 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -247,7 +247,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;
 }
@@ -346,10 +352,19 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 		retval = -EINVAL;
 		goto out_path;
 	}
-	if (f_handle.handle_flags) {
+	if (f_handle.handle_flags & ~EXPORT_FH_USER_FLAGS) {
 		retval = -EINVAL;
 		goto out_path;
 	}
+	/*
+	 * 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.
+	 */
+	ctx.fh_flags |= f_handle.handle_flags;
+	if (ctx.fh_flags & EXPORT_FH_CONNECTABLE)
+		ctx.flags |= HANDLE_CHECK_SUBTREE;
+
 	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
 			 GFP_KERNEL);
 	if (!handle) {
-- 
2.34.1

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Christian Brauner]

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

This seems the best option to me too if this api is to be added.

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
>
> > 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.
>
> This seems the best option to me too if this api is to be added.

Thanks.

Jeff, Chuck,

Any thoughts on this?

Thanks,
Amir.

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Chuck Lever III]

> On Oct 7, 2024, at 11:26 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> 
> On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
>> 
>>> 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.
>> 
>> This seems the best option to me too if this api is to be added.
> 
> Thanks.
> 
> Jeff, Chuck,
> 
> Any thoughts on this?

I don't have anything clever to add.


--
Chuck Lever

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Mon, Oct 7, 2024 at 8:09 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Oct 7, 2024, at 11:26 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> >>
> >>> 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.
> >>
> >> This seems the best option to me too if this api is to be added.
> >
> > Thanks.
> >
> > Jeff, Chuck,
> >
> > Any thoughts on this?
>
> I don't have anything clever to add.
>

Sometimes that's a good thing ;)

FYI, I wrote a draft for the would be man page update to go with the
proposed flag:

   name_to_handle_at()
       The name_to_handle_at() system call returns a file handle and a mount ID
       corresponding to the file specified by the dirfd and pathname arguments.
...
       When  flags  contain  the  AT_HANDLE_FID (since Linux 6.5) flag,
       the caller indicates that the returned file_handle is needed to
identify the filesystem object,
       and not for opening the file later,
       so it should be expected that a subsequent call to open_by_handle_at()
       with the returned file_handle may fail.

+     When flags contain the AT_HANDLE_CONNECTABLE (since Linux 6.x) flag,
+     the caller indicates that the returned file_handle is needed to
open a file with known path later,
+     so it should be expected that a subsequent call to open_by_handle_at()
+     with the returned  file_handle may fail if the file was moved,
but otherwise,
+     the path of the opened file is expected to be visible from the
/proc/pid/fd/* magic link.
+     This flag can not be used in combination with the flags
AT_HANDLE_FID, and AT_EMPTY_PATH.
...
       ESTALE The specified handle is not valid for opening a file.
              This error will occur if, for example, the file has been deleted.
              This error can also occur if the handle was acquired
using the AT_HANDLE_FID flag
              and the filesystem does not support open_by_handle_at().
+            This error can also occur if the handle was acquired
using the AT_HANDLE_CONNECTABLE
+            flag and the file was moved to a different parent.
--

Apropos man page,

Aleksa,

Are you planning to post a man page patch for AT_HANDLE_MNT_ID_UNIQUE?
I realize that there is no man page maintainer at the moment, but I myself
would love to have this patch in my man-page updates queue,
until a man-page maintainer shows up, because, well, my update to
AT_HANDLE_CONNECTABLE depends on it and so will my changes to
fanotify to support mount/unmount events if/when I will get to them.

Thanks,
Amir.

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Jeff Layton]

On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > 
> > > 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.
> > 
> > This seems the best option to me too if this api is to be added.
> 
> Thanks.
> 
> Jeff, Chuck,
> 
> Any thoughts on this?
> 

Sorry for the delay. I think encoding the new flag into the fh itself
is a reasonable approach.

I'm less thrilled with using bitfields for this, just because I have a
general dislike of them, and they aren't implemented the same way on
all arches. Would it break ABI if we just turned the handle_type int
into two uint16_t's instead?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > > 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.
> > >
> > > This seems the best option to me too if this api is to be added.
> >
> > Thanks.
> >
> > Jeff, Chuck,
> >
> > Any thoughts on this?
> >
>
> Sorry for the delay. I think encoding the new flag into the fh itself
> is a reasonable approach.
>

Adding Jan.
Sorry I forgot to CC you on the patches, but struct file_handle is officially
a part of fanotify ABI, so your ACK is also needed on this change.

> I'm less thrilled with using bitfields for this, just because I have a
> general dislike of them, and they aren't implemented the same way on
> all arches. Would it break ABI if we just turned the handle_type int
> into two uint16_t's instead?

I think it will because this will not be backward compat on LE arch:

 struct file_handle {
        __u32 handle_bytes;
-       int handle_type;
+      __u16 handle_type;
+      __u16 handle_flags;
        /* file identifier */
        unsigned char f_handle[] __counted_by(handle_bytes);
 };

But I can also do without the bitfileds, maybe it's better this way.
See diff from v2:

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 4ce4ffddec62..64d44fc61d43 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -87,9 +87,9 @@ static long do_sys_name_to_handle(const struct path *path,
                 * 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 |= EXPORT_FH_DIR_ONLY;
-                       handle->handle_flags = fh_flags;
+                               fh_flags |= FILEID_IS_DIR;
                }
                retval = 0;
        }
@@ -352,7 +352,7 @@ static int handle_to_path(int mountdirfd, struct
file_handle __user *ufh,
                retval = -EINVAL;
                goto out_path;
        }
-       if (f_handle.handle_flags & ~EXPORT_FH_USER_FLAGS) {
+       if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) {
                retval = -EINVAL;
                goto out_path;
        }
@@ -377,10 +377,14 @@ static int handle_to_path(int mountdirfd, struct
file_handle __user *ufh,
         * are decoding an fd with connected path, which is accessible from
         * the mount fd path.
         */
-       ctx.fh_flags |= f_handle.handle_flags;
-       if (ctx.fh_flags & EXPORT_FH_CONNECTABLE)
+       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);

 out_handle:
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 96b62e502f71..3e60bac74fa3 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -159,8 +159,17 @@ struct fid {
 #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
 #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
 #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
directory */
-/* Flags allowed in encoded handle_flags that is exported to user */
-#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
+
+/* Flags supported in encoded handle_type that is exported to user */
+#define FILEID_USER_FLAGS_MASK 0xffff0000
+#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
+
+#define FILEID_IS_CONNECTABLE  0x10000
+#define FILEID_IS_DIR          0x40000
+#define FILEID_VALID_USER_FLAGS        (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
+
+#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
diff --git a/include/linux/fs.h b/include/linux/fs.h
index cca7e575d1f8..6329fec40872 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1071,8 +1071,7 @@ struct file {

 struct file_handle {
        __u32 handle_bytes;
-       int handle_type:16;
-       int handle_flags:16;
+       int handle_type;
        /* file identifier */
        unsigned char f_handle[] __counted_by(handle_bytes);
 };

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Jeff Layton]

On Tue, 2024-10-08 at 15:11 +0200, Amir Goldstein wrote:
> On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > 
> > > > > 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.
> > > > 
> > > > This seems the best option to me too if this api is to be added.
> > > 
> > > Thanks.
> > > 
> > > Jeff, Chuck,
> > > 
> > > Any thoughts on this?
> > > 
> > 
> > Sorry for the delay. I think encoding the new flag into the fh itself
> > is a reasonable approach.
> > 
> 
> Adding Jan.
> Sorry I forgot to CC you on the patches, but struct file_handle is officially
> a part of fanotify ABI, so your ACK is also needed on this change.
> 
> > I'm less thrilled with using bitfields for this, just because I have a
> > general dislike of them, and they aren't implemented the same way on
> > all arches. Would it break ABI if we just turned the handle_type int
> > into two uint16_t's instead?
> 
> I think it will because this will not be backward compat on LE arch:
> 
>  struct file_handle {
>         __u32 handle_bytes;
> -       int handle_type;
> +      __u16 handle_type;
> +      __u16 handle_flags;
>         /* file identifier */
>         unsigned char f_handle[] __counted_by(handle_bytes);
>  };
> 

Ok, good point.

> But I can also do without the bitfileds, maybe it's better this way.
> See diff from v2:
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 4ce4ffddec62..64d44fc61d43 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -87,9 +87,9 @@ static long do_sys_name_to_handle(const struct path *path,
>                  * 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 |= EXPORT_FH_DIR_ONLY;
> -                       handle->handle_flags = fh_flags;
> +                               fh_flags |= FILEID_IS_DIR;
>                 }
>                 retval = 0;
>         }
> @@ -352,7 +352,7 @@ static int handle_to_path(int mountdirfd, struct
> file_handle __user *ufh,
>                 retval = -EINVAL;
>                 goto out_path;
>         }
> -       if (f_handle.handle_flags & ~EXPORT_FH_USER_FLAGS) {
> +       if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) {
>                 retval = -EINVAL;
>                 goto out_path;
>         }
> @@ -377,10 +377,14 @@ static int handle_to_path(int mountdirfd, struct
> file_handle __user *ufh,
>          * are decoding an fd with connected path, which is accessible from
>          * the mount fd path.
>          */
> -       ctx.fh_flags |= f_handle.handle_flags;
> -       if (ctx.fh_flags & EXPORT_FH_CONNECTABLE)
> +       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);
> 
>  out_handle:
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 96b62e502f71..3e60bac74fa3 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -159,8 +159,17 @@ struct fid {
>  #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
>  #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
>  #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
> directory */
> -/* Flags allowed in encoded handle_flags that is exported to user */
> -#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)

Maybe add a nice comment here about how the handle_type word is
partitioned?

> +
> +/* Flags supported in encoded handle_type that is exported to user */
> +#define FILEID_USER_FLAGS_MASK 0xffff0000
> +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> +
> +#define FILEID_IS_CONNECTABLE  0x10000
> +#define FILEID_IS_DIR          0x40000
> +#define FILEID_VALID_USER_FLAGS        (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
> +
> +#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
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index cca7e575d1f8..6329fec40872 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1071,8 +1071,7 @@ struct file {
> 
>  struct file_handle {
>         __u32 handle_bytes;
> -       int handle_type:16;
> -       int handle_flags:16;
> +       int handle_type;
>         /* file identifier */
>         unsigned char f_handle[] __counted_by(handle_bytes);
>  };


I like that better than bitfields, fwiw.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Tue, Oct 8, 2024 at 3:43 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2024-10-08 at 15:11 +0200, Amir Goldstein wrote:
> > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > > 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.
> > > > >
> > > > > This seems the best option to me too if this api is to be added.
> > > >
> > > > Thanks.
> > > >
> > > > Jeff, Chuck,
> > > >
> > > > Any thoughts on this?
> > > >
> > >
> > > Sorry for the delay. I think encoding the new flag into the fh itself
> > > is a reasonable approach.
> > >
> >
> > Adding Jan.
> > Sorry I forgot to CC you on the patches, but struct file_handle is officially
> > a part of fanotify ABI, so your ACK is also needed on this change.
> >
> > > I'm less thrilled with using bitfields for this, just because I have a
> > > general dislike of them, and they aren't implemented the same way on
> > > all arches. Would it break ABI if we just turned the handle_type int
> > > into two uint16_t's instead?
> >
> > I think it will because this will not be backward compat on LE arch:
> >
> >  struct file_handle {
> >         __u32 handle_bytes;
> > -       int handle_type;
> > +      __u16 handle_type;
> > +      __u16 handle_flags;
> >         /* file identifier */
> >         unsigned char f_handle[] __counted_by(handle_bytes);
> >  };
> >
>
> Ok, good point.
>
> > But I can also do without the bitfileds, maybe it's better this way.
> > See diff from v2:
> >
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 4ce4ffddec62..64d44fc61d43 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -87,9 +87,9 @@ static long do_sys_name_to_handle(const struct path *path,
> >                  * 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 |= EXPORT_FH_DIR_ONLY;
> > -                       handle->handle_flags = fh_flags;
> > +                               fh_flags |= FILEID_IS_DIR;
> >                 }
> >                 retval = 0;
> >         }
> > @@ -352,7 +352,7 @@ static int handle_to_path(int mountdirfd, struct
> > file_handle __user *ufh,
> >                 retval = -EINVAL;
> >                 goto out_path;
> >         }
> > -       if (f_handle.handle_flags & ~EXPORT_FH_USER_FLAGS) {
> > +       if (!FILEID_USER_TYPE_IS_VALID(f_handle.handle_type)) {
> >                 retval = -EINVAL;
> >                 goto out_path;
> >         }
> > @@ -377,10 +377,14 @@ static int handle_to_path(int mountdirfd, struct
> > file_handle __user *ufh,
> >          * are decoding an fd with connected path, which is accessible from
> >          * the mount fd path.
> >          */
> > -       ctx.fh_flags |= f_handle.handle_flags;
> > -       if (ctx.fh_flags & EXPORT_FH_CONNECTABLE)
> > +       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);
> >
> >  out_handle:
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index 96b62e502f71..3e60bac74fa3 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -159,8 +159,17 @@ struct fid {
> >  #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
> >  #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
> >  #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
> > directory */
> > -/* Flags allowed in encoded handle_flags that is exported to user */
> > -#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
>
> Maybe add a nice comment here about how the handle_type word is
> partitioned?
>

Sure, I added:

+/*
+ * 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().
+ */

> > +
> > +/* Flags supported in encoded handle_type that is exported to user */
> > +#define FILEID_USER_FLAGS_MASK 0xffff0000
> > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> > +
> > +#define FILEID_IS_CONNECTABLE  0x10000
> > +#define FILEID_IS_DIR          0x40000
> > +#define FILEID_VALID_USER_FLAGS        (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
> > +
> > +#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
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index cca7e575d1f8..6329fec40872 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1071,8 +1071,7 @@ struct file {
> >
> >  struct file_handle {
> >         __u32 handle_bytes;
> > -       int handle_type:16;
> > -       int handle_flags:16;
> > +       int handle_type;
> >         /* file identifier */
> >         unsigned char f_handle[] __counted_by(handle_bytes);
> >  };
>
>
> I like that better than bitfields, fwiw.

Me too :)

I also added assertions in case there is an out of tree fs with weird
handle type:

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

I will post it in v3 after testing.

Thanks for the review!
Amir.

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Jan Kara]

On Tue 08-10-24 15:11:39, Amir Goldstein wrote:
> On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > > 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.
> > > >
> > > > This seems the best option to me too if this api is to be added.
> > >
> > > Thanks.
> > >
> > > Jeff, Chuck,
> > >
> > > Any thoughts on this?
> > >
> >
> > Sorry for the delay. I think encoding the new flag into the fh itself
> > is a reasonable approach.
> >
> 
> Adding Jan.
> Sorry I forgot to CC you on the patches, but struct file_handle is officially
> a part of fanotify ABI, so your ACK is also needed on this change.

Thanks. I've actually seen this series on list, went "eww bitfields, let's
sleep to this" and never got back to it.

> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 96b62e502f71..3e60bac74fa3 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -159,8 +159,17 @@ struct fid {
>  #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
>  #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
>  #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
> directory */
> -/* Flags allowed in encoded handle_flags that is exported to user */
> -#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
> +
> +/* Flags supported in encoded handle_type that is exported to user */
> +#define FILEID_USER_FLAGS_MASK 0xffff0000
> +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> +
> +#define FILEID_IS_CONNECTABLE  0x10000
> +#define FILEID_IS_DIR          0x40000
> +#define FILEID_VALID_USER_FLAGS        (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)

FWIW I prefer this variant much over bitfields as their binary format
depends on the compiler which leads to unpleasant surprises sooner rather
than later.

> +#define FILEID_USER_TYPE_IS_VALID(type) \
> +       (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)

The macro name is confusing because it speaks about type but actually
checks flags. Frankly, I'd just fold this in the single call site to make
things obvious.

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index cca7e575d1f8..6329fec40872 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1071,8 +1071,7 @@ struct file {
> 
>  struct file_handle {
>         __u32 handle_bytes;
> -       int handle_type:16;
> -       int handle_flags:16;
> +       int handle_type;

Maybe you want to make handle_type unsigned when you treat it (partially)
as flags? Otherwise some constructs can lead to surprises with sign
extension etc...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Wed, Oct 9, 2024 at 11:40 AM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 08-10-24 15:11:39, Amir Goldstein wrote:
> > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > > 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.
> > > > >
> > > > > This seems the best option to me too if this api is to be added.
> > > >
> > > > Thanks.
> > > >
> > > > Jeff, Chuck,
> > > >
> > > > Any thoughts on this?
> > > >
> > >
> > > Sorry for the delay. I think encoding the new flag into the fh itself
> > > is a reasonable approach.
> > >
> >
> > Adding Jan.
> > Sorry I forgot to CC you on the patches, but struct file_handle is officially
> > a part of fanotify ABI, so your ACK is also needed on this change.
>
> Thanks. I've actually seen this series on list, went "eww bitfields, let's
> sleep to this" and never got back to it.
>
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index 96b62e502f71..3e60bac74fa3 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -159,8 +159,17 @@ struct fid {
> >  #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
> >  #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
> >  #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
> > directory */
> > -/* Flags allowed in encoded handle_flags that is exported to user */
> > -#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
> > +
> > +/* Flags supported in encoded handle_type that is exported to user */
> > +#define FILEID_USER_FLAGS_MASK 0xffff0000
> > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> > +
> > +#define FILEID_IS_CONNECTABLE  0x10000
> > +#define FILEID_IS_DIR          0x40000
> > +#define FILEID_VALID_USER_FLAGS        (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
>
> FWIW I prefer this variant much over bitfields as their binary format
> depends on the compiler which leads to unpleasant surprises sooner rather
> than later.
>mask
> > +#define FILEID_USER_TYPE_IS_VALID(type) \
> > +       (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)
>
> The macro name is confusing

Confusing enough to hide the fact that it was negated in v2...

> because it speaks about type but actually
> checks flags. Frankly, I'd just fold this in the single call site to make
> things obvious.

Agree. but see below...

>
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index cca7e575d1f8..6329fec40872 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1071,8 +1071,7 @@ struct file {
> >
> >  struct file_handle {
> >         __u32 handle_bytes;
> > -       int handle_type:16;
> > -       int handle_flags:16;
> > +       int handle_type;
>
> Maybe you want to make handle_type unsigned when you treat it (partially)
> as flags? Otherwise some constructs can lead to surprises with sign
> extension etc...

That seems like a good idea, but when I look at:

        /* we ask for a non connectable maybe decodeable file handle */
        retval = exportfs_encode_fh(path->dentry,
                                    (struct fid *)handle->f_handle,
                                    &handle_dwords, fh_flags);
        handle->handle_type = retval;
        /* convert handle size to bytes */
        handle_bytes = handle_dwords * sizeof(u32);
        handle->handle_bytes = handle_bytes;
        if ((handle->handle_bytes > f_handle.handle_bytes) ||
            (retval == FILEID_INVALID) || (retval < 0)) {
                /* As per old exportfs_encode_fh documentation
                 * we could return ENOSPC to indicate overflow
                 * But file system returned 255 always. So handle
                 * both the values
                 */
                if (retval == FILEID_INVALID || retval == -ENOSPC)
                        retval = -EOVERFLOW;
                /*

I realize that we actually return negative values in handle_type
(-ESTALE would be quite common).

So we probably don't want to make handle_type unsigned now,
but maybe we do want a macro:

#define FILEID_IS_USER_TYPE_VALID(type) \
             ((type) >= 0 && \
              !(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS))

that will be used for the assertions instead of assuming that
FILEID_USER_FLAGS_MASK includes the sign bit.

huh?

Thanks,
Amir.

Re: [PATCH v2 0/2] API for exporting connectable file handles to userspace

[Amir Goldstein]

On Wed, Oct 9, 2024 at 5:16 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Wed, Oct 9, 2024 at 11:40 AM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 08-10-24 15:11:39, Amir Goldstein wrote:
> > > On Tue, Oct 8, 2024 at 1:07 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > >
> > > > On Mon, 2024-10-07 at 17:26 +0200, Amir Goldstein wrote:
> > > > > On Wed, Sep 25, 2024 at 11:14 AM Christian Brauner <brauner@kernel.org> wrote:
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > This seems the best option to me too if this api is to be added.
> > > > >
> > > > > Thanks.
> > > > >
> > > > > Jeff, Chuck,
> > > > >
> > > > > Any thoughts on this?
> > > > >
> > > >
> > > > Sorry for the delay. I think encoding the new flag into the fh itself
> > > > is a reasonable approach.
> > > >
> > >
> > > Adding Jan.
> > > Sorry I forgot to CC you on the patches, but struct file_handle is officially
> > > a part of fanotify ABI, so your ACK is also needed on this change.
> >
> > Thanks. I've actually seen this series on list, went "eww bitfields, let's
> > sleep to this" and never got back to it.
> >
> > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > index 96b62e502f71..3e60bac74fa3 100644
> > > --- a/include/linux/exportfs.h
> > > +++ b/include/linux/exportfs.h
> > > @@ -159,8 +159,17 @@ struct fid {
> > >  #define EXPORT_FH_CONNECTABLE  0x1 /* Encode file handle with parent */
> > >  #define EXPORT_FH_FID          0x2 /* File handle may be non-decodeable */
> > >  #define EXPORT_FH_DIR_ONLY     0x4 /* Only decode file handle for a
> > > directory */
> > > -/* Flags allowed in encoded handle_flags that is exported to user */
> > > -#define EXPORT_FH_USER_FLAGS   (EXPORT_FH_CONNECTABLE | EXPORT_FH_DIR_ONLY)
> > > +
> > > +/* Flags supported in encoded handle_type that is exported to user */
> > > +#define FILEID_USER_FLAGS_MASK 0xffff0000
> > > +#define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> > > +
> > > +#define FILEID_IS_CONNECTABLE  0x10000
> > > +#define FILEID_IS_DIR          0x40000
> > > +#define FILEID_VALID_USER_FLAGS        (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
> >
> > FWIW I prefer this variant much over bitfields as their binary format
> > depends on the compiler which leads to unpleasant surprises sooner rather
> > than later.
> >mask
> > > +#define FILEID_USER_TYPE_IS_VALID(type) \
> > > +       (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)
> >
> > The macro name is confusing
>
> Confusing enough to hide the fact that it was negated in v2...
>
> > because it speaks about type but actually
> > checks flags. Frankly, I'd just fold this in the single call site to make
> > things obvious.
>
> Agree. but see below...
>
> >
> > > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > > index cca7e575d1f8..6329fec40872 100644
> > > --- a/include/linux/fs.h
> > > +++ b/include/linux/fs.h
> > > @@ -1071,8 +1071,7 @@ struct file {
> > >
> > >  struct file_handle {
> > >         __u32 handle_bytes;
> > > -       int handle_type:16;
> > > -       int handle_flags:16;
> > > +       int handle_type;
> >
> > Maybe you want to make handle_type unsigned when you treat it (partially)
> > as flags? Otherwise some constructs can lead to surprises with sign
> > extension etc...
>
> That seems like a good idea, but when I look at:
>
>         /* we ask for a non connectable maybe decodeable file handle */
>         retval = exportfs_encode_fh(path->dentry,
>                                     (struct fid *)handle->f_handle,
>                                     &handle_dwords, fh_flags);
>         handle->handle_type = retval;
>         /* convert handle size to bytes */
>         handle_bytes = handle_dwords * sizeof(u32);
>         handle->handle_bytes = handle_bytes;
>         if ((handle->handle_bytes > f_handle.handle_bytes) ||
>             (retval == FILEID_INVALID) || (retval < 0)) {
>                 /* As per old exportfs_encode_fh documentation
>                  * we could return ENOSPC to indicate overflow
>                  * But file system returned 255 always. So handle
>                  * both the values
>                  */
>                 if (retval == FILEID_INVALID || retval == -ENOSPC)
>                         retval = -EOVERFLOW;
>                 /*
>
> I realize that we actually return negative values in handle_type
> (-ESTALE would be quite common).
>
> So we probably don't want to make handle_type unsigned now,
> but maybe we do want a macro:
>
> #define FILEID_IS_USER_TYPE_VALID(type) \
>              ((type) >= 0 && \
>               !(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS))
>
> that will be used for the assertions instead of assuming that
> FILEID_USER_FLAGS_MASK includes the sign bit.
>
> huh?

Scratch that, the assertions check for any user flags at all.
I will just open code type < 0 there as well.

Thanks,
Amir.