[PATCH 0/9] fhandle, pidfs: allow open_by_handle_at() purely based on file handle

[PATCH 0/9] fhandle, pidfs: allow open_by_handle_at() purely based on file handle

[Christian Brauner]

Various filesystems such as pidfs and drm support opening file handles
without having to require a file descriptor to identify the filesystem.
The filesystem are global single instances and can be trivially
identified solely on the information encoded in the file handle.

This makes it possible to not have to keep or acquire a sentinal file
descriptor just to pass it to open_by_handle_at() to identify the
filesystem. That's especially useful when such sentinel file descriptor
cannot or should not be acquired.

For pidfs this means a file handle can function as full replacement for
storing a pid in a file. Instead a file handle can be stored and
reopened purely based on the file handle.

Such autonomous file handles can be opened with or without specifying a
sentinal file descriptor. Userspace can trivially test for support by
trying to open the file handle with an invalid file descriptor.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Christian Brauner (9):
      fhandle: raise FILEID_IS_DIR in handle_type
      fhandle: hoist copy_from_user() above get_path_from_fd()
      fhandle: rename to get_path_anchor()
      pidfs: add pidfs_root_path() helper
      fhandle: reflow get_path_anchor()
      exportfs: add FILEID_PIDFS
      fhandle: add EXPORT_OP_AUTONOMOUS_HANDLES marker
      fhandle, pidfs: support open_by_handle_at() purely based on file handle
      selftests/pidfd: decode pidfd file handles withou having to specify an fd

 fs/fhandle.c                                       | 79 +++++++++++++---------
 fs/internal.h                                      |  1 +
 fs/pidfs.c                                         | 16 ++++-
 include/linux/exportfs.h                           | 15 +++-
 tools/testing/selftests/pidfd/Makefile             |  2 +-
 .../selftests/pidfd/pidfd_file_handle_test.c       | 54 +++++++++++++++
 6 files changed, 133 insertions(+), 34 deletions(-)
---
base-commit: 1ff46043a6745d56b37acfc888d6e2b4f4d90663
change-id: 20250619-work-pidfs-fhandle-b63ff35c4924

[PATCH 1/9] fhandle: raise FILEID_IS_DIR in handle_type

[Christian Brauner]

Currently FILEID_IS_DIR is raised in fh_flags which is wrong.
Raise it in handle->handle_type were it's supposed to be.

Fixes: c374196b2b9f ("fs: name_to_handle_at() support for "explicit connectable" file handles")
Cc: <stable@kernel.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 3e092ae6d142..66ff60591d17 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -88,7 +88,7 @@ static long do_sys_name_to_handle(const struct path *path,
 		if (fh_flags & EXPORT_FH_CONNECTABLE) {
 			handle->handle_type |= FILEID_IS_CONNECTABLE;
 			if (d_is_dir(path->dentry))
-				fh_flags |= FILEID_IS_DIR;
+				handle->handle_type |= FILEID_IS_DIR;
 		}
 		retval = 0;
 	}

-- 
2.47.2

Re: [PATCH 1/9] fhandle: raise FILEID_IS_DIR in handle_type

[Jan Kara]

On Mon 23-06-25 11:01:23, Christian Brauner wrote:
> Currently FILEID_IS_DIR is raised in fh_flags which is wrong.
> Raise it in handle->handle_type were it's supposed to be.
> 
> Fixes: c374196b2b9f ("fs: name_to_handle_at() support for "explicit connectable" file handles")
> Cc: <stable@kernel.org>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Indeed. Feel free to add:

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

								Honza

> ---
>  fs/fhandle.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 3e092ae6d142..66ff60591d17 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -88,7 +88,7 @@ static long do_sys_name_to_handle(const struct path *path,
>  		if (fh_flags & EXPORT_FH_CONNECTABLE) {
>  			handle->handle_type |= FILEID_IS_CONNECTABLE;
>  			if (d_is_dir(path->dentry))
> -				fh_flags |= FILEID_IS_DIR;
> +				handle->handle_type |= FILEID_IS_DIR;
>  		}
>  		retval = 0;
>  	}
> 
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/9] fhandle: hoist copy_from_user() above get_path_from_fd()

[Christian Brauner]

In follow-up patches we need access to @file_handle->handle_type
before we start caring about get_path_from_fd().

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 66ff60591d17..73f56f8e7d5d 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -323,13 +323,24 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 {
 	int retval = 0;
 	struct file_handle f_handle;
-	struct file_handle *handle = NULL;
+	struct file_handle *handle __free(kfree) = NULL;
 	struct handle_to_path_ctx ctx = {};
 	const struct export_operations *eops;
 
+	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
+		return -EFAULT;
+
+	if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
+	    (f_handle.handle_bytes == 0))
+		return -EINVAL;
+
+	if (f_handle.handle_type < 0 ||
+	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
+		return -EINVAL;
+
 	retval = get_path_from_fd(mountdirfd, &ctx.root);
 	if (retval)
-		goto out_err;
+		return retval;
 
 	eops = ctx.root.mnt->mnt_sb->s_export_op;
 	if (eops && eops->permission)
@@ -339,21 +350,6 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 	if (retval)
 		goto out_path;
 
-	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
-		retval = -EFAULT;
-		goto out_path;
-	}
-	if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
-	    (f_handle.handle_bytes == 0)) {
-		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) {
@@ -366,7 +362,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 			   &ufh->f_handle,
 			   f_handle.handle_bytes)) {
 		retval = -EFAULT;
-		goto out_handle;
+		goto out_path;
 	}
 
 	/*
@@ -384,11 +380,8 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 	handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
 	retval = do_handle_to_path(handle, path, &ctx);
 
-out_handle:
-	kfree(handle);
 out_path:
 	path_put(&ctx.root);
-out_err:
 	return retval;
 }
 

-- 
2.47.2

Re: [PATCH 2/9] fhandle: hoist copy_from_user() above get_path_from_fd()

[Jan Kara]

On Mon 23-06-25 11:01:24, Christian Brauner wrote:
> In follow-up patches we need access to @file_handle->handle_type
> before we start caring about get_path_from_fd().
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/fhandle.c | 35 ++++++++++++++---------------------
>  1 file changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 66ff60591d17..73f56f8e7d5d 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -323,13 +323,24 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  {
>  	int retval = 0;
>  	struct file_handle f_handle;
> -	struct file_handle *handle = NULL;
> +	struct file_handle *handle __free(kfree) = NULL;
>  	struct handle_to_path_ctx ctx = {};
>  	const struct export_operations *eops;
>  
> +	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> +		return -EFAULT;
> +
> +	if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
> +	    (f_handle.handle_bytes == 0))
> +		return -EINVAL;
> +
> +	if (f_handle.handle_type < 0 ||
> +	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
> +		return -EINVAL;
> +
>  	retval = get_path_from_fd(mountdirfd, &ctx.root);
>  	if (retval)
> -		goto out_err;
> +		return retval;
>  
>  	eops = ctx.root.mnt->mnt_sb->s_export_op;
>  	if (eops && eops->permission)
> @@ -339,21 +350,6 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  	if (retval)
>  		goto out_path;
>  
> -	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle))) {
> -		retval = -EFAULT;
> -		goto out_path;
> -	}
> -	if ((f_handle.handle_bytes > MAX_HANDLE_SZ) ||
> -	    (f_handle.handle_bytes == 0)) {
> -		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) {
> @@ -366,7 +362,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  			   &ufh->f_handle,
>  			   f_handle.handle_bytes)) {
>  		retval = -EFAULT;
> -		goto out_handle;
> +		goto out_path;
>  	}
>  
>  	/*
> @@ -384,11 +380,8 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  	handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
>  	retval = do_handle_to_path(handle, path, &ctx);
>  
> -out_handle:
> -	kfree(handle);
>  out_path:
>  	path_put(&ctx.root);
> -out_err:
>  	return retval;
>  }
>  
> 
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/9] fhandle: rename to get_path_anchor()

[Christian Brauner]

Rename as we're going to expand the function in the next step. The path
just serves as the anchor tying the decoding to the filesystem.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 73f56f8e7d5d..d8d32208c621 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -168,7 +168,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	return err;
 }
 
-static int get_path_from_fd(int fd, struct path *root)
+static int get_path_anchor(int fd, struct path *root)
 {
 	if (fd == AT_FDCWD) {
 		struct fs_struct *fs = current->fs;
@@ -338,7 +338,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
 		return -EINVAL;
 
-	retval = get_path_from_fd(mountdirfd, &ctx.root);
+	retval = get_path_anchor(mountdirfd, &ctx.root);
 	if (retval)
 		return retval;
 

-- 
2.47.2

Re: [PATCH 3/9] fhandle: rename to get_path_anchor()

[Jan Kara]

On Mon 23-06-25 11:01:25, Christian Brauner wrote:
> Rename as we're going to expand the function in the next step. The path
> just serves as the anchor tying the decoding to the filesystem.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

OK. Feel free to add:

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

								Honza

> ---
>  fs/fhandle.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 73f56f8e7d5d..d8d32208c621 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -168,7 +168,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	return err;
>  }
>  
> -static int get_path_from_fd(int fd, struct path *root)
> +static int get_path_anchor(int fd, struct path *root)
>  {
>  	if (fd == AT_FDCWD) {
>  		struct fs_struct *fs = current->fs;
> @@ -338,7 +338,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
>  		return -EINVAL;
>  
> -	retval = get_path_from_fd(mountdirfd, &ctx.root);
> +	retval = get_path_anchor(mountdirfd, &ctx.root);
>  	if (retval)
>  		return retval;
>  
> 
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 4/9] pidfs: add pidfs_root_path() helper

[Christian Brauner]

Allow to return the root of the global pidfs filesystem.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/internal.h |  1 +
 fs/pidfs.c    | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/fs/internal.h b/fs/internal.h
index 22ba066d1dba..ad256bccdc85 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -353,3 +353,4 @@ int anon_inode_getattr(struct mnt_idmap *idmap, const struct path *path,
 		       unsigned int query_flags);
 int anon_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
 		       struct iattr *attr);
+void pidfs_get_root(struct path *path);
diff --git a/fs/pidfs.c b/fs/pidfs.c
index 1cf66fd9961e..1b7bd14366dc 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -31,6 +31,14 @@
 static struct kmem_cache *pidfs_attr_cachep __ro_after_init;
 static struct kmem_cache *pidfs_xattr_cachep __ro_after_init;
 
+static struct path pidfs_root_path = {};
+
+void pidfs_get_root(struct path *path)
+{
+	*path = pidfs_root_path;
+	path_get(path);
+}
+
 /*
  * Stashes information that userspace needs to access even after the
  * process has been reaped.
@@ -1065,4 +1073,7 @@ void __init pidfs_init(void)
 	pidfs_mnt = kern_mount(&pidfs_type);
 	if (IS_ERR(pidfs_mnt))
 		panic("Failed to mount pidfs pseudo filesystem");
+
+	pidfs_root_path.mnt = pidfs_mnt;
+	pidfs_root_path.dentry = pidfs_mnt->mnt_root;
 }

-- 
2.47.2

Re: [PATCH 4/9] pidfs: add pidfs_root_path() helper

[Jan Kara]

On Mon 23-06-25 11:01:26, Christian Brauner wrote:
> Allow to return the root of the global pidfs filesystem.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/internal.h |  1 +
>  fs/pidfs.c    | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/fs/internal.h b/fs/internal.h
> index 22ba066d1dba..ad256bccdc85 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -353,3 +353,4 @@ int anon_inode_getattr(struct mnt_idmap *idmap, const struct path *path,
>  		       unsigned int query_flags);
>  int anon_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  		       struct iattr *attr);
> +void pidfs_get_root(struct path *path);
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index 1cf66fd9961e..1b7bd14366dc 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -31,6 +31,14 @@
>  static struct kmem_cache *pidfs_attr_cachep __ro_after_init;
>  static struct kmem_cache *pidfs_xattr_cachep __ro_after_init;
>  
> +static struct path pidfs_root_path = {};
> +
> +void pidfs_get_root(struct path *path)
> +{
> +	*path = pidfs_root_path;
> +	path_get(path);
> +}
> +
>  /*
>   * Stashes information that userspace needs to access even after the
>   * process has been reaped.
> @@ -1065,4 +1073,7 @@ void __init pidfs_init(void)
>  	pidfs_mnt = kern_mount(&pidfs_type);
>  	if (IS_ERR(pidfs_mnt))
>  		panic("Failed to mount pidfs pseudo filesystem");
> +
> +	pidfs_root_path.mnt = pidfs_mnt;
> +	pidfs_root_path.dentry = pidfs_mnt->mnt_root;
>  }
> 
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 5/9] fhandle: reflow get_path_anchor()

[Christian Brauner]

Switch to a more common coding style.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index d8d32208c621..22edced83e4c 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -170,18 +170,22 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 
 static int get_path_anchor(int fd, struct path *root)
 {
+	if (fd >= 0) {
+		CLASS(fd, f)(fd);
+		if (fd_empty(f))
+			return -EBADF;
+		*root = fd_file(f)->f_path;
+		path_get(root);
+		return 0;
+	}
+
 	if (fd == AT_FDCWD) {
 		struct fs_struct *fs = current->fs;
 		spin_lock(&fs->lock);
 		*root = fs->pwd;
 		path_get(root);
 		spin_unlock(&fs->lock);
-	} else {
-		CLASS(fd, f)(fd);
-		if (fd_empty(f))
-			return -EBADF;
-		*root = fd_file(f)->f_path;
-		path_get(root);
+		return 0;
 	}
 
 	return 0;

-- 
2.47.2

Re: [PATCH 5/9] fhandle: reflow get_path_anchor()

[Jan Kara]

On Mon 23-06-25 11:01:27, Christian Brauner wrote:
> Switch to a more common coding style.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Sure. Feel free to add:

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

								Honza

> ---
>  fs/fhandle.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index d8d32208c621..22edced83e4c 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -170,18 +170,22 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  
>  static int get_path_anchor(int fd, struct path *root)
>  {
> +	if (fd >= 0) {
> +		CLASS(fd, f)(fd);
> +		if (fd_empty(f))
> +			return -EBADF;
> +		*root = fd_file(f)->f_path;
> +		path_get(root);
> +		return 0;
> +	}
> +
>  	if (fd == AT_FDCWD) {
>  		struct fs_struct *fs = current->fs;
>  		spin_lock(&fs->lock);
>  		*root = fs->pwd;
>  		path_get(root);
>  		spin_unlock(&fs->lock);
> -	} else {
> -		CLASS(fd, f)(fd);
> -		if (fd_empty(f))
> -			return -EBADF;
> -		*root = fd_file(f)->f_path;
> -		path_get(root);
> +		return 0;
>  	}
>  
>  	return 0;
> 
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 6/9] exportfs: add FILEID_PIDFS

[Christian Brauner]

Introduce new pidfs file handle values.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/exportfs.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 25c4a5afbd44..45b38a29643f 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -99,6 +99,11 @@ enum fid_type {
 	 */
 	FILEID_FAT_WITH_PARENT = 0x72,
 
+	/*
+	 * 64 bit inode number.
+	 */
+	FILEID_INO64 = 0x80,
+
 	/*
 	 * 64 bit inode number, 32 bit generation number.
 	 */
@@ -131,6 +136,12 @@ enum fid_type {
 	 * Filesystems must not use 0xff file ID.
 	 */
 	FILEID_INVALID = 0xff,
+
+	/* Internal kernel fid types */
+
+	/* pidfs fid types */
+	FILEID_PIDFS_FSTYPE = 0x100,
+	FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
 };
 
 struct fid {

-- 
2.47.2

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Jan Kara]

On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> Introduce new pidfs file handle values.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
>  include/linux/exportfs.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 25c4a5afbd44..45b38a29643f 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -99,6 +99,11 @@ enum fid_type {
>  	 */
>  	FILEID_FAT_WITH_PARENT = 0x72,
>  
> +	/*
> +	 * 64 bit inode number.
> +	 */
> +	FILEID_INO64 = 0x80,
> +
>  	/*
>  	 * 64 bit inode number, 32 bit generation number.
>  	 */
> @@ -131,6 +136,12 @@ enum fid_type {
>  	 * Filesystems must not use 0xff file ID.
>  	 */
>  	FILEID_INVALID = 0xff,
> +
> +	/* Internal kernel fid types */
> +
> +	/* pidfs fid types */
> +	FILEID_PIDFS_FSTYPE = 0x100,
> +	FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,

What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
Why not just allocate one value for FILEID_PIDFS and be done with it? Do
you expect some future extensions for pidfs?

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

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Christian Brauner]

On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > Introduce new pidfs file handle values.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > ---
> >  include/linux/exportfs.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index 25c4a5afbd44..45b38a29643f 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -99,6 +99,11 @@ enum fid_type {
> >  	 */
> >  	FILEID_FAT_WITH_PARENT = 0x72,
> >  
> > +	/*
> > +	 * 64 bit inode number.
> > +	 */
> > +	FILEID_INO64 = 0x80,
> > +
> >  	/*
> >  	 * 64 bit inode number, 32 bit generation number.
> >  	 */
> > @@ -131,6 +136,12 @@ enum fid_type {
> >  	 * Filesystems must not use 0xff file ID.
> >  	 */
> >  	FILEID_INVALID = 0xff,
> > +
> > +	/* Internal kernel fid types */
> > +
> > +	/* pidfs fid types */
> > +	FILEID_PIDFS_FSTYPE = 0x100,
> > +	FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> 
> What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> you expect some future extensions for pidfs?

I wouldn't rule it out, yes. This was also one of Amir's suggestions.

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Amir Goldstein]

On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > Introduce new pidfs file handle values.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > ---
> > >  include/linux/exportfs.h | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > index 25c4a5afbd44..45b38a29643f 100644
> > > --- a/include/linux/exportfs.h
> > > +++ b/include/linux/exportfs.h
> > > @@ -99,6 +99,11 @@ enum fid_type {
> > >      */
> > >     FILEID_FAT_WITH_PARENT = 0x72,
> > >
> > > +   /*
> > > +    * 64 bit inode number.
> > > +    */
> > > +   FILEID_INO64 = 0x80,
> > > +
> > >     /*
> > >      * 64 bit inode number, 32 bit generation number.
> > >      */
> > > @@ -131,6 +136,12 @@ enum fid_type {
> > >      * Filesystems must not use 0xff file ID.
> > >      */
> > >     FILEID_INVALID = 0xff,
> > > +
> > > +   /* Internal kernel fid types */
> > > +
> > > +   /* pidfs fid types */
> > > +   FILEID_PIDFS_FSTYPE = 0x100,
> > > +   FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> >
> > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > you expect some future extensions for pidfs?
>
> I wouldn't rule it out, yes. This was also one of Amir's suggestions.

The idea was to parcel the autonomous fid type to fstype (pidfs)
which determines which is the fs to decode the autonomous fid
and a per-fs sub-type like we have today.

Maybe it is a bit over design, but I don't think this is really limiting us
going forward, because those constants are not part of the uapi.

Thanks,
Amir.

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Jan Kara]

On Mon 23-06-25 14:22:26, Amir Goldstein wrote:
> On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > > Introduce new pidfs file handle values.
> > > >
> > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > ---
> > > >  include/linux/exportfs.h | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > > index 25c4a5afbd44..45b38a29643f 100644
> > > > --- a/include/linux/exportfs.h
> > > > +++ b/include/linux/exportfs.h
> > > > @@ -99,6 +99,11 @@ enum fid_type {
> > > >      */
> > > >     FILEID_FAT_WITH_PARENT = 0x72,
> > > >
> > > > +   /*
> > > > +    * 64 bit inode number.
> > > > +    */
> > > > +   FILEID_INO64 = 0x80,
> > > > +
> > > >     /*
> > > >      * 64 bit inode number, 32 bit generation number.
> > > >      */
> > > > @@ -131,6 +136,12 @@ enum fid_type {
> > > >      * Filesystems must not use 0xff file ID.
> > > >      */
> > > >     FILEID_INVALID = 0xff,
> > > > +
> > > > +   /* Internal kernel fid types */
> > > > +
> > > > +   /* pidfs fid types */
> > > > +   FILEID_PIDFS_FSTYPE = 0x100,
> > > > +   FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> > >
> > > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > > you expect some future extensions for pidfs?
> >
> > I wouldn't rule it out, yes. This was also one of Amir's suggestions.
> 
> The idea was to parcel the autonomous fid type to fstype (pidfs)
> which determines which is the fs to decode the autonomous fid
> and a per-fs sub-type like we have today.
> 
> Maybe it is a bit over design, but I don't think this is really limiting us
> going forward, because those constants are not part of the uapi.

OK, I agree these file handles do not survive reboot anyway so we are free
to redefine the encoding in the future. So it is not a big deal (but it
also wouldn't be a big deal to start simple and add some subtyping in the
future when there's actual usecase). But in the current patch set we have
one flag FILEID_IS_AUTONOMOUS which does provide this subtyping and then
this FILEID_PIDFS_FSTYPE which doesn't seem to be about subtyping but about
pidfs expecting some future extensions and wanting to recognize all its
file handle types more easily (without having to enumerate all types like
other filesystems)? My concern is that fh_type space isn't that big and if
every filesystem started to reserve flag-like bits in it, we'd soon run out
of it. So I don't think this is a great precedens although in this
particular case I agree it can be modified in the future if we decide so...

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

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Amir Goldstein]

On Mon, Jun 23, 2025 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 23-06-25 14:22:26, Amir Goldstein wrote:
> > On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > > > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > > > Introduce new pidfs file handle values.
> > > > >
> > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > ---
> > > > >  include/linux/exportfs.h | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > > > index 25c4a5afbd44..45b38a29643f 100644
> > > > > --- a/include/linux/exportfs.h
> > > > > +++ b/include/linux/exportfs.h
> > > > > @@ -99,6 +99,11 @@ enum fid_type {
> > > > >      */
> > > > >     FILEID_FAT_WITH_PARENT = 0x72,
> > > > >
> > > > > +   /*
> > > > > +    * 64 bit inode number.
> > > > > +    */
> > > > > +   FILEID_INO64 = 0x80,
> > > > > +
> > > > >     /*
> > > > >      * 64 bit inode number, 32 bit generation number.
> > > > >      */
> > > > > @@ -131,6 +136,12 @@ enum fid_type {
> > > > >      * Filesystems must not use 0xff file ID.
> > > > >      */
> > > > >     FILEID_INVALID = 0xff,
> > > > > +
> > > > > +   /* Internal kernel fid types */
> > > > > +
> > > > > +   /* pidfs fid types */
> > > > > +   FILEID_PIDFS_FSTYPE = 0x100,
> > > > > +   FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> > > >
> > > > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > > > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > > > you expect some future extensions for pidfs?
> > >
> > > I wouldn't rule it out, yes. This was also one of Amir's suggestions.
> >
> > The idea was to parcel the autonomous fid type to fstype (pidfs)
> > which determines which is the fs to decode the autonomous fid
> > and a per-fs sub-type like we have today.
> >
> > Maybe it is a bit over design, but I don't think this is really limiting us
> > going forward, because those constants are not part of the uapi.
>
> OK, I agree these file handles do not survive reboot anyway so we are free
> to redefine the encoding in the future. So it is not a big deal (but it
> also wouldn't be a big deal to start simple and add some subtyping in the
> future when there's actual usecase). But in the current patch set we have
> one flag FILEID_IS_AUTONOMOUS which does provide this subtyping and then
> this FILEID_PIDFS_FSTYPE which doesn't seem to be about subtyping but about
> pidfs expecting some future extensions and wanting to recognize all its
> file handle types more easily (without having to enumerate all types like
> other filesystems)? My concern is that fh_type space isn't that big and if
> every filesystem started to reserve flag-like bits in it, we'd soon run out
> of it. So I don't think this is a great precedens although in this
> particular case I agree it can be modified in the future if we decide so...
>

Yes, I agree.
For the sake of argument let's assume we have two types to begin with
pidfs and drm and then would you want to define them as:

   /* Internal kernel fid types */
   FILEID_PIDFS = 0x100,
   FILEID_DRM = 0x200,

Or

   FILEID_PIDFS = 0x100,
   FILEID_DRM = 0x101,

I think the former is easy to start with and we have plenty of time to
make reparceling if we get to dousens and file id type...

Regarding the lower bits, I think it would be wise to reserve

FILEID_PIDFS_FSTYPE = 0x100,
FILEID_PIDFS_ROOT = FILEID_PIDFS_FSTYPE | FILEID_ROOT /* also 0x100 */

This is why I suggested using non zero lower bits and then why
not use the actual format descriptor for the lower bits as it was intended.

Thanks,
Amir.

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Jan Kara]

On Mon 23-06-25 15:05:45, Amir Goldstein wrote:
> On Mon, Jun 23, 2025 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 23-06-25 14:22:26, Amir Goldstein wrote:
> > > On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > > > > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > > > > Introduce new pidfs file handle values.
> > > > > >
> > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > > ---
> > > > > >  include/linux/exportfs.h | 11 +++++++++++
> > > > > >  1 file changed, 11 insertions(+)
> > > > > >
> > > > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > > > > index 25c4a5afbd44..45b38a29643f 100644
> > > > > > --- a/include/linux/exportfs.h
> > > > > > +++ b/include/linux/exportfs.h
> > > > > > @@ -99,6 +99,11 @@ enum fid_type {
> > > > > >      */
> > > > > >     FILEID_FAT_WITH_PARENT = 0x72,
> > > > > >
> > > > > > +   /*
> > > > > > +    * 64 bit inode number.
> > > > > > +    */
> > > > > > +   FILEID_INO64 = 0x80,
> > > > > > +
> > > > > >     /*
> > > > > >      * 64 bit inode number, 32 bit generation number.
> > > > > >      */
> > > > > > @@ -131,6 +136,12 @@ enum fid_type {
> > > > > >      * Filesystems must not use 0xff file ID.
> > > > > >      */
> > > > > >     FILEID_INVALID = 0xff,
> > > > > > +
> > > > > > +   /* Internal kernel fid types */
> > > > > > +
> > > > > > +   /* pidfs fid types */
> > > > > > +   FILEID_PIDFS_FSTYPE = 0x100,
> > > > > > +   FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> > > > >
> > > > > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > > > > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > > > > you expect some future extensions for pidfs?
> > > >
> > > > I wouldn't rule it out, yes. This was also one of Amir's suggestions.
> > >
> > > The idea was to parcel the autonomous fid type to fstype (pidfs)
> > > which determines which is the fs to decode the autonomous fid
> > > and a per-fs sub-type like we have today.
> > >
> > > Maybe it is a bit over design, but I don't think this is really limiting us
> > > going forward, because those constants are not part of the uapi.
> >
> > OK, I agree these file handles do not survive reboot anyway so we are free
> > to redefine the encoding in the future. So it is not a big deal (but it
> > also wouldn't be a big deal to start simple and add some subtyping in the
> > future when there's actual usecase). But in the current patch set we have
> > one flag FILEID_IS_AUTONOMOUS which does provide this subtyping and then
> > this FILEID_PIDFS_FSTYPE which doesn't seem to be about subtyping but about
> > pidfs expecting some future extensions and wanting to recognize all its
> > file handle types more easily (without having to enumerate all types like
> > other filesystems)? My concern is that fh_type space isn't that big and if
> > every filesystem started to reserve flag-like bits in it, we'd soon run out
> > of it. So I don't think this is a great precedens although in this
> > particular case I agree it can be modified in the future if we decide so...
> >
> 
> Yes, I agree.
> For the sake of argument let's assume we have two types to begin with
> pidfs and drm and then would you want to define them as:
> 
>    /* Internal kernel fid types */
>    FILEID_PIDFS = 0x100,
>    FILEID_DRM = 0x200,
> 
> Or
> 
>    FILEID_PIDFS = 0x100,
>    FILEID_DRM = 0x101,
> 
> I think the former is easy to start with and we have plenty of time to
> make reparceling if we get to dousens and file id type...

No strong preference if we then test for equality with FILEID_PIDFS and
FILEID_DRM and not like fh_type & FILEID_PIDFS.

> Regarding the lower bits, I think it would be wise to reserve
> 
> FILEID_PIDFS_FSTYPE = 0x100,
> FILEID_PIDFS_ROOT = FILEID_PIDFS_FSTYPE | FILEID_ROOT /* also 0x100 */
> 
> This is why I suggested using non zero lower bits and then why
> not use the actual format descriptor for the lower bits as it was intended.

I'm getting lost in these names a bit :) It's hard to see a difference for
me without a concrete examples of where one should be used compared to the
other...

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

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Amir Goldstein]

On Mon, Jun 23, 2025 at 3:18 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 23-06-25 15:05:45, Amir Goldstein wrote:
> > On Mon, Jun 23, 2025 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Mon 23-06-25 14:22:26, Amir Goldstein wrote:
> > > > On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@kernel.org> wrote:
> > > > >
> > > > > On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > > > > > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > > > > > Introduce new pidfs file handle values.
> > > > > > >
> > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > > > ---
> > > > > > >  include/linux/exportfs.h | 11 +++++++++++
> > > > > > >  1 file changed, 11 insertions(+)
> > > > > > >
> > > > > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > > > > > index 25c4a5afbd44..45b38a29643f 100644
> > > > > > > --- a/include/linux/exportfs.h
> > > > > > > +++ b/include/linux/exportfs.h
> > > > > > > @@ -99,6 +99,11 @@ enum fid_type {
> > > > > > >      */
> > > > > > >     FILEID_FAT_WITH_PARENT = 0x72,
> > > > > > >
> > > > > > > +   /*
> > > > > > > +    * 64 bit inode number.
> > > > > > > +    */
> > > > > > > +   FILEID_INO64 = 0x80,
> > > > > > > +
> > > > > > >     /*
> > > > > > >      * 64 bit inode number, 32 bit generation number.
> > > > > > >      */
> > > > > > > @@ -131,6 +136,12 @@ enum fid_type {
> > > > > > >      * Filesystems must not use 0xff file ID.
> > > > > > >      */
> > > > > > >     FILEID_INVALID = 0xff,
> > > > > > > +
> > > > > > > +   /* Internal kernel fid types */
> > > > > > > +
> > > > > > > +   /* pidfs fid types */
> > > > > > > +   FILEID_PIDFS_FSTYPE = 0x100,
> > > > > > > +   FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> > > > > >
> > > > > > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > > > > > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > > > > > you expect some future extensions for pidfs?
> > > > >
> > > > > I wouldn't rule it out, yes. This was also one of Amir's suggestions.
> > > >
> > > > The idea was to parcel the autonomous fid type to fstype (pidfs)
> > > > which determines which is the fs to decode the autonomous fid
> > > > and a per-fs sub-type like we have today.
> > > >
> > > > Maybe it is a bit over design, but I don't think this is really limiting us
> > > > going forward, because those constants are not part of the uapi.
> > >
> > > OK, I agree these file handles do not survive reboot anyway so we are free
> > > to redefine the encoding in the future. So it is not a big deal (but it
> > > also wouldn't be a big deal to start simple and add some subtyping in the
> > > future when there's actual usecase). But in the current patch set we have
> > > one flag FILEID_IS_AUTONOMOUS which does provide this subtyping and then
> > > this FILEID_PIDFS_FSTYPE which doesn't seem to be about subtyping but about
> > > pidfs expecting some future extensions and wanting to recognize all its
> > > file handle types more easily (without having to enumerate all types like
> > > other filesystems)? My concern is that fh_type space isn't that big and if
> > > every filesystem started to reserve flag-like bits in it, we'd soon run out
> > > of it. So I don't think this is a great precedens although in this
> > > particular case I agree it can be modified in the future if we decide so...
> > >
> >
> > Yes, I agree.
> > For the sake of argument let's assume we have two types to begin with
> > pidfs and drm and then would you want to define them as:
> >
> >    /* Internal kernel fid types */
> >    FILEID_PIDFS = 0x100,
> >    FILEID_DRM = 0x200,
> >
> > Or
> >
> >    FILEID_PIDFS = 0x100,
> >    FILEID_DRM = 0x101,
> >
> > I think the former is easy to start with and we have plenty of time to
> > make reparceling if we get to dousens and file id type...
>
> No strong preference if we then test for equality with FILEID_PIDFS and
> FILEID_DRM and not like fh_type & FILEID_PIDFS.
>
> > Regarding the lower bits, I think it would be wise to reserve
> >
> > FILEID_PIDFS_FSTYPE = 0x100,
> > FILEID_PIDFS_ROOT = FILEID_PIDFS_FSTYPE | FILEID_ROOT /* also 0x100 */
> >
> > This is why I suggested using non zero lower bits and then why
> > not use the actual format descriptor for the lower bits as it was intended.
>
> I'm getting lost in these names a bit :) It's hard to see a difference for
> me without a concrete examples of where one should be used compared to the
> other...

In any case, I don't feel strongly about it.
You can leave it as is or use
    FILEID_PIDFS = 0x100,
or
    FILEID_PIDFS = 0x180,

we can always change it later if we want to.

Thanks,
Amir.

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Christian Brauner]

On Mon, Jun 23, 2025 at 04:05:18PM +0200, Amir Goldstein wrote:
> On Mon, Jun 23, 2025 at 3:18 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Mon 23-06-25 15:05:45, Amir Goldstein wrote:
> > > On Mon, Jun 23, 2025 at 2:41 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Mon 23-06-25 14:22:26, Amir Goldstein wrote:
> > > > > On Mon, Jun 23, 2025 at 1:58 PM Christian Brauner <brauner@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Jun 23, 2025 at 01:55:38PM +0200, Jan Kara wrote:
> > > > > > > On Mon 23-06-25 11:01:28, Christian Brauner wrote:
> > > > > > > > Introduce new pidfs file handle values.
> > > > > > > >
> > > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > > > > > ---
> > > > > > > >  include/linux/exportfs.h | 11 +++++++++++
> > > > > > > >  1 file changed, 11 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > > > > > > > index 25c4a5afbd44..45b38a29643f 100644
> > > > > > > > --- a/include/linux/exportfs.h
> > > > > > > > +++ b/include/linux/exportfs.h
> > > > > > > > @@ -99,6 +99,11 @@ enum fid_type {
> > > > > > > >      */
> > > > > > > >     FILEID_FAT_WITH_PARENT = 0x72,
> > > > > > > >
> > > > > > > > +   /*
> > > > > > > > +    * 64 bit inode number.
> > > > > > > > +    */
> > > > > > > > +   FILEID_INO64 = 0x80,
> > > > > > > > +
> > > > > > > >     /*
> > > > > > > >      * 64 bit inode number, 32 bit generation number.
> > > > > > > >      */
> > > > > > > > @@ -131,6 +136,12 @@ enum fid_type {
> > > > > > > >      * Filesystems must not use 0xff file ID.
> > > > > > > >      */
> > > > > > > >     FILEID_INVALID = 0xff,
> > > > > > > > +
> > > > > > > > +   /* Internal kernel fid types */
> > > > > > > > +
> > > > > > > > +   /* pidfs fid types */
> > > > > > > > +   FILEID_PIDFS_FSTYPE = 0x100,
> > > > > > > > +   FILEID_PIDFS = FILEID_PIDFS_FSTYPE | FILEID_INO64,
> > > > > > >
> > > > > > > What is the point behind having FILEID_INO64 and FILEID_PIDFS separately?
> > > > > > > Why not just allocate one value for FILEID_PIDFS and be done with it? Do
> > > > > > > you expect some future extensions for pidfs?
> > > > > >
> > > > > > I wouldn't rule it out, yes. This was also one of Amir's suggestions.
> > > > >
> > > > > The idea was to parcel the autonomous fid type to fstype (pidfs)
> > > > > which determines which is the fs to decode the autonomous fid
> > > > > and a per-fs sub-type like we have today.
> > > > >
> > > > > Maybe it is a bit over design, but I don't think this is really limiting us
> > > > > going forward, because those constants are not part of the uapi.
> > > >
> > > > OK, I agree these file handles do not survive reboot anyway so we are free
> > > > to redefine the encoding in the future. So it is not a big deal (but it
> > > > also wouldn't be a big deal to start simple and add some subtyping in the
> > > > future when there's actual usecase). But in the current patch set we have
> > > > one flag FILEID_IS_AUTONOMOUS which does provide this subtyping and then
> > > > this FILEID_PIDFS_FSTYPE which doesn't seem to be about subtyping but about
> > > > pidfs expecting some future extensions and wanting to recognize all its
> > > > file handle types more easily (without having to enumerate all types like
> > > > other filesystems)? My concern is that fh_type space isn't that big and if
> > > > every filesystem started to reserve flag-like bits in it, we'd soon run out
> > > > of it. So I don't think this is a great precedens although in this
> > > > particular case I agree it can be modified in the future if we decide so...
> > > >
> > >
> > > Yes, I agree.
> > > For the sake of argument let's assume we have two types to begin with
> > > pidfs and drm and then would you want to define them as:
> > >
> > >    /* Internal kernel fid types */
> > >    FILEID_PIDFS = 0x100,
> > >    FILEID_DRM = 0x200,
> > >
> > > Or
> > >
> > >    FILEID_PIDFS = 0x100,
> > >    FILEID_DRM = 0x101,
> > >
> > > I think the former is easy to start with and we have plenty of time to
> > > make reparceling if we get to dousens and file id type...
> >
> > No strong preference if we then test for equality with FILEID_PIDFS and
> > FILEID_DRM and not like fh_type & FILEID_PIDFS.
> >
> > > Regarding the lower bits, I think it would be wise to reserve
> > >
> > > FILEID_PIDFS_FSTYPE = 0x100,
> > > FILEID_PIDFS_ROOT = FILEID_PIDFS_FSTYPE | FILEID_ROOT /* also 0x100 */
> > >
> > > This is why I suggested using non zero lower bits and then why
> > > not use the actual format descriptor for the lower bits as it was intended.
> >
> > I'm getting lost in these names a bit :) It's hard to see a difference for
> > me without a concrete examples of where one should be used compared to the
> > other...
> 
> In any case, I don't feel strongly about it.
> You can leave it as is or use
>     FILEID_PIDFS = 0x100,
> or
>     FILEID_PIDFS = 0x180,
> 
> we can always change it later if we want to.
> 
> Thanks,
> Amir.

I'm completely lost.

Re: [PATCH 6/9] exportfs: add FILEID_PIDFS

[Christian Brauner]

> > You can leave it as is or use
> >     FILEID_PIDFS = 0x100,

I used that.

[PATCH 7/9] fhandle: add EXPORT_OP_AUTONOMOUS_HANDLES marker

[Christian Brauner]

Allow a filesystem to indicate that it supports encoding autonomous file
handles that can be decoded without having to pass a filesystem for the
filesystem. In other words, the file handle uniquely identifies the
filesystem.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c             | 7 ++++++-
 include/linux/exportfs.h | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 22edced83e4c..ab4891925b52 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -23,12 +23,13 @@ static long do_sys_name_to_handle(const struct path *path,
 	struct file_handle f_handle;
 	int handle_dwords, handle_bytes;
 	struct file_handle *handle = NULL;
+	const struct export_operations *eops = path->dentry->d_sb->s_export_op;
 
 	/*
 	 * We need to make sure whether the file system support decoding of
 	 * the file handle if decodeable file handle was requested.
 	 */
-	if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
+	if (!exportfs_can_encode_fh(eops, fh_flags))
 		return -EOPNOTSUPP;
 
 	/*
@@ -90,6 +91,10 @@ static long do_sys_name_to_handle(const struct path *path,
 			if (d_is_dir(path->dentry))
 				handle->handle_type |= FILEID_IS_DIR;
 		}
+
+		/* Filesystems supports autonomous file handles. */
+		if (eops->flags & EXPORT_OP_AUTONOMOUS_HANDLES)
+			handle->handle_type |= FILEID_IS_AUTONOMOUS;
 		retval = 0;
 	}
 	/* copy the mount id */
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 45b38a29643f..959a1f7d46d0 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -194,7 +194,8 @@ struct handle_to_path_ctx {
 /* 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	(FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
+#define FILEID_IS_AUTONOMOUS	0x40000
+#define FILEID_VALID_USER_FLAGS	(FILEID_IS_CONNECTABLE | FILEID_IS_DIR | FILEID_IS_AUTONOMOUS)
 
 /**
  * struct export_operations - for nfsd to communicate with file systems
@@ -291,6 +292,7 @@ struct export_operations {
 						*/
 #define EXPORT_OP_FLUSH_ON_CLOSE	(0x20) /* fs flushes file data on close */
 #define EXPORT_OP_NOLOCKS		(0x40) /* no file locking support */
+#define EXPORT_OP_AUTONOMOUS_HANDLES	(0x80) /* filesystem supports autonomous file handles */
 	unsigned long	flags;
 };
 

-- 
2.47.2

Re: [PATCH 7/9] fhandle: add EXPORT_OP_AUTONOMOUS_HANDLES marker

[Jan Kara]

On Mon 23-06-25 11:01:29, Christian Brauner wrote:
> Allow a filesystem to indicate that it supports encoding autonomous file
> handles that can be decoded without having to pass a filesystem for the
> filesystem. In other words, the file handle uniquely identifies the
> filesystem.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

...

> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 45b38a29643f..959a1f7d46d0 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -194,7 +194,8 @@ struct handle_to_path_ctx {
>  /* 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	(FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
> +#define FILEID_IS_AUTONOMOUS	0x40000
> +#define FILEID_VALID_USER_FLAGS	(FILEID_IS_CONNECTABLE | FILEID_IS_DIR | FILEID_IS_AUTONOMOUS)

Is there a reason for FILEID_IS_AUTONOMOUS? As far as I understand the
fh_type has to encode filesystem type anyway so that you know which root to
pick. So FILEID_IS_AUTONOMOUS is just duplicating the information? But
maybe there's some benefit in having FILEID_IS_AUTONOMOUS which I'm
missing...

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

Re: [PATCH 7/9] fhandle: add EXPORT_OP_AUTONOMOUS_HANDLES marker

[Amir Goldstein]

On Mon, Jun 23, 2025 at 1:59 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 23-06-25 11:01:29, Christian Brauner wrote:
> > Allow a filesystem to indicate that it supports encoding autonomous file
> > handles that can be decoded without having to pass a filesystem for the
> > filesystem. In other words, the file handle uniquely identifies the
> > filesystem.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
>
> ...
>
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index 45b38a29643f..959a1f7d46d0 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -194,7 +194,8 @@ struct handle_to_path_ctx {
> >  /* 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      (FILEID_IS_CONNECTABLE | FILEID_IS_DIR)
> > +#define FILEID_IS_AUTONOMOUS 0x40000
> > +#define FILEID_VALID_USER_FLAGS      (FILEID_IS_CONNECTABLE | FILEID_IS_DIR | FILEID_IS_AUTONOMOUS)
>
> Is there a reason for FILEID_IS_AUTONOMOUS? As far as I understand the
> fh_type has to encode filesystem type anyway so that you know which root to
> pick. So FILEID_IS_AUTONOMOUS is just duplicating the information? But
> maybe there's some benefit in having FILEID_IS_AUTONOMOUS which I'm
> missing...

The use of the high 16bits as a way for vfs to describe properties on
the fhandle
relies on the fact that filesystems, out of tree as well, are not
allowed to return a type
with high 16 bits set and we also enforce that.
FWIW, the type is documented in exporting.rst FWIW as single byte:
"A filehandle fragment consists of an array of 1 or more 4byte words,
together with a one byte "type"."

It is important to remember that filesystems file handles and their types
are opaque and that filesystems in and of tree do not need to use constants
defined in exportfs.h nor to avoid with collisions with the namespace of
file type constants.

This way, to let vfs raise the autonomous flag based on export_op flags
seems more backward compat and leaves less room for mistakes IMO.

This way, to let vfs raise the autonomous flag based on export_op flags
seems more backward compat and leaves less room for mistakes IMO.

Thanks,
Amir.

[PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Christian Brauner]

Various filesystems such as pidfs (and likely drm in the future) have a
use-case to support opening files purely based on the handle without
having to require a file descriptor to another object. That's especially
the case for filesystems that don't do any lookup whatsoever and there's
zero relationship between the objects. Such filesystems are also
singletons that stay around for the lifetime of the system meaning that
they can be uniquely identified and accessed purely based on the file
handle type. Enable that so that userspace doesn't have to allocate an
object needlessly especially if they can't do that for whatever reason.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/fhandle.c | 19 +++++++++++++++++--
 fs/pidfs.c   |  5 ++++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/fs/fhandle.c b/fs/fhandle.c
index ab4891925b52..20d6477b5a9e 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -173,7 +173,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	return err;
 }
 
-static int get_path_anchor(int fd, struct path *root)
+static int get_path_anchor(int fd, struct path *root, int handle_type)
 {
 	if (fd >= 0) {
 		CLASS(fd, f)(fd);
@@ -193,6 +193,21 @@ static int get_path_anchor(int fd, struct path *root)
 		return 0;
 	}
 
+	/*
+	 * Only autonomous handles can be decoded without a file
+	 * descriptor.
+	 */
+	if (!(handle_type & FILEID_IS_AUTONOMOUS))
+		return -EOPNOTSUPP;
+
+	switch (handle_type & ~FILEID_USER_FLAGS_MASK) {
+	case FILEID_PIDFS:
+		pidfs_get_root(root);
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -347,7 +362,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
 		return -EINVAL;
 
-	retval = get_path_anchor(mountdirfd, &ctx.root);
+	retval = get_path_anchor(mountdirfd, &ctx.root, f_handle.handle_type);
 	if (retval)
 		return retval;
 
diff --git a/fs/pidfs.c b/fs/pidfs.c
index 1b7bd14366dc..ea50a6afc325 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -747,7 +747,7 @@ static int pidfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
 
 	*max_len = 2;
 	*(u64 *)fh = pid->ino;
-	return FILEID_KERNFS;
+	return FILEID_PIDFS;
 }
 
 static int pidfs_ino_find(const void *key, const struct rb_node *node)
@@ -802,6 +802,8 @@ static struct dentry *pidfs_fh_to_dentry(struct super_block *sb,
 		return NULL;
 
 	switch (fh_type) {
+	case FILEID_PIDFS:
+		fallthrough;
 	case FILEID_KERNFS:
 		pid_ino = *(u64 *)fid;
 		break;
@@ -860,6 +862,7 @@ static const struct export_operations pidfs_export_operations = {
 	.fh_to_dentry	= pidfs_fh_to_dentry,
 	.open		= pidfs_export_open,
 	.permission	= pidfs_export_permission,
+	.flags		= EXPORT_OP_AUTONOMOUS_HANDLES,
 };
 
 static int pidfs_init_inode(struct inode *inode, void *data)

-- 
2.47.2

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Jan Kara]

On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> Various filesystems such as pidfs (and likely drm in the future) have a
> use-case to support opening files purely based on the handle without
> having to require a file descriptor to another object. That's especially
> the case for filesystems that don't do any lookup whatsoever and there's
> zero relationship between the objects. Such filesystems are also
> singletons that stay around for the lifetime of the system meaning that
> they can be uniquely identified and accessed purely based on the file
> handle type. Enable that so that userspace doesn't have to allocate an
> object needlessly especially if they can't do that for whatever reason.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Hmm, maybe we should predefine some invalid fd value userspace should pass
when it wants to "autopick" fs root? Otherwise defining more special fd
values like AT_FDCWD would become difficult in the future. Or we could just
define that FILEID_PIDFS file handles *always* ignore the fd value and
auto-pick the root.

								Honza

> ---
>  fs/fhandle.c | 19 +++++++++++++++++--
>  fs/pidfs.c   |  5 ++++-
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index ab4891925b52..20d6477b5a9e 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -173,7 +173,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  	return err;
>  }
>  
> -static int get_path_anchor(int fd, struct path *root)
> +static int get_path_anchor(int fd, struct path *root, int handle_type)
>  {
>  	if (fd >= 0) {
>  		CLASS(fd, f)(fd);
> @@ -193,6 +193,21 @@ static int get_path_anchor(int fd, struct path *root)
>  		return 0;
>  	}
>  
> +	/*
> +	 * Only autonomous handles can be decoded without a file
> +	 * descriptor.
> +	 */
> +	if (!(handle_type & FILEID_IS_AUTONOMOUS))
> +		return -EOPNOTSUPP;
> +
> +	switch (handle_type & ~FILEID_USER_FLAGS_MASK) {
> +	case FILEID_PIDFS:
> +		pidfs_get_root(root);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -347,7 +362,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  	    FILEID_USER_FLAGS(f_handle.handle_type) & ~FILEID_VALID_USER_FLAGS)
>  		return -EINVAL;
>  
> -	retval = get_path_anchor(mountdirfd, &ctx.root);
> +	retval = get_path_anchor(mountdirfd, &ctx.root, f_handle.handle_type);
>  	if (retval)
>  		return retval;
>  
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index 1b7bd14366dc..ea50a6afc325 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -747,7 +747,7 @@ static int pidfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>  
>  	*max_len = 2;
>  	*(u64 *)fh = pid->ino;
> -	return FILEID_KERNFS;
> +	return FILEID_PIDFS;
>  }
>  
>  static int pidfs_ino_find(const void *key, const struct rb_node *node)
> @@ -802,6 +802,8 @@ static struct dentry *pidfs_fh_to_dentry(struct super_block *sb,
>  		return NULL;
>  
>  	switch (fh_type) {
> +	case FILEID_PIDFS:
> +		fallthrough;
>  	case FILEID_KERNFS:
>  		pid_ino = *(u64 *)fid;
>  		break;
> @@ -860,6 +862,7 @@ static const struct export_operations pidfs_export_operations = {
>  	.fh_to_dentry	= pidfs_fh_to_dentry,
>  	.open		= pidfs_export_open,
>  	.permission	= pidfs_export_permission,
> +	.flags		= EXPORT_OP_AUTONOMOUS_HANDLES,
>  };
>  
>  static int pidfs_init_inode(struct inode *inode, void *data)
> 
> -- 
> 2.47.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Christian Brauner]

On Mon, Jun 23, 2025 at 02:06:43PM +0200, Jan Kara wrote:
> On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> > Various filesystems such as pidfs (and likely drm in the future) have a
> > use-case to support opening files purely based on the handle without
> > having to require a file descriptor to another object. That's especially
> > the case for filesystems that don't do any lookup whatsoever and there's
> > zero relationship between the objects. Such filesystems are also
> > singletons that stay around for the lifetime of the system meaning that
> > they can be uniquely identified and accessed purely based on the file
> > handle type. Enable that so that userspace doesn't have to allocate an
> > object needlessly especially if they can't do that for whatever reason.
> > 
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> Hmm, maybe we should predefine some invalid fd value userspace should pass
> when it wants to "autopick" fs root? Otherwise defining more special fd
> values like AT_FDCWD would become difficult in the future. Or we could just

Fwiw, I already did that with:

#define PIDFD_SELF_THREAD		-10000 /* Current thread. */
#define PIDFD_SELF_THREAD_GROUP		-20000 /* Current thread group leader. */

I think the correct thing to do would have been to say anything below

#define AT_FDCWD		-100    /* Special value for dirfd used to

is reserved for the kernel. But we can probably easily do this and say
anything from -10000 to -40000 is reserved for the kernel.

I would then change:

#define PIDFD_SELF_THREAD		-10000 /* Current thread. */
#define PIDFD_SELF_THREAD_GROUP		-10001 /* Current thread group leader. */

since that's very very new and then move
PIDFD_SELF_THREAD/PIDFD_SELF_THREAD_GROUP to include/uapi/linux/fcntl.h

and add that comment about the reserved range in there.

The thing is that we'd need to enforce this on the system call level.

Thoughts?

> define that FILEID_PIDFS file handles *always* ignore the fd value and
> auto-pick the root.

I see the issue I don't think it's a big deal but I'm open to adding:

#define AT_EBADF -10009 /* -10000 - EBADF */

and document that as a stand-in for a handle that can't be resolved.

Thoughts?

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Amir Goldstein]

On Mon, Jun 23, 2025 at 2:25 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Mon, Jun 23, 2025 at 02:06:43PM +0200, Jan Kara wrote:
> > On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> > > Various filesystems such as pidfs (and likely drm in the future) have a
> > > use-case to support opening files purely based on the handle without
> > > having to require a file descriptor to another object. That's especially
> > > the case for filesystems that don't do any lookup whatsoever and there's
> > > zero relationship between the objects. Such filesystems are also
> > > singletons that stay around for the lifetime of the system meaning that
> > > they can be uniquely identified and accessed purely based on the file
> > > handle type. Enable that so that userspace doesn't have to allocate an
> > > object needlessly especially if they can't do that for whatever reason.
> > >
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> >
> > Hmm, maybe we should predefine some invalid fd value userspace should pass
> > when it wants to "autopick" fs root? Otherwise defining more special fd
> > values like AT_FDCWD would become difficult in the future. Or we could just
>
> Fwiw, I already did that with:
>
> #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> #define PIDFD_SELF_THREAD_GROUP         -20000 /* Current thread group leader. */
>
> I think the correct thing to do would have been to say anything below
>
> #define AT_FDCWD                -100    /* Special value for dirfd used to
>
> is reserved for the kernel. But we can probably easily do this and say
> anything from -10000 to -40000 is reserved for the kernel.
>
> I would then change:
>
> #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> #define PIDFD_SELF_THREAD_GROUP         -10001 /* Current thread group leader. */
>
> since that's very very new and then move
> PIDFD_SELF_THREAD/PIDFD_SELF_THREAD_GROUP to include/uapi/linux/fcntl.h
>
> and add that comment about the reserved range in there.
>
> The thing is that we'd need to enforce this on the system call level.
>
> Thoughts?
>
> > define that FILEID_PIDFS file handles *always* ignore the fd value and
> > auto-pick the root.
>
> I see the issue I don't think it's a big deal but I'm open to adding:
>
> #define AT_EBADF -10009 /* -10000 - EBADF */
>
> and document that as a stand-in for a handle that can't be resolved.
>
> Thoughts?

I think the AT prefix of AT_FDCWD may have been a mistake
because it is quite easy to confuse this value with the completely
unrelated namespace of AT_ flags.

This is a null dirfd value. Is it not?

FD_NULL, FD_NONE?

You could envision that an *at() syscalls could in theory accept
(FD_NONE , "/an/absolute/path/only", ...

or MOUNTFD_NONE if we want to define a constant specifically for
this open_by_handle_at() extension.

Thanks,
Amir.

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Christian Brauner]

On Mon, Jun 23, 2025 at 02:54:00PM +0200, Amir Goldstein wrote:
> On Mon, Jun 23, 2025 at 2:25 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Mon, Jun 23, 2025 at 02:06:43PM +0200, Jan Kara wrote:
> > > On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> > > > Various filesystems such as pidfs (and likely drm in the future) have a
> > > > use-case to support opening files purely based on the handle without
> > > > having to require a file descriptor to another object. That's especially
> > > > the case for filesystems that don't do any lookup whatsoever and there's
> > > > zero relationship between the objects. Such filesystems are also
> > > > singletons that stay around for the lifetime of the system meaning that
> > > > they can be uniquely identified and accessed purely based on the file
> > > > handle type. Enable that so that userspace doesn't have to allocate an
> > > > object needlessly especially if they can't do that for whatever reason.
> > > >
> > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > >
> > > Hmm, maybe we should predefine some invalid fd value userspace should pass
> > > when it wants to "autopick" fs root? Otherwise defining more special fd
> > > values like AT_FDCWD would become difficult in the future. Or we could just
> >
> > Fwiw, I already did that with:
> >
> > #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> > #define PIDFD_SELF_THREAD_GROUP         -20000 /* Current thread group leader. */
> >
> > I think the correct thing to do would have been to say anything below
> >
> > #define AT_FDCWD                -100    /* Special value for dirfd used to
> >
> > is reserved for the kernel. But we can probably easily do this and say
> > anything from -10000 to -40000 is reserved for the kernel.
> >
> > I would then change:
> >
> > #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> > #define PIDFD_SELF_THREAD_GROUP         -10001 /* Current thread group leader. */
> >
> > since that's very very new and then move
> > PIDFD_SELF_THREAD/PIDFD_SELF_THREAD_GROUP to include/uapi/linux/fcntl.h
> >
> > and add that comment about the reserved range in there.
> >
> > The thing is that we'd need to enforce this on the system call level.
> >
> > Thoughts?
> >
> > > define that FILEID_PIDFS file handles *always* ignore the fd value and
> > > auto-pick the root.
> >
> > I see the issue I don't think it's a big deal but I'm open to adding:
> >
> > #define AT_EBADF -10009 /* -10000 - EBADF */
> >
> > and document that as a stand-in for a handle that can't be resolved.
> >
> > Thoughts?
> 
> I think the AT prefix of AT_FDCWD may have been a mistake
> because it is quite easy to confuse this value with the completely
> unrelated namespace of AT_ flags.
> 
> This is a null dirfd value. Is it not?

Not necessarily dirfd. We do allow direct operations of file descriptor
of any type. For example, in the mount api where you can mount files
onto other files.

> 
> FD_NULL, FD_NONE?

FD_INVALID, I think.

> 
> You could envision that an *at() syscalls could in theory accept
> (FD_NONE , "/an/absolute/path/only", ...
> 
> or MOUNTFD_NONE if we want to define a constant specifically for
> this open_by_handle_at() extension.

I think this is useful beyond open_by_handle_at().

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Jan Kara]

On Mon 23-06-25 15:00:43, Christian Brauner wrote:
> On Mon, Jun 23, 2025 at 02:54:00PM +0200, Amir Goldstein wrote:
> > On Mon, Jun 23, 2025 at 2:25 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Mon, Jun 23, 2025 at 02:06:43PM +0200, Jan Kara wrote:
> > > > On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> > > > > Various filesystems such as pidfs (and likely drm in the future) have a
> > > > > use-case to support opening files purely based on the handle without
> > > > > having to require a file descriptor to another object. That's especially
> > > > > the case for filesystems that don't do any lookup whatsoever and there's
> > > > > zero relationship between the objects. Such filesystems are also
> > > > > singletons that stay around for the lifetime of the system meaning that
> > > > > they can be uniquely identified and accessed purely based on the file
> > > > > handle type. Enable that so that userspace doesn't have to allocate an
> > > > > object needlessly especially if they can't do that for whatever reason.
> > > > >
> > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > >
> > > > Hmm, maybe we should predefine some invalid fd value userspace should pass
> > > > when it wants to "autopick" fs root? Otherwise defining more special fd
> > > > values like AT_FDCWD would become difficult in the future. Or we could just
> > >
> > > Fwiw, I already did that with:
> > >
> > > #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> > > #define PIDFD_SELF_THREAD_GROUP         -20000 /* Current thread group leader. */
> > >
> > > I think the correct thing to do would have been to say anything below
> > >
> > > #define AT_FDCWD                -100    /* Special value for dirfd used to
> > >
> > > is reserved for the kernel. But we can probably easily do this and say
> > > anything from -10000 to -40000 is reserved for the kernel.
> > >
> > > I would then change:
> > >
> > > #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> > > #define PIDFD_SELF_THREAD_GROUP         -10001 /* Current thread group leader. */
> > >
> > > since that's very very new and then move
> > > PIDFD_SELF_THREAD/PIDFD_SELF_THREAD_GROUP to include/uapi/linux/fcntl.h
> > >
> > > and add that comment about the reserved range in there.
> > >
> > > The thing is that we'd need to enforce this on the system call level.
> > >
> > > Thoughts?
> > >
> > > > define that FILEID_PIDFS file handles *always* ignore the fd value and
> > > > auto-pick the root.
> > >
> > > I see the issue I don't think it's a big deal but I'm open to adding:
> > >
> > > #define AT_EBADF -10009 /* -10000 - EBADF */
> > >
> > > and document that as a stand-in for a handle that can't be resolved.
> > >
> > > Thoughts?
> > 
> > I think the AT prefix of AT_FDCWD may have been a mistake
> > because it is quite easy to confuse this value with the completely
> > unrelated namespace of AT_ flags.
> > 
> > This is a null dirfd value. Is it not?
> 
> Not necessarily dirfd. We do allow direct operations of file descriptor
> of any type. For example, in the mount api where you can mount files
> onto other files.
> 
> > 
> > FD_NULL, FD_NONE?
> 
> FD_INVALID, I think.
> 
> > 
> > You could envision that an *at() syscalls could in theory accept
> > (FD_NONE , "/an/absolute/path/only", ...
> > 
> > or MOUNTFD_NONE if we want to define a constant specifically for
> > this open_by_handle_at() extension.
> 
> I think this is useful beyond open_by_handle_at().

Yes, defining FD_INVALID seems like useful addition that may become useful
in other cases in the future as well.

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

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Amir Goldstein]

On Mon, Jun 23, 2025 at 3:21 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 23-06-25 15:00:43, Christian Brauner wrote:
> > On Mon, Jun 23, 2025 at 02:54:00PM +0200, Amir Goldstein wrote:
> > > On Mon, Jun 23, 2025 at 2:25 PM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > On Mon, Jun 23, 2025 at 02:06:43PM +0200, Jan Kara wrote:
> > > > > On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> > > > > > Various filesystems such as pidfs (and likely drm in the future) have a
> > > > > > use-case to support opening files purely based on the handle without
> > > > > > having to require a file descriptor to another object. That's especially
> > > > > > the case for filesystems that don't do any lookup whatsoever and there's
> > > > > > zero relationship between the objects. Such filesystems are also
> > > > > > singletons that stay around for the lifetime of the system meaning that
> > > > > > they can be uniquely identified and accessed purely based on the file
> > > > > > handle type. Enable that so that userspace doesn't have to allocate an
> > > > > > object needlessly especially if they can't do that for whatever reason.
> > > > > >
> > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > > > >
> > > > > Hmm, maybe we should predefine some invalid fd value userspace should pass
> > > > > when it wants to "autopick" fs root? Otherwise defining more special fd
> > > > > values like AT_FDCWD would become difficult in the future. Or we could just
> > > >
> > > > Fwiw, I already did that with:
> > > >
> > > > #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> > > > #define PIDFD_SELF_THREAD_GROUP         -20000 /* Current thread group leader. */
> > > >
> > > > I think the correct thing to do would have been to say anything below
> > > >
> > > > #define AT_FDCWD                -100    /* Special value for dirfd used to
> > > >
> > > > is reserved for the kernel. But we can probably easily do this and say
> > > > anything from -10000 to -40000 is reserved for the kernel.
> > > >
> > > > I would then change:
> > > >
> > > > #define PIDFD_SELF_THREAD               -10000 /* Current thread. */
> > > > #define PIDFD_SELF_THREAD_GROUP         -10001 /* Current thread group leader. */
> > > >
> > > > since that's very very new and then move
> > > > PIDFD_SELF_THREAD/PIDFD_SELF_THREAD_GROUP to include/uapi/linux/fcntl.h
> > > >
> > > > and add that comment about the reserved range in there.
> > > >
> > > > The thing is that we'd need to enforce this on the system call level.
> > > >
> > > > Thoughts?
> > > >
> > > > > define that FILEID_PIDFS file handles *always* ignore the fd value and
> > > > > auto-pick the root.
> > > >
> > > > I see the issue I don't think it's a big deal but I'm open to adding:
> > > >
> > > > #define AT_EBADF -10009 /* -10000 - EBADF */
> > > >
> > > > and document that as a stand-in for a handle that can't be resolved.
> > > >
> > > > Thoughts?
> > >
> > > I think the AT prefix of AT_FDCWD may have been a mistake
> > > because it is quite easy to confuse this value with the completely
> > > unrelated namespace of AT_ flags.
> > >
> > > This is a null dirfd value. Is it not?
> >
> > Not necessarily dirfd. We do allow direct operations of file descriptor
> > of any type. For example, in the mount api where you can mount files
> > onto other files.
> >
> > >
> > > FD_NULL, FD_NONE?
> >
> > FD_INVALID, I think.
> >
> > >
> > > You could envision that an *at() syscalls could in theory accept
> > > (FD_NONE , "/an/absolute/path/only", ...
> > >
> > > or MOUNTFD_NONE if we want to define a constant specifically for
> > > this open_by_handle_at() extension.
> >
> > I think this is useful beyond open_by_handle_at().
>
> Yes, defining FD_INVALID seems like useful addition that may become useful
> in other cases in the future as well.

Sounds good to me.

Thanks,
Amir.

Re: [PATCH 8/9] fhandle, pidfs: support open_by_handle_at() purely based on file handle

[Jan Kara]

On Mon 23-06-25 14:25:45, Christian Brauner wrote:
> On Mon, Jun 23, 2025 at 02:06:43PM +0200, Jan Kara wrote:
> > On Mon 23-06-25 11:01:30, Christian Brauner wrote:
> > > Various filesystems such as pidfs (and likely drm in the future) have a
> > > use-case to support opening files purely based on the handle without
> > > having to require a file descriptor to another object. That's especially
> > > the case for filesystems that don't do any lookup whatsoever and there's
> > > zero relationship between the objects. Such filesystems are also
> > > singletons that stay around for the lifetime of the system meaning that
> > > they can be uniquely identified and accessed purely based on the file
> > > handle type. Enable that so that userspace doesn't have to allocate an
> > > object needlessly especially if they can't do that for whatever reason.
> > > 
> > > Signed-off-by: Christian Brauner <brauner@kernel.org>
> > 
> > Hmm, maybe we should predefine some invalid fd value userspace should pass
> > when it wants to "autopick" fs root? Otherwise defining more special fd
> > values like AT_FDCWD would become difficult in the future. Or we could just
> 
> Fwiw, I already did that with:
> 
> #define PIDFD_SELF_THREAD		-10000 /* Current thread. */
> #define PIDFD_SELF_THREAD_GROUP		-20000 /* Current thread group leader. */

Right.

> I think the correct thing to do would have been to say anything below
> 
> #define AT_FDCWD		-100    /* Special value for dirfd used to
> 
> is reserved for the kernel. But we can probably easily do this and say
> anything from -10000 to -40000 is reserved for the kernel.
> 
> I would then change:
> 
> #define PIDFD_SELF_THREAD		-10000 /* Current thread. */
> #define PIDFD_SELF_THREAD_GROUP		-10001 /* Current thread group leader. */
> 
> since that's very very new and then move
> PIDFD_SELF_THREAD/PIDFD_SELF_THREAD_GROUP to include/uapi/linux/fcntl.h
> 
> and add that comment about the reserved range in there.

Given the experience with AT_ flags and various extension flags
combinations getting used in various syscalls and finally leading to odd
flag conflicts I agree this would be probably a good future proofing. I'll
leave it upto your judgement whether renumbering PIDFD_SELF_THREAD_GROUP is
safe to do or not. It is kind of optional in my opinion.

> The thing is that we'd need to enforce this on the system call level.

Not sure what do you mean by this...

> > define that FILEID_PIDFS file handles *always* ignore the fd value and
> > auto-pick the root.
> 
> I see the issue I don't think it's a big deal but I'm open to adding:
> 
> #define AT_EBADF -10009 /* -10000 - EBADF */
> 
> and document that as a stand-in for a handle that can't be resolved.

Yeah, or the FD_INVALID name you've suggested later. That sounded even
better to me.

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

[PATCH 9/9] selftests/pidfd: decode pidfd file handles withou having to specify an fd

[Christian Brauner]

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 tools/testing/selftests/pidfd/Makefile             |  2 +-
 .../selftests/pidfd/pidfd_file_handle_test.c       | 54 ++++++++++++++++++++++
 2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index 03a6eede9c9e..764a8f9ecefa 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
-CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
+CFLAGS += -g $(KHDR_INCLUDES) $(TOOLS_INCLUDES) -pthread -Wall
 
 TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \
 	pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test \
diff --git a/tools/testing/selftests/pidfd/pidfd_file_handle_test.c b/tools/testing/selftests/pidfd/pidfd_file_handle_test.c
index 439b9c6c0457..f7e29416152c 100644
--- a/tools/testing/selftests/pidfd/pidfd_file_handle_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_file_handle_test.c
@@ -500,4 +500,58 @@ TEST_F(file_handle, valid_name_to_handle_at_flags)
 	ASSERT_EQ(close(pidfd), 0);
 }
 
+/*
+ * That we decode a file handle without having to pass a pidfd.
+ */
+TEST_F(file_handle, decode_purely_based_on_file_handle)
+{
+	int mnt_id;
+	struct file_handle *fh;
+	int pidfd = -EBADF;
+	struct stat st1, st2;
+
+	fh = malloc(sizeof(struct file_handle) + MAX_HANDLE_SZ);
+	ASSERT_NE(fh, NULL);
+	memset(fh, 0, sizeof(struct file_handle) + MAX_HANDLE_SZ);
+	fh->handle_bytes = MAX_HANDLE_SZ;
+
+	ASSERT_EQ(name_to_handle_at(self->child_pidfd1, "", fh, &mnt_id, AT_EMPTY_PATH), 0);
+
+	ASSERT_EQ(fstat(self->child_pidfd1, &st1), 0);
+
+	pidfd = open_by_handle_at(-EBADF, fh, 0);
+	ASSERT_GE(pidfd, 0);
+
+	ASSERT_EQ(fstat(pidfd, &st2), 0);
+	ASSERT_TRUE(st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+
+	ASSERT_EQ(close(pidfd), 0);
+
+	pidfd = open_by_handle_at(-EBADF, fh, O_CLOEXEC);
+	ASSERT_GE(pidfd, 0);
+
+	ASSERT_EQ(fstat(pidfd, &st2), 0);
+	ASSERT_TRUE(st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+
+	ASSERT_EQ(close(pidfd), 0);
+
+	pidfd = open_by_handle_at(-EBADF, fh, O_NONBLOCK);
+	ASSERT_GE(pidfd, 0);
+
+	ASSERT_EQ(fstat(pidfd, &st2), 0);
+	ASSERT_TRUE(st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+
+	ASSERT_EQ(close(pidfd), 0);
+
+	pidfd = open_by_handle_at(self->pidfd, fh, 0);
+	ASSERT_GE(pidfd, 0);
+
+	ASSERT_EQ(fstat(pidfd, &st2), 0);
+	ASSERT_TRUE(st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+
+	ASSERT_EQ(close(pidfd), 0);
+
+	free(fh);
+}
+
 TEST_HARNESS_MAIN

-- 
2.47.2