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

[PATCH v3 0/3] 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.

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

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

I chose to implemnent this by using upper bits in the handle type field
It may be valid (?) for filesystems to return a handle type with upper
bits set, but AFAIK, no in-tree filesystem does that.
I added some assertions just in case.

Thanks,
Amir.

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

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

[1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
[2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/

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

 fs/exportfs/expfs.c        | 14 ++++++--
 fs/fhandle.c               | 74 ++++++++++++++++++++++++++++++++++----
 include/linux/exportfs.h   | 16 +++++++++
 include/uapi/linux/fcntl.h |  1 +
 4 files changed, 97 insertions(+), 8 deletions(-)

-- 
2.34.1

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

[Amir Goldstein]

We would like to use the high 16bit of the handle_type field to encode
file handle traits, such as "connectable".

In preparation for this change, make sure that filesystems do not return
a handle_type value with upper bits set and that the open_by_handle_at(2)
syscall rejects these handle types.

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

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

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

[Jeff Layton]

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

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

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


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


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

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

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

[Amir Goldstein]

On Tue, Oct 8, 2024 at 8:19 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> > We would like to use the high 16bit of the handle_type field to encode
> > file handle traits, such as "connectable".
> >
> > In preparation for this change, make sure that filesystems do not return
> > a handle_type value with upper bits set and that the open_by_handle_at(2)
> > syscall rejects these handle types.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/exportfs/expfs.c      | 14 ++++++++++++--
> >  fs/fhandle.c             |  6 ++++++
> >  include/linux/exportfs.h | 14 ++++++++++++++
> >  3 files changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index 4f2dd4ab4486..c8eb660fdde4 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> >                            int *max_len, struct inode *parent, int flags)
> >  {
> >       const struct export_operations *nop = inode->i_sb->s_export_op;
> > +     enum fid_type type;
> >
> >       if (!exportfs_can_encode_fh(nop, flags))
> >               return -EOPNOTSUPP;
> >
> >       if (!nop && (flags & EXPORT_FH_FID))
> > -             return exportfs_encode_ino64_fid(inode, fid, max_len);
> > +             type = exportfs_encode_ino64_fid(inode, fid, max_len);
> > +     else
> > +             type = nop->encode_fh(inode, fid->raw, max_len, parent);
> > +
> > +     if (WARN_ON_ONCE(FILEID_USER_FLAGS(type)))
> > +             return -EINVAL;
> > +
>
> The stack trace won't be very useful here. Rather than a WARN, it might
> be better to dump out some info about the fstype (and maybe other
> info?) that returned the bogus type value here. I'm pretty sure most
> in-kernel fs's don't do this, but who knows what 3rd party fs's might
> do.
>

Right. I changed to:

        if (FILEID_USER_FLAGS(type)) {
                pr_warn_once("%s: unexpected fh type value 0x%x from
fstype %s.\n",
                             __func__, type, inode->i_sb->s_type->name);
                return -EINVAL;
        }


> > +     return type;
> >
> > -     return nop->encode_fh(inode, fid->raw, max_len, parent);
> >  }
> >  EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
> >
> > @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
> >       char nbuf[NAME_MAX+1];
> >       int err;
> >
> > +     if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
> > +             return -EINVAL;
> > +
>
>
> This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which
> means that this fh comes from userland or from an NFS client. I don't
> think we want to WARN because someone crafted a bogus fh and passed it
> to us.
>

Good point, I will remove the WARN and also fix the bug :-/

        if (FILEID_USER_FLAGS(fileid_type))
                return ERR_PTR(-EINVAL);

Pushed this and othe minor fixes to
https://github.com/amir73il/linux/commits/connectable-fh/
until we sort out the rest of your comments and maybe get more feedback.

Thanks for the review!
Amir.

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

[Amir Goldstein]

On Tue, Oct 8, 2024 at 10:31 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Oct 8, 2024 at 8:19 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> > > We would like to use the high 16bit of the handle_type field to encode
> > > file handle traits, such as "connectable".
> > >
> > > In preparation for this change, make sure that filesystems do not return
> > > a handle_type value with upper bits set and that the open_by_handle_at(2)
> > > syscall rejects these handle types.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  fs/exportfs/expfs.c      | 14 ++++++++++++--
> > >  fs/fhandle.c             |  6 ++++++
> > >  include/linux/exportfs.h | 14 ++++++++++++++
> > >  3 files changed, 32 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > > index 4f2dd4ab4486..c8eb660fdde4 100644
> > > --- a/fs/exportfs/expfs.c
> > > +++ b/fs/exportfs/expfs.c
> > > @@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> > >                            int *max_len, struct inode *parent, int flags)
> > >  {
> > >       const struct export_operations *nop = inode->i_sb->s_export_op;
> > > +     enum fid_type type;
> > >
> > >       if (!exportfs_can_encode_fh(nop, flags))
> > >               return -EOPNOTSUPP;
> > >
> > >       if (!nop && (flags & EXPORT_FH_FID))
> > > -             return exportfs_encode_ino64_fid(inode, fid, max_len);
> > > +             type = exportfs_encode_ino64_fid(inode, fid, max_len);
> > > +     else
> > > +             type = nop->encode_fh(inode, fid->raw, max_len, parent);
> > > +
> > > +     if (WARN_ON_ONCE(FILEID_USER_FLAGS(type)))
> > > +             return -EINVAL;
> > > +
> >
> > The stack trace won't be very useful here. Rather than a WARN, it might
> > be better to dump out some info about the fstype (and maybe other
> > info?) that returned the bogus type value here. I'm pretty sure most
> > in-kernel fs's don't do this, but who knows what 3rd party fs's might
> > do.
> >
>
> Right. I changed to:
>
>         if (FILEID_USER_FLAGS(type)) {
>                 pr_warn_once("%s: unexpected fh type value 0x%x from
> fstype %s.\n",
>                              __func__, type, inode->i_sb->s_type->name);
>                 return -EINVAL;
>         }
>
>

FYI, following Jan's comment about mixing bitwise with negative values,
I changes this to:

        if (type > 0 && FILEID_USER_FLAGS(type)) {
                pr_warn_once("%s: unexpected fh type value 0x%x from
fstype %s.\n",
                             __func__, type, inode->i_sb->s_type->name);
                return -EINVAL;
        }

        return type;

because ->encode_fh() method are allowed to return a negative error
(e.g. -EOVERFLOW)

> > > +     return type;
> > >
> > > -     return nop->encode_fh(inode, fid->raw, max_len, parent);
> > >  }
> > >  EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
> > >
> > > @@ -436,6 +443,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
> > >       char nbuf[NAME_MAX+1];
> > >       int err;
> > >
> > > +     if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
> > > +             return -EINVAL;
> > > +
> >
> >
> > This is called from do_handle_to_path() or nfsd_set_fh_dentry(), which
> > means that this fh comes from userland or from an NFS client. I don't
> > think we want to WARN because someone crafted a bogus fh and passed it
> > to us.
> >
>
> Good point, I will remove the WARN and also fix the bug :-/
>
>         if (FILEID_USER_FLAGS(fileid_type))
>                 return ERR_PTR(-EINVAL);
>

And changed this to:

@@ -436,6 +446,9 @@ exportfs_decode_fh_raw(struct vfsmount *mnt,
struct fid *fid, int fh_len,
        char nbuf[NAME_MAX+1];
        int err;

+       if (fileid_type < 0 || FILEID_USER_FLAGS(fileid_type))
+               return ERR_PTR(-EINVAL);
+

> Pushed this and othe minor fixes to
> https://github.com/amir73il/linux/commits/connectable-fh/
> until we sort out the rest of your comments and maybe get more feedback.
>

If no further comments I will post v4.

Thanks,
Amir.

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

[kernel test robot]

Hi Amir,

kernel test robot noticed the following build errors:

[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on linus/master v6.12-rc2 next-20241011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Amir-Goldstein/fs-prepare-for-explicit-connectable-file-handles/20241008-232246
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20241008152118.453724-2-amir73il%40gmail.com
patch subject: [PATCH v3 1/3] fs: prepare for "explicit connectable" file handles
config: s390-allnoconfig (https://download.01.org/0day-ci/archive/20241011/202410111804.pPMW9rqn-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 70e0a7e7e6a8541bcc46908c592eed561850e416)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241011/202410111804.pPMW9rqn-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410111804.pPMW9rqn-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from fs/exportfs/expfs.c:15:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> fs/exportfs/expfs.c:447:10: error: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct dentry *' [-Wint-conversion]
     447 |                 return -EINVAL;
         |                        ^~~~~~~
   1 warning and 1 error generated.


vim +447 fs/exportfs/expfs.c

   434	
   435	struct dentry *
   436	exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
   437			       int fileid_type, unsigned int flags,
   438			       int (*acceptable)(void *, struct dentry *),
   439			       void *context)
   440	{
   441		const struct export_operations *nop = mnt->mnt_sb->s_export_op;
   442		struct dentry *result, *alias;
   443		char nbuf[NAME_MAX+1];
   444		int err;
   445	
   446		if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
 > 447			return -EINVAL;
   448	
   449		/*
   450		 * Try to get any dentry for the given file handle from the filesystem.
   451		 */
   452		if (!exportfs_can_decode_fh(nop))
   453			return ERR_PTR(-ESTALE);
   454		result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
   455		if (IS_ERR_OR_NULL(result))
   456			return result;
   457	
   458		if ((flags & EXPORT_FH_DIR_ONLY) && !d_is_dir(result)) {
   459			err = -ENOTDIR;
   460			goto err_result;
   461		}
   462	
   463		/*
   464		 * If no acceptance criteria was specified by caller, a disconnected
   465		 * dentry is also accepatable. Callers may use this mode to query if
   466		 * file handle is stale or to get a reference to an inode without
   467		 * risking the high overhead caused by directory reconnect.
   468		 */
   469		if (!acceptable)
   470			return result;
   471	
   472		if (d_is_dir(result)) {
   473			/*
   474			 * This request is for a directory.
   475			 *
   476			 * On the positive side there is only one dentry for each
   477			 * directory inode.  On the negative side this implies that we
   478			 * to ensure our dentry is connected all the way up to the
   479			 * filesystem root.
   480			 */
   481			if (result->d_flags & DCACHE_DISCONNECTED) {
   482				err = reconnect_path(mnt, result, nbuf);
   483				if (err)
   484					goto err_result;
   485			}
   486	
   487			if (!acceptable(context, result)) {
   488				err = -EACCES;
   489				goto err_result;
   490			}
   491	
   492			return result;
   493		} else {
   494			/*
   495			 * It's not a directory.  Life is a little more complicated.
   496			 */
   497			struct dentry *target_dir, *nresult;
   498	
   499			/*
   500			 * See if either the dentry we just got from the filesystem
   501			 * or any alias for it is acceptable.  This is always true
   502			 * if this filesystem is exported without the subtreecheck
   503			 * option.  If the filesystem is exported with the subtree
   504			 * check option there's a fair chance we need to look at
   505			 * the parent directory in the file handle and make sure
   506			 * it's connected to the filesystem root.
   507			 */
   508			alias = find_acceptable_alias(result, acceptable, context);
   509			if (alias)
   510				return alias;
   511	
   512			/*
   513			 * Try to extract a dentry for the parent directory from the
   514			 * file handle.  If this fails we'll have to give up.
   515			 */
   516			err = -ESTALE;
   517			if (!nop->fh_to_parent)
   518				goto err_result;
   519	
   520			target_dir = nop->fh_to_parent(mnt->mnt_sb, fid,
   521					fh_len, fileid_type);
   522			if (!target_dir)
   523				goto err_result;
   524			err = PTR_ERR(target_dir);
   525			if (IS_ERR(target_dir))
   526				goto err_result;
   527	
   528			/*
   529			 * And as usual we need to make sure the parent directory is
   530			 * connected to the filesystem root.  The VFS really doesn't
   531			 * like disconnected directories..
   532			 */
   533			err = reconnect_path(mnt, target_dir, nbuf);
   534			if (err) {
   535				dput(target_dir);
   536				goto err_result;
   537			}
   538	
   539			/*
   540			 * Now that we've got both a well-connected parent and a
   541			 * dentry for the inode we're after, make sure that our
   542			 * inode is actually connected to the parent.
   543			 */
   544			err = exportfs_get_name(mnt, target_dir, nbuf, result);
   545			if (err) {
   546				dput(target_dir);
   547				goto err_result;
   548			}
   549	
   550			inode_lock(target_dir->d_inode);
   551			nresult = lookup_one(mnt_idmap(mnt), nbuf,
   552					     target_dir, strlen(nbuf));
   553			if (!IS_ERR(nresult)) {
   554				if (unlikely(nresult->d_inode != result->d_inode)) {
   555					dput(nresult);
   556					nresult = ERR_PTR(-ESTALE);
   557				}
   558			}
   559			inode_unlock(target_dir->d_inode);
   560			/*
   561			 * At this point we are done with the parent, but it's pinned
   562			 * by the child dentry anyway.
   563			 */
   564			dput(target_dir);
   565	
   566			if (IS_ERR(nresult)) {
   567				err = PTR_ERR(nresult);
   568				goto err_result;
   569			}
   570			dput(result);
   571			result = nresult;
   572	
   573			/*
   574			 * And finally make sure the dentry is actually acceptable
   575			 * to NFSD.
   576			 */
   577			alias = find_acceptable_alias(result, acceptable, context);
   578			if (!alias) {
   579				err = -EACCES;
   580				goto err_result;
   581			}
   582	
   583			return alias;
   584		}
   585	
   586	 err_result:
   587		dput(result);
   588		return ERR_PTR(err);
   589	}
   590	EXPORT_SYMBOL_GPL(exportfs_decode_fh_raw);
   591	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Amir Goldstein]

On Fri, Oct 11, 2024 at 1:04 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Amir,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on brauner-vfs/vfs.all]
> [also build test ERROR on linus/master v6.12-rc2 next-20241011]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Amir-Goldstein/fs-prepare-for-explicit-connectable-file-handles/20241008-232246
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
> patch link:    https://lore.kernel.org/r/20241008152118.453724-2-amir73il%40gmail.com
> patch subject: [PATCH v3 1/3] fs: prepare for "explicit connectable" file handles
> config: s390-allnoconfig (https://download.01.org/0day-ci/archive/20241011/202410111804.pPMW9rqn-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 70e0a7e7e6a8541bcc46908c592eed561850e416)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241011/202410111804.pPMW9rqn-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410111804.pPMW9rqn-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    In file included from fs/exportfs/expfs.c:15:
>    In file included from include/linux/module.h:19:
>    In file included from include/linux/elf.h:6:
>    In file included from arch/s390/include/asm/elf.h:181:
>    In file included from arch/s390/include/asm/mmu_context.h:11:
>    In file included from arch/s390/include/asm/pgalloc.h:18:
>    In file included from include/linux/mm.h:2213:
>    include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
>      518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
>          |                               ~~~~~~~~~~~ ^ ~~~
> >> fs/exportfs/expfs.c:447:10: error: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct dentry *' [-Wint-conversion]
>      447 |                 return -EINVAL;
>          |                        ^~~~~~~
>    1 warning and 1 error generated.
>
>

This braino was already fixed in v4.

Sorry for the noise.

Thanks,
Amir.

> vim +447 fs/exportfs/expfs.c
>
>    434
>    435  struct dentry *
>    436  exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
>    437                         int fileid_type, unsigned int flags,
>    438                         int (*acceptable)(void *, struct dentry *),
>    439                         void *context)
>    440  {
>    441          const struct export_operations *nop = mnt->mnt_sb->s_export_op;
>    442          struct dentry *result, *alias;
>    443          char nbuf[NAME_MAX+1];
>    444          int err;
>    445
>    446          if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
>  > 447                  return -EINVAL;
>    448
>    449          /*
>    450           * Try to get any dentry for the given file handle from the filesystem.
>    451           */
>    452          if (!exportfs_can_decode_fh(nop))
>    453                  return ERR_PTR(-ESTALE);
>    454          result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
>    455          if (IS_ERR_OR_NULL(result))
>    456                  return result;
>    457
>    458          if ((flags & EXPORT_FH_DIR_ONLY) && !d_is_dir(result)) {
>    459                  err = -ENOTDIR;
>    460                  goto err_result;
>    461          }
>    462
>    463          /*
>    464           * If no acceptance criteria was specified by caller, a disconnected
>    465           * dentry is also accepatable. Callers may use this mode to query if
>    466           * file handle is stale or to get a reference to an inode without
>    467           * risking the high overhead caused by directory reconnect.
>    468           */
>    469          if (!acceptable)
>    470                  return result;
>    471
>    472          if (d_is_dir(result)) {
>    473                  /*
>    474                   * This request is for a directory.
>    475                   *
>    476                   * On the positive side there is only one dentry for each
>    477                   * directory inode.  On the negative side this implies that we
>    478                   * to ensure our dentry is connected all the way up to the
>    479                   * filesystem root.
>    480                   */
>    481                  if (result->d_flags & DCACHE_DISCONNECTED) {
>    482                          err = reconnect_path(mnt, result, nbuf);
>    483                          if (err)
>    484                                  goto err_result;
>    485                  }
>    486
>    487                  if (!acceptable(context, result)) {
>    488                          err = -EACCES;
>    489                          goto err_result;
>    490                  }
>    491
>    492                  return result;
>    493          } else {
>    494                  /*
>    495                   * It's not a directory.  Life is a little more complicated.
>    496                   */
>    497                  struct dentry *target_dir, *nresult;
>    498
>    499                  /*
>    500                   * See if either the dentry we just got from the filesystem
>    501                   * or any alias for it is acceptable.  This is always true
>    502                   * if this filesystem is exported without the subtreecheck
>    503                   * option.  If the filesystem is exported with the subtree
>    504                   * check option there's a fair chance we need to look at
>    505                   * the parent directory in the file handle and make sure
>    506                   * it's connected to the filesystem root.
>    507                   */
>    508                  alias = find_acceptable_alias(result, acceptable, context);
>    509                  if (alias)
>    510                          return alias;
>    511
>    512                  /*
>    513                   * Try to extract a dentry for the parent directory from the
>    514                   * file handle.  If this fails we'll have to give up.
>    515                   */
>    516                  err = -ESTALE;
>    517                  if (!nop->fh_to_parent)
>    518                          goto err_result;
>    519
>    520                  target_dir = nop->fh_to_parent(mnt->mnt_sb, fid,
>    521                                  fh_len, fileid_type);
>    522                  if (!target_dir)
>    523                          goto err_result;
>    524                  err = PTR_ERR(target_dir);
>    525                  if (IS_ERR(target_dir))
>    526                          goto err_result;
>    527
>    528                  /*
>    529                   * And as usual we need to make sure the parent directory is
>    530                   * connected to the filesystem root.  The VFS really doesn't
>    531                   * like disconnected directories..
>    532                   */
>    533                  err = reconnect_path(mnt, target_dir, nbuf);
>    534                  if (err) {
>    535                          dput(target_dir);
>    536                          goto err_result;
>    537                  }
>    538
>    539                  /*
>    540                   * Now that we've got both a well-connected parent and a
>    541                   * dentry for the inode we're after, make sure that our
>    542                   * inode is actually connected to the parent.
>    543                   */
>    544                  err = exportfs_get_name(mnt, target_dir, nbuf, result);
>    545                  if (err) {
>    546                          dput(target_dir);
>    547                          goto err_result;
>    548                  }
>    549
>    550                  inode_lock(target_dir->d_inode);
>    551                  nresult = lookup_one(mnt_idmap(mnt), nbuf,
>    552                                       target_dir, strlen(nbuf));
>    553                  if (!IS_ERR(nresult)) {
>    554                          if (unlikely(nresult->d_inode != result->d_inode)) {
>    555                                  dput(nresult);
>    556                                  nresult = ERR_PTR(-ESTALE);
>    557                          }
>    558                  }
>    559                  inode_unlock(target_dir->d_inode);
>    560                  /*
>    561                   * At this point we are done with the parent, but it's pinned
>    562                   * by the child dentry anyway.
>    563                   */
>    564                  dput(target_dir);
>    565
>    566                  if (IS_ERR(nresult)) {
>    567                          err = PTR_ERR(nresult);
>    568                          goto err_result;
>    569                  }
>    570                  dput(result);
>    571                  result = nresult;
>    572
>    573                  /*
>    574                   * And finally make sure the dentry is actually acceptable
>    575                   * to NFSD.
>    576                   */
>    577                  alias = find_acceptable_alias(result, acceptable, context);
>    578                  if (!alias) {
>    579                          err = -EACCES;
>    580                          goto err_result;
>    581                  }
>    582
>    583                  return alias;
>    584          }
>    585
>    586   err_result:
>    587          dput(result);
>    588          return ERR_PTR(err);
>    589  }
>    590  EXPORT_SYMBOL_GPL(exportfs_decode_fh_raw);
>    591
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Amir,

kernel test robot noticed the following build errors:

[auto build test ERROR on brauner-vfs/vfs.all]
[also build test ERROR on linus/master v6.12-rc2 next-20241011]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Amir-Goldstein/fs-prepare-for-explicit-connectable-file-handles/20241008-232246
base:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all
patch link:    https://lore.kernel.org/r/20241008152118.453724-2-amir73il%40gmail.com
patch subject: [PATCH v3 1/3] fs: prepare for "explicit connectable" file handles
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20241011/202410112259.QfHlqgCD-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241011/202410112259.QfHlqgCD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410112259.QfHlqgCD-lkp@intel.com/

All errors (new ones prefixed by >>):

   fs/exportfs/expfs.c: In function 'exportfs_decode_fh_raw':
>> fs/exportfs/expfs.c:447:24: error: returning 'int' from a function with return type 'struct dentry *' makes pointer from integer without a cast [-Wint-conversion]
     447 |                 return -EINVAL;
         |                        ^


vim +447 fs/exportfs/expfs.c

   434	
   435	struct dentry *
   436	exportfs_decode_fh_raw(struct vfsmount *mnt, struct fid *fid, int fh_len,
   437			       int fileid_type, unsigned int flags,
   438			       int (*acceptable)(void *, struct dentry *),
   439			       void *context)
   440	{
   441		const struct export_operations *nop = mnt->mnt_sb->s_export_op;
   442		struct dentry *result, *alias;
   443		char nbuf[NAME_MAX+1];
   444		int err;
   445	
   446		if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
 > 447			return -EINVAL;
   448	
   449		/*
   450		 * Try to get any dentry for the given file handle from the filesystem.
   451		 */
   452		if (!exportfs_can_decode_fh(nop))
   453			return ERR_PTR(-ESTALE);
   454		result = nop->fh_to_dentry(mnt->mnt_sb, fid, fh_len, fileid_type);
   455		if (IS_ERR_OR_NULL(result))
   456			return result;
   457	
   458		if ((flags & EXPORT_FH_DIR_ONLY) && !d_is_dir(result)) {
   459			err = -ENOTDIR;
   460			goto err_result;
   461		}
   462	
   463		/*
   464		 * If no acceptance criteria was specified by caller, a disconnected
   465		 * dentry is also accepatable. Callers may use this mode to query if
   466		 * file handle is stale or to get a reference to an inode without
   467		 * risking the high overhead caused by directory reconnect.
   468		 */
   469		if (!acceptable)
   470			return result;
   471	
   472		if (d_is_dir(result)) {
   473			/*
   474			 * This request is for a directory.
   475			 *
   476			 * On the positive side there is only one dentry for each
   477			 * directory inode.  On the negative side this implies that we
   478			 * to ensure our dentry is connected all the way up to the
   479			 * filesystem root.
   480			 */
   481			if (result->d_flags & DCACHE_DISCONNECTED) {
   482				err = reconnect_path(mnt, result, nbuf);
   483				if (err)
   484					goto err_result;
   485			}
   486	
   487			if (!acceptable(context, result)) {
   488				err = -EACCES;
   489				goto err_result;
   490			}
   491	
   492			return result;
   493		} else {
   494			/*
   495			 * It's not a directory.  Life is a little more complicated.
   496			 */
   497			struct dentry *target_dir, *nresult;
   498	
   499			/*
   500			 * See if either the dentry we just got from the filesystem
   501			 * or any alias for it is acceptable.  This is always true
   502			 * if this filesystem is exported without the subtreecheck
   503			 * option.  If the filesystem is exported with the subtree
   504			 * check option there's a fair chance we need to look at
   505			 * the parent directory in the file handle and make sure
   506			 * it's connected to the filesystem root.
   507			 */
   508			alias = find_acceptable_alias(result, acceptable, context);
   509			if (alias)
   510				return alias;
   511	
   512			/*
   513			 * Try to extract a dentry for the parent directory from the
   514			 * file handle.  If this fails we'll have to give up.
   515			 */
   516			err = -ESTALE;
   517			if (!nop->fh_to_parent)
   518				goto err_result;
   519	
   520			target_dir = nop->fh_to_parent(mnt->mnt_sb, fid,
   521					fh_len, fileid_type);
   522			if (!target_dir)
   523				goto err_result;
   524			err = PTR_ERR(target_dir);
   525			if (IS_ERR(target_dir))
   526				goto err_result;
   527	
   528			/*
   529			 * And as usual we need to make sure the parent directory is
   530			 * connected to the filesystem root.  The VFS really doesn't
   531			 * like disconnected directories..
   532			 */
   533			err = reconnect_path(mnt, target_dir, nbuf);
   534			if (err) {
   535				dput(target_dir);
   536				goto err_result;
   537			}
   538	
   539			/*
   540			 * Now that we've got both a well-connected parent and a
   541			 * dentry for the inode we're after, make sure that our
   542			 * inode is actually connected to the parent.
   543			 */
   544			err = exportfs_get_name(mnt, target_dir, nbuf, result);
   545			if (err) {
   546				dput(target_dir);
   547				goto err_result;
   548			}
   549	
   550			inode_lock(target_dir->d_inode);
   551			nresult = lookup_one(mnt_idmap(mnt), nbuf,
   552					     target_dir, strlen(nbuf));
   553			if (!IS_ERR(nresult)) {
   554				if (unlikely(nresult->d_inode != result->d_inode)) {
   555					dput(nresult);
   556					nresult = ERR_PTR(-ESTALE);
   557				}
   558			}
   559			inode_unlock(target_dir->d_inode);
   560			/*
   561			 * At this point we are done with the parent, but it's pinned
   562			 * by the child dentry anyway.
   563			 */
   564			dput(target_dir);
   565	
   566			if (IS_ERR(nresult)) {
   567				err = PTR_ERR(nresult);
   568				goto err_result;
   569			}
   570			dput(result);
   571			result = nresult;
   572	
   573			/*
   574			 * And finally make sure the dentry is actually acceptable
   575			 * to NFSD.
   576			 */
   577			alias = find_acceptable_alias(result, acceptable, context);
   578			if (!alias) {
   579				err = -EACCES;
   580				goto err_result;
   581			}
   582	
   583			return alias;
   584		}
   585	
   586	 err_result:
   587		dput(result);
   588		return ERR_PTR(err);
   589	}
   590	EXPORT_SYMBOL_GPL(exportfs_decode_fh_raw);
   591	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Amir Goldstein]

nfsd encodes "connectable" file handles for the subtree_check feature,
which can be resolved to an open file with a connected path.
So far, userspace nfs server could not make use of this functionality.

Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
When used, the encoded file handle is "explicitly connectable".

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

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

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

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

diff --git a/fs/fhandle.c b/fs/fhandle.c
index c5792cf3c6e9..7b4c8945efcb 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -31,6 +31,14 @@ static long do_sys_name_to_handle(const struct path *path,
 	if (!exportfs_can_encode_fh(path->dentry->d_sb->s_export_op, fh_flags))
 		return -EOPNOTSUPP;
 
+	/*
+	 * A request to encode a connectable handle for a disconnected dentry
+	 * is unexpected since AT_EMPTY_PATH is not allowed.
+	 */
+	if (fh_flags & EXPORT_FH_CONNECTABLE &&
+	    WARN_ON(path->dentry->d_flags & DCACHE_DISCONNECTED))
+		return -EINVAL;
+
 	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
 		return -EFAULT;
 
@@ -45,7 +53,7 @@ static long do_sys_name_to_handle(const struct path *path,
 	/* convert handle size to multiple of sizeof(u32) */
 	handle_dwords = f_handle.handle_bytes >> 2;
 
-	/* we ask for a non connectable maybe decodeable file handle */
+	/* Encode a possibly decodeable/connectable file handle */
 	retval = exportfs_encode_fh(path->dentry,
 				    (struct fid *)handle->f_handle,
 				    &handle_dwords, fh_flags);
@@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
 		 * non variable part of the file_handle
 		 */
 		handle_bytes = 0;
-	} else
+	} else {
+		/*
+		 * When asked to encode a connectable file handle, encode this
+		 * property in the file handle itself, so that we later know
+		 * how to decode it.
+		 * For sanity, also encode in the file handle if the encoded
+		 * object is a directory and verify this during decode, because
+		 * decoding directory file handles is quite different than
+		 * decoding connectable non-directory file handles.
+		 */
+		if (fh_flags & EXPORT_FH_CONNECTABLE) {
+			handle->handle_type |= FILEID_IS_CONNECTABLE;
+			if (d_is_dir(path->dentry))
+				fh_flags |= FILEID_IS_DIR;
+		}
 		retval = 0;
+	}
 	/* copy the mount id */
 	if (unique_mntid) {
 		if (put_user(real_mount(path->mnt)->mnt_id_unique,
@@ -109,15 +132,30 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 {
 	struct path path;
 	int lookup_flags;
-	int fh_flags;
+	int fh_flags = 0;
 	int err;
 
 	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
-		     AT_HANDLE_MNT_ID_UNIQUE))
+		     AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
+		return -EINVAL;
+
+	/*
+	 * AT_HANDLE_FID means there is no intention to decode file handle
+	 * AT_HANDLE_CONNECTABLE means there is an intention to decode a
+	 * connected fd (with known path), so these flags are conflicting.
+	 * AT_EMPTY_PATH could be used along with a dfd that refers to a
+	 * disconnected non-directory, which cannot be used to encode a
+	 * connectable file handle, because its parent is unknown.
+	 */
+	if (flag & AT_HANDLE_CONNECTABLE &&
+	    flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
 		return -EINVAL;
+	else if (flag & AT_HANDLE_FID)
+		fh_flags |= EXPORT_FH_FID;
+	else if (flag & AT_HANDLE_CONNECTABLE)
+		fh_flags |= EXPORT_FH_CONNECTABLE;
 
 	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
-	fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
 	if (flag & AT_EMPTY_PATH)
 		lookup_flags |= LOOKUP_EMPTY;
 	err = user_path_at(dfd, name, lookup_flags, &path);
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 76a3050b3593..230b0e1d669d 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -169,6 +169,8 @@ struct fid {
 #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
 
 /* Flags supported in encoded handle_type that is exported to user */
+#define FILEID_IS_CONNECTABLE	0x10000
+#define FILEID_IS_DIR		0x40000
 #define FILEID_VALID_USER_FLAGS	(0)
 
 #define FILEID_USER_TYPE_IS_VALID(type) \
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

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

[Jeff Layton]

On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> nfsd encodes "connectable" file handles for the subtree_check feature,
> which can be resolved to an open file with a connected path.
> So far, userspace nfs server could not make use of this functionality.
> 
> Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
> When used, the encoded file handle is "explicitly connectable".
> 
> The "explicitly connectable" file handle sets bits in the high 16bit of
> the handle_type field, so open_by_handle_at(2) will know that it needs
> to open a file with a connected path.
> 
> old kernels will now recognize the handle_type with high bits set,
> so "explicitly connectable" file handles cannot be decoded by
> open_by_handle_at(2) on old kernels.
> 
> The flag AT_HANDLE_CONNECTABLE is not allowed together with either
> AT_HANDLE_FID or AT_EMPTY_PATH.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
>  include/linux/exportfs.h   |  2 ++
>  include/uapi/linux/fcntl.h |  1 +
>  3 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index c5792cf3c6e9..7b4c8945efcb 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;
> +

Is it possible to get a DCACHE_DISCONNECTED dentry here? This thing
comes from a userland path walk (a'la name_to_handle_at()). That means
that it necessarily is connected.

I'd drop the fh_flags check, since it seems like having that set on any
dentry we get in this interface would be a bug.

>  	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
>  		return -EFAULT;
>  
> @@ -45,7 +53,7 @@ static long do_sys_name_to_handle(const struct path *path,
>  	/* convert handle size to multiple of sizeof(u32) */
>  	handle_dwords = f_handle.handle_bytes >> 2;
>  
> -	/* we ask for a non connectable maybe decodeable file handle */
> +	/* Encode a possibly decodeable/connectable file handle */
>  	retval = exportfs_encode_fh(path->dentry,
>  				    (struct fid *)handle->f_handle,
>  				    &handle_dwords, fh_flags);
> @@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
>  		 * non variable part of the file_handle
>  		 */
>  		handle_bytes = 0;
> -	} else
> +	} else {
> +		/*
> +		 * When asked to encode a connectable file handle, encode this
> +		 * property in the file handle itself, so that we later know
> +		 * how to decode it.
> +		 * For sanity, also encode in the file handle if the encoded
> +		 * object is a directory and verify this during decode, because
> +		 * decoding directory file handles is quite different than
> +		 * decoding connectable non-directory file handles.
> +		 */
> +		if (fh_flags & EXPORT_FH_CONNECTABLE) {
> +			handle->handle_type |= FILEID_IS_CONNECTABLE;
> +			if (d_is_dir(path->dentry))
> +				fh_flags |= FILEID_IS_DIR;
> +		}
>  		retval = 0;
> +	}
>  	/* copy the mount id */
>  	if (unique_mntid) {
>  		if (put_user(real_mount(path->mnt)->mnt_id_unique,
> @@ -109,15 +132,30 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
>  {
>  	struct path path;
>  	int lookup_flags;
> -	int fh_flags;
> +	int fh_flags = 0;
>  	int err;
>  
>  	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> -		     AT_HANDLE_MNT_ID_UNIQUE))
> +		     AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
> +		return -EINVAL;
> +
> +	/*
> +	 * AT_HANDLE_FID means there is no intention to decode file handle
> +	 * AT_HANDLE_CONNECTABLE means there is an intention to decode a
> +	 * connected fd (with known path), so these flags are conflicting.
> +	 * AT_EMPTY_PATH could be used along with a dfd that refers to a
> +	 * disconnected non-directory, which cannot be used to encode a
> +	 * connectable file handle, because its parent is unknown.
> +	 */
> +	if (flag & AT_HANDLE_CONNECTABLE &&
> +	    flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
>  		return -EINVAL;
> +	else if (flag & AT_HANDLE_FID)
> +		fh_flags |= EXPORT_FH_FID;
> +	else if (flag & AT_HANDLE_CONNECTABLE)
> +		fh_flags |= EXPORT_FH_CONNECTABLE;
>  
>  	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> -	fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
>  	if (flag & AT_EMPTY_PATH)
>  		lookup_flags |= LOOKUP_EMPTY;
>  	err = user_path_at(dfd, name, lookup_flags, &path);
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 76a3050b3593..230b0e1d669d 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -169,6 +169,8 @@ struct fid {
>  #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
>  
>  /* Flags supported in encoded handle_type that is exported to user */
> +#define FILEID_IS_CONNECTABLE	0x10000
> +#define FILEID_IS_DIR		0x40000

nit: why skip 0x20000 ?

>  #define FILEID_VALID_USER_FLAGS	(0)
>  
>  #define FILEID_USER_TYPE_IS_VALID(type) \
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 87e2dec79fea..56ff2100e021 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -153,6 +153,7 @@
>  					   object identity and may not be
>  					   usable with open_by_handle_at(2). */
>  #define AT_HANDLE_MNT_ID_UNIQUE	0x001	/* Return the u64 unique mount ID. */
> +#define AT_HANDLE_CONNECTABLE	0x002	/* Request a connectable file handle */
>  
>  #if defined(__KERNEL__)
>  #define AT_GETATTR_NOSEC	0x80000000

-- 
Jeff Layton <jlayton@kernel.org>

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

[Amir Goldstein]

On Tue, Oct 8, 2024 at 8:31 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> > nfsd encodes "connectable" file handles for the subtree_check feature,
> > which can be resolved to an open file with a connected path.
> > So far, userspace nfs server could not make use of this functionality.
> >
> > Introduce a new flag AT_HANDLE_CONNECTABLE to name_to_handle_at(2).
> > When used, the encoded file handle is "explicitly connectable".
> >
> > The "explicitly connectable" file handle sets bits in the high 16bit of
> > the handle_type field, so open_by_handle_at(2) will know that it needs
> > to open a file with a connected path.
> >
> > old kernels will now recognize the handle_type with high bits set,
> > so "explicitly connectable" file handles cannot be decoded by
> > open_by_handle_at(2) on old kernels.
> >
> > The flag AT_HANDLE_CONNECTABLE is not allowed together with either
> > AT_HANDLE_FID or AT_EMPTY_PATH.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/fhandle.c               | 48 ++++++++++++++++++++++++++++++++++----
> >  include/linux/exportfs.h   |  2 ++
> >  include/uapi/linux/fcntl.h |  1 +
> >  3 files changed, 46 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index c5792cf3c6e9..7b4c8945efcb 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;
> > +
>
> Is it possible to get a DCACHE_DISCONNECTED dentry here? This thing
> comes from a userland path walk (a'la name_to_handle_at()). That means
> that it necessarily is connected.

Can't you get it with AT_EMPTY_PATH if dfd is obtained by open_by_handle()?

>
> I'd drop the fh_flags check, since it seems like having that set on any
> dentry we get in this interface would be a bug.
>

Well I don't know if giving a disconnected dentry to name_to_handle
is intentional, but it is not wrong, unless user actually requests a
connectable file handle. No?

> >       if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
> >               return -EFAULT;
> >
> > @@ -45,7 +53,7 @@ static long do_sys_name_to_handle(const struct path *path,
> >       /* convert handle size to multiple of sizeof(u32) */
> >       handle_dwords = f_handle.handle_bytes >> 2;
> >
> > -     /* we ask for a non connectable maybe decodeable file handle */
> > +     /* Encode a possibly decodeable/connectable file handle */
> >       retval = exportfs_encode_fh(path->dentry,
> >                                   (struct fid *)handle->f_handle,
> >                                   &handle_dwords, fh_flags);
> > @@ -67,8 +75,23 @@ static long do_sys_name_to_handle(const struct path *path,
> >                * non variable part of the file_handle
> >                */
> >               handle_bytes = 0;
> > -     } else
> > +     } else {
> > +             /*
> > +              * When asked to encode a connectable file handle, encode this
> > +              * property in the file handle itself, so that we later know
> > +              * how to decode it.
> > +              * For sanity, also encode in the file handle if the encoded
> > +              * object is a directory and verify this during decode, because
> > +              * decoding directory file handles is quite different than
> > +              * decoding connectable non-directory file handles.
> > +              */
> > +             if (fh_flags & EXPORT_FH_CONNECTABLE) {
> > +                     handle->handle_type |= FILEID_IS_CONNECTABLE;
> > +                     if (d_is_dir(path->dentry))
> > +                             fh_flags |= FILEID_IS_DIR;
> > +             }
> >               retval = 0;
> > +     }
> >       /* copy the mount id */
> >       if (unique_mntid) {
> >               if (put_user(real_mount(path->mnt)->mnt_id_unique,
> > @@ -109,15 +132,30 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
> >  {
> >       struct path path;
> >       int lookup_flags;
> > -     int fh_flags;
> > +     int fh_flags = 0;
> >       int err;
> >
> >       if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID |
> > -                  AT_HANDLE_MNT_ID_UNIQUE))
> > +                  AT_HANDLE_MNT_ID_UNIQUE | AT_HANDLE_CONNECTABLE))
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * AT_HANDLE_FID means there is no intention to decode file handle
> > +      * AT_HANDLE_CONNECTABLE means there is an intention to decode a
> > +      * connected fd (with known path), so these flags are conflicting.
> > +      * AT_EMPTY_PATH could be used along with a dfd that refers to a
> > +      * disconnected non-directory, which cannot be used to encode a
> > +      * connectable file handle, because its parent is unknown.
> > +      */
> > +     if (flag & AT_HANDLE_CONNECTABLE &&
> > +         flag & (AT_HANDLE_FID | AT_EMPTY_PATH))
> >               return -EINVAL;
> > +     else if (flag & AT_HANDLE_FID)
> > +             fh_flags |= EXPORT_FH_FID;
> > +     else if (flag & AT_HANDLE_CONNECTABLE)
> > +             fh_flags |= EXPORT_FH_CONNECTABLE;
> >
> >       lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
> > -     fh_flags = (flag & AT_HANDLE_FID) ? EXPORT_FH_FID : 0;
> >       if (flag & AT_EMPTY_PATH)
> >               lookup_flags |= LOOKUP_EMPTY;
> >       err = user_path_at(dfd, name, lookup_flags, &path);
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index 76a3050b3593..230b0e1d669d 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -169,6 +169,8 @@ struct fid {
> >  #define FILEID_USER_FLAGS(type) ((type) & FILEID_USER_FLAGS_MASK)
> >
> >  /* Flags supported in encoded handle_type that is exported to user */
> > +#define FILEID_IS_CONNECTABLE        0x10000
> > +#define FILEID_IS_DIR                0x40000
>
> nit: why skip 0x20000 ?

Well in v2, the values were just shifting the EXPORT_FH_ flags left
so I kept it this way, although I am not sure if FILEID_ID_FID is ever
going to be useful, so I guess there is no good reason to skip 0x20000

Thanks,
Amir.

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

[Amir Goldstein]

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

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

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

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

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

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

[Jeff Layton]

On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> Teach open_by_handle_at(2) about the type format of "explicit connectable"
> file handles that were created using the AT_HANDLE_CONNECTABLE flag to
> name_to_handle_at(2).
> 
> When decoding an "explicit connectable" file handles, name_to_handle_at(2)
> should fail if it cannot open a "connected" fd with known path, which is
> accessible (to capable user) from mount fd path.
> 
> Note that this does not check if the path is accessible to the calling
> user, just that it is accessible wrt the mount namesapce, so if there
> is no "connected" alias, or if parts of the path are hidden in the
> mount namespace, open_by_handle_at(2) will return -ESTALE.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/fhandle.c             | 20 +++++++++++++++++++-
>  include/linux/exportfs.h |  2 +-
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fhandle.c b/fs/fhandle.c
> index 7b4c8945efcb..6a5458c3c6c9 100644
> --- a/fs/fhandle.c
> +++ b/fs/fhandle.c
> @@ -246,7 +246,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
>  
>  	if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
>  		retval = 1;
> -	WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> +	/*
> +	 * exportfs_decode_fh_raw() does not call acceptable() callback with
> +	 * a disconnected directory dentry, so we should have reached either
> +	 * mount fd directory or sb root.
> +	 */
> +	if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> +		WARN_ON_ONCE(d != root && d != root->d_sb->s_root);

I don't quite get the test for EXPORT_FH_DIR_ONLY here. Why does this
change require that instead of just continuing to WARN unconditionally?

>  	dput(d);
>  	return retval;
>  }
> @@ -349,6 +355,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		retval = -EINVAL;
>  		goto out_path;
>  	}
> +
>  	handle = kmalloc(struct_size(handle, f_handle, f_handle.handle_bytes),
>  			 GFP_KERNEL);
>  	if (!handle) {
> @@ -364,6 +371,17 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
>  		goto out_handle;
>  	}
>  
> +	/*
> +	 * If handle was encoded with AT_HANDLE_CONNECTABLE, verify that we
> +	 * are decoding an fd with connected path, which is accessible from
> +	 * the mount fd path.
> +	 */
> +	if (f_handle.handle_type & FILEID_IS_CONNECTABLE) {
> +		ctx.fh_flags |= EXPORT_FH_CONNECTABLE;
> +		ctx.flags |= HANDLE_CHECK_SUBTREE;
> +	}
> +	if (f_handle.handle_type & FILEID_IS_DIR)
> +		ctx.fh_flags |= EXPORT_FH_DIR_ONLY;
>  	/* Filesystem code should not be exposed to user flags */
>  	handle->handle_type &= ~FILEID_USER_FLAGS_MASK;
>  	retval = do_handle_to_path(handle, path, &ctx);
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 230b0e1d669d..a48d4c94ff74 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -171,7 +171,7 @@ struct fid {
>  /* Flags supported in encoded handle_type that is exported to user */
>  #define FILEID_IS_CONNECTABLE	0x10000
>  #define FILEID_IS_DIR		0x40000
> -#define FILEID_VALID_USER_FLAGS	(0)
> +#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))

-- 
Jeff Layton <jlayton@kernel.org>

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

[Amir Goldstein]

On Tue, Oct 8, 2024 at 8:37 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2024-10-08 at 17:21 +0200, Amir Goldstein wrote:
> > Teach open_by_handle_at(2) about the type format of "explicit connectable"
> > file handles that were created using the AT_HANDLE_CONNECTABLE flag to
> > name_to_handle_at(2).
> >
> > When decoding an "explicit connectable" file handles, name_to_handle_at(2)
> > should fail if it cannot open a "connected" fd with known path, which is
> > accessible (to capable user) from mount fd path.
> >
> > Note that this does not check if the path is accessible to the calling
> > user, just that it is accessible wrt the mount namesapce, so if there
> > is no "connected" alias, or if parts of the path are hidden in the
> > mount namespace, open_by_handle_at(2) will return -ESTALE.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/fhandle.c             | 20 +++++++++++++++++++-
> >  include/linux/exportfs.h |  2 +-
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fhandle.c b/fs/fhandle.c
> > index 7b4c8945efcb..6a5458c3c6c9 100644
> > --- a/fs/fhandle.c
> > +++ b/fs/fhandle.c
> > @@ -246,7 +246,13 @@ static int vfs_dentry_acceptable(void *context, struct dentry *dentry)
> >
> >       if (!(ctx->flags & HANDLE_CHECK_SUBTREE) || d == root)
> >               retval = 1;
> > -     WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
> > +     /*
> > +      * exportfs_decode_fh_raw() does not call acceptable() callback with
> > +      * a disconnected directory dentry, so we should have reached either
> > +      * mount fd directory or sb root.
> > +      */
> > +     if (ctx->fh_flags & EXPORT_FH_DIR_ONLY)
> > +             WARN_ON_ONCE(d != root && d != root->d_sb->s_root);
>
> I don't quite get the test for EXPORT_FH_DIR_ONLY here. Why does this
> change require that instead of just continuing to WARN unconditionally?
>

The reason is at the end of may_decode_fh(), you have:
       ctx->fh_flags = EXPORT_FH_DIR_ONLY;
       return true;

So until THIS patch, vfs_dentry_acceptable() was always called
with EXPORT_FH_DIR_ONLY.

THIS patch adds another use case where HANDLE_CHECK_SUBTREE
is being requested, but this time EXPORT_FH_DIR_ONLY is optional.

The comment above "exportfs_decode_fh_raw() does not call acceptable()..."
explains why the assertion is only true for directories.

Thanks,
Amir.

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

[Amir Goldstein]

On Tue, Oct 8, 2024 at 5:21 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> 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.
>
> The API I chose for encoding conenctable file handles is pretty
> conventional (AT_HANDLE_CONNECTABLE).
>
> open_by_handle_at(2) does not have AT_ flags argument, but also, I find
> it more useful API that encoding a connectable file handle can mandate
> the resolving of a connected fd, without having to opt-in for a
> connected fd independently.
>
> I chose to implemnent this by using upper bits in the handle type field
> It may be valid (?) for filesystems to return a handle type with upper
> bits set, but AFAIK, no in-tree filesystem does that.
> I added some assertions just in case.

FYI, version with softened assertions at:
https://github.com/amir73il/linux/commits/connectable-fh/

fstest at:
https://github.com/amir73il/xfstests/commits/connectable-fh/

man-page at:
https://github.com/amir73il/man-pages/commits/connectable-fh/

>
> Thanks,
> Amir.
>
> Changes since v2 [2]:
> - Use bit arithmetics instead of bitfileds (Jeff)
> - Add assertions about use of high type bits
>
> Changes since v1 [1]:
> - Assert on encode for disconnected path (Jeff)
> - Don't allow AT_HANDLE_CONNECTABLE with AT_EMPTY_PATH
> - Drop the O_PATH mount_fd API hack (Jeff)
> - Encode an explicit "connectable" flag in handle type
>
> [1] https://lore.kernel.org/linux-fsdevel/20240919140611.1771651-1-amir73il@gmail.com/
> [2] https://lore.kernel.org/linux-fsdevel/20240923082829.1910210-1-amir73il@gmail.com/
>
> Amir Goldstein (3):
>   fs: prepare for "explicit connectable" file handles
>   fs: name_to_handle_at() support for "explicit connectable" file
>     handles
>   fs: open_by_handle_at() support for decoding "explicit connectable"
>     file handles
>
>  fs/exportfs/expfs.c        | 14 ++++++--
>  fs/fhandle.c               | 74 ++++++++++++++++++++++++++++++++++----
>  include/linux/exportfs.h   | 16 +++++++++
>  include/uapi/linux/fcntl.h |  1 +
>  4 files changed, 97 insertions(+), 8 deletions(-)
>
> --
> 2.34.1
>