[RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

Jan,

Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
alternative proposal to seamlessly support more filesystems.

While fanotify relaxes the requirements for filesystems to support
reporting fid to require only the ->encode_fh() operation, there are
currently no new filesystems that meet the relaxed requirements.

I will shortly post patches that allow overlayfs to meet the new
requirements with default overlay configurations.

The overlay and vfs/fanotify patch sets are completely independent.
The are both available on my github branch [2] and there is a simple
LTP test variant that tests reporting fid from overlayfs [3], which
also demonstrates the minor UAPI change of name_to_handle_at(2) for
requesting a non-decodeable file handle by userspace.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
[2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
[3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid

Amir Goldstein (4):
  exportfs: change connectable argument to bit flags
  exportfs: add explicit flag to request non-decodeable file handles
  exportfs: allow exporting non-decodeable file handles to userspace
  fanotify: support reporting non-decodeable file handles

 Documentation/filesystems/nfs/exporting.rst |  4 +--
 fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
 fs/fhandle.c                                | 20 ++++++++------
 fs/nfsd/nfsfh.c                             |  5 ++--
 fs/notify/fanotify/fanotify.c               |  4 +--
 fs/notify/fanotify/fanotify_user.c          |  6 ++---
 fs/notify/fdinfo.c                          |  2 +-
 include/linux/exportfs.h                    | 18 ++++++++++---
 include/uapi/linux/fcntl.h                  |  5 ++++
 9 files changed, 67 insertions(+), 26 deletions(-)

-- 
2.34.1

[RFC][PATCH 1/4] exportfs: change connectable argument to bit flags

[Amir Goldstein]

Convert the bool connectable arguemnt into a bit flags argument and
define the EXPORT_FS_CONNECTABLE flag as a requested property of the
file handle.

We are going to add a flag for requesting non-decodeable file handles.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/exportfs/expfs.c      | 11 +++++++++--
 fs/nfsd/nfsfh.c          |  5 +++--
 include/linux/exportfs.h |  6 ++++--
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index ab88d33d106c..bf1b4925fedd 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -393,14 +393,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
 }
 EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
 
+/**
+ * exportfs_encode_fh - encode a file handle from dentry
+ * @dentry:  the object to encode
+ * @fid:     where to store the file handle fragment
+ * @max_len: maximum length to store there
+ * @flags:   properties of the requrested file handle
+ */
 int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
-		int connectable)
+		       int flags)
 {
 	int error;
 	struct dentry *p = NULL;
 	struct inode *inode = dentry->d_inode, *parent = NULL;
 
-	if (connectable && !S_ISDIR(inode->i_mode)) {
+	if ((flags & EXPORT_FH_CONNECTABLE) && !S_ISDIR(inode->i_mode)) {
 		p = dget_parent(dentry);
 		/*
 		 * note that while p might've ceased to be our parent already,
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index ccd8485fee04..31e4505c0df3 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -414,10 +414,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp,
 		struct fid *fid = (struct fid *)
 			(fhp->fh_handle.fh_fsid + fhp->fh_handle.fh_size/4 - 1);
 		int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4;
-		int subtreecheck = !(exp->ex_flags & NFSEXP_NOSUBTREECHECK);
+		int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 :
+				EXPORT_FH_CONNECTABLE;
 
 		fhp->fh_handle.fh_fileid_type =
-			exportfs_encode_fh(dentry, fid, &maxsize, subtreecheck);
+			exportfs_encode_fh(dentry, fid, &maxsize, fh_flags);
 		fhp->fh_handle.fh_size += maxsize * 4;
 	} else {
 		fhp->fh_handle.fh_fileid_type = FILEID_ROOT;
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 601700fedc91..2b1048238170 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -135,6 +135,8 @@ struct fid {
 	};
 };
 
+#define EXPORT_FH_CONNECTABLE	0x1
+
 /**
  * struct export_operations - for nfsd to communicate with file systems
  * @encode_fh:      encode a file handle fragment from a dentry
@@ -150,7 +152,7 @@ struct fid {
  * encode_fh:
  *    @encode_fh should store in the file handle fragment @fh (using at most
  *    @max_len bytes) information that can be used by @decode_fh to recover the
- *    file referred to by the &struct dentry @de.  If the @connectable flag is
+ *    file referred to by the &struct dentry @de.  If @flag has CONNECTABLE bit
  *    set, the encode_fh() should store sufficient information so that a good
  *    attempt can be made to find not only the file but also it's place in the
  *    filesystem.   This typically means storing a reference to de->d_parent in
@@ -226,7 +228,7 @@ struct export_operations {
 extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
 				    int *max_len, struct inode *parent);
 extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
-	int *max_len, int connectable);
+			      int *max_len, int flags);
 extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
 					     struct fid *fid, int fh_len,
 					     int fileid_type,
-- 
2.34.1

[RFC][PATCH 2/4] exportfs: add explicit flag to request non-decodeable file handles

[Amir Goldstein]

So far, all callers of exportfs_encode_inode_fh(), except for fsnotify's
show_mark_fhandle(), check that filesystem can decode file handles, but
we would like to add more callers that do not require a file handle that
can be decoded.

Introduce a flag to explicitly request a file handle that may not to be
decoded later and a wrapper exportfs_encode_fid() that sets this flag
and convert show_mark_fhandle() to use the new wrapper.

This will be used to allow adding fanotify support to filesystems that
do not support NFS export.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 Documentation/filesystems/nfs/exporting.rst |  4 ++--
 fs/exportfs/expfs.c                         | 18 ++++++++++++++++--
 fs/notify/fanotify/fanotify.c               |  4 ++--
 fs/notify/fdinfo.c                          |  2 +-
 include/linux/exportfs.h                    | 12 +++++++++++-
 5 files changed, 32 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
index 0e98edd353b5..3d97b8d8f735 100644
--- a/Documentation/filesystems/nfs/exporting.rst
+++ b/Documentation/filesystems/nfs/exporting.rst
@@ -122,8 +122,8 @@ are exportable by setting the s_export_op field in the struct
 super_block.  This field must point to a "struct export_operations"
 struct which has the following members:
 
- encode_fh  (optional)
-    Takes a dentry and creates a filehandle fragment which can later be used
+  encode_fh (optional)
+    Takes a dentry and creates a filehandle fragment which may later be used
     to find or create a dentry for the same object.  The default
     implementation creates a filehandle fragment that encodes a 32bit inode
     and generation number for the inode encoded, and if necessary the
diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
index bf1b4925fedd..1b35dda5bdda 100644
--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -381,11 +381,25 @@ static int export_encode_fh(struct inode *inode, struct fid *fid,
 	return type;
 }
 
+/**
+ * exportfs_encode_inode_fh - encode a file handle from inode
+ * @inode:   the object to encode
+ * @fid:     where to store the file handle fragment
+ * @max_len: maximum length to store there
+ * @flags:   properties of the requrested file handle
+ */
 int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
-			     int *max_len, struct inode *parent)
+			     int *max_len, struct inode *parent, int flags)
 {
 	const struct export_operations *nop = inode->i_sb->s_export_op;
 
+	/*
+	 * If a decodeable file handle was requested, we need to make sure that
+	 * filesystem can decode file handles.
+	 */
+	if (nop && !(flags & EXPORT_FH_FID) && !nop->fh_to_dentry)
+		return -EOPNOTSUPP;
+
 	if (nop && nop->encode_fh)
 		return nop->encode_fh(inode, fid->raw, max_len, parent);
 
@@ -416,7 +430,7 @@ int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
 		parent = p->d_inode;
 	}
 
-	error = exportfs_encode_inode_fh(inode, fid, max_len, parent);
+	error = exportfs_encode_inode_fh(inode, fid, max_len, parent, flags);
 	dput(p);
 
 	return error;
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 29bdd99b29fa..d1a49f5b6e6d 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -380,7 +380,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
 	if (!inode)
 		return 0;
 
-	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
+	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL, 0);
 	fh_len = dwords << 2;
 
 	/*
@@ -443,7 +443,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	}
 
 	dwords = fh_len >> 2;
-	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
+	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL, 0);
 	err = -EINVAL;
 	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
 		goto out_err;
diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index 55081ae3a6ec..5c430736ec12 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -50,7 +50,7 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
 	f.handle.handle_bytes = sizeof(f.pad);
 	size = f.handle.handle_bytes >> 2;
 
-	ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
+	ret = exportfs_encode_fid(inode, (struct fid *)f.handle.f_handle, &size);
 	if ((ret == FILEID_INVALID) || (ret < 0)) {
 		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
 		return;
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 2b1048238170..635e89e1dae7 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -136,6 +136,7 @@ struct fid {
 };
 
 #define EXPORT_FH_CONNECTABLE	0x1
+#define EXPORT_FH_FID		0x2
 
 /**
  * struct export_operations - for nfsd to communicate with file systems
@@ -226,9 +227,18 @@ struct export_operations {
 };
 
 extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
-				    int *max_len, struct inode *parent);
+				    int *max_len, struct inode *parent,
+				    int flags);
 extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
 			      int *max_len, int flags);
+
+static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
+				      int *max_len)
+{
+	return exportfs_encode_inode_fh(inode, fid, max_len, NULL,
+					EXPORT_FH_FID);
+}
+
 extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
 					     struct fid *fid, int fh_len,
 					     int fileid_type,
-- 
2.34.1

[RFC][PATCH 3/4] exportfs: allow exporting non-decodeable file handles to userspace

[Amir Goldstein]

Some userspace programs use st_ino as a unique object identifier, even
though inode numbers may be recycable.

This issue has been addressed for NFS export long ago using the exportfs
file handle API and the unique file handle identifiers are also exported
to userspace via name_to_handle_at(2).

fanotify also uses file handles to identify objects in events, but only
for filesystems that support NFS export.

Relax the requirement for NFS export support and allow more filesystems
to export a unique object identifier via name_to_handle_at(2) with the
flag AT_HANDLE_FID.

A file handle requested with the AT_HANDLE_FID flag, may or may not be
usable as an argument to open_by_handle_at(2).

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

diff --git a/fs/fhandle.c b/fs/fhandle.c
index f2bc27d1975e..131c23ae77d8 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -16,7 +16,7 @@
 
 static long do_sys_name_to_handle(const struct path *path,
 				  struct file_handle __user *ufh,
-				  int __user *mnt_id)
+				  int __user *mnt_id, int fh_flags)
 {
 	long retval;
 	struct file_handle f_handle;
@@ -24,11 +24,12 @@ static long do_sys_name_to_handle(const struct path *path,
 	struct file_handle *handle = NULL;
 
 	/*
-	 * We need to make sure whether the file system
-	 * support decoding of the file handle
+	 * We need to make sure whether the file system support decoding of
+	 * the file handle if decodeable file handle was requested.
 	 */
 	if (!path->dentry->d_sb->s_export_op ||
-	    !path->dentry->d_sb->s_export_op->fh_to_dentry)
+	    (!(fh_flags & EXPORT_FH_FID) &&
+	     !path->dentry->d_sb->s_export_op->fh_to_dentry))
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&f_handle, ufh, sizeof(struct file_handle)))
@@ -45,10 +46,10 @@ 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 connected handle */
+	/* we ask for a non connectable maybe decodeable file handle */
 	retval = exportfs_encode_fh(path->dentry,
 				    (struct fid *)handle->f_handle,
-				    &handle_dwords,  0);
+				    &handle_dwords, fh_flags);
 	handle->handle_type = retval;
 	/* convert handle size to bytes */
 	handle_bytes = handle_dwords * sizeof(u32);
@@ -84,6 +85,7 @@ static long do_sys_name_to_handle(const struct path *path,
  * @handle: resulting file handle
  * @mnt_id: mount id of the file system containing the file
  * @flag: flag value to indicate whether to follow symlink or not
+ *        and whether a decodable file handle is required.
  *
  * @handle->handle_size indicate the space available to store the
  * variable part of the file handle in bytes. If there is not
@@ -96,17 +98,19 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 {
 	struct path path;
 	int lookup_flags;
+	int fh_flags;
 	int err;
 
-	if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
+	if (flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_FID))
 		return -EINVAL;
 
 	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);
 	if (!err) {
-		err = do_sys_name_to_handle(&path, handle, mnt_id);
+		err = do_sys_name_to_handle(&path, handle, mnt_id, fh_flags);
 		path_put(&path);
 	}
 	return err;
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index e8c07da58c9f..3091080db069 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -112,4 +112,9 @@
 
 #define AT_RECURSIVE		0x8000	/* Apply to the entire subtree */
 
+/* Flags for name_to_handle_at(2) */
+#define AT_HANDLE_FID		0x10000	/* file handle is needed to compare
+					   object indentity and may not be
+					   usable to open_by_handle_at(2) */
+
 #endif /* _UAPI_LINUX_FCNTL_H */
-- 
2.34.1

[RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Amir Goldstein]

fanotify users do not always need to decode the file handles reported
with FAN_REPORT_FID.

Relax the restriction that filesystem needs to support NFS export and
allow reporting file handles from filesystems that only support ecoding
unique file handles.

For such filesystems, users will have to use the AT_HANDLE_FID of
name_to_handle_at(2) if they want to compare the object in path to the
object fid reported in an event.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 4 ++--
 fs/notify/fanotify/fanotify_user.c | 6 ++----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index d1a49f5b6e6d..d2bbf1445a9e 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -380,7 +380,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
 	if (!inode)
 		return 0;
 
-	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL, 0);
+	exportfs_encode_fid(inode, NULL, &dwords);
 	fh_len = dwords << 2;
 
 	/*
@@ -443,7 +443,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	}
 
 	dwords = fh_len >> 2;
-	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL, 0);
+	type = exportfs_encode_fid(inode, buf, &dwords);
 	err = -EINVAL;
 	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
 		goto out_err;
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8f430bfad487..a5af84cbb30d 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
 	 * We need to make sure that the file system supports at least
 	 * encoding a file handle so user can use name_to_handle_at() to
 	 * compare fid returned with event to the file handle of watched
-	 * objects. However, name_to_handle_at() requires that the
-	 * filesystem also supports decoding file handles.
+	 * objects, but it does not need to support decoding file handles.
 	 */
-	if (!dentry->d_sb->s_export_op ||
-	    !dentry->d_sb->s_export_op->fh_to_dentry)
+	if (!dentry->d_sb->s_export_op)
 		return -EOPNOTSUPP;
 
 	return 0;
-- 
2.34.1

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Chuck Lever]

On Tue, Apr 25, 2023 at 04:01:01PM +0300, Amir Goldstein wrote:
> Jan,
> 
> Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> alternative proposal to seamlessly support more filesystems.
> 
> While fanotify relaxes the requirements for filesystems to support
> reporting fid to require only the ->encode_fh() operation, there are
> currently no new filesystems that meet the relaxed requirements.
> 
> I will shortly post patches that allow overlayfs to meet the new
> requirements with default overlay configurations.
> 
> The overlay and vfs/fanotify patch sets are completely independent.
> The are both available on my github branch [2] and there is a simple
> LTP test variant that tests reporting fid from overlayfs [3], which
> also demonstrates the minor UAPI change of name_to_handle_at(2) for
> requesting a non-decodeable file handle by userspace.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> 
> Amir Goldstein (4):
>   exportfs: change connectable argument to bit flags
>   exportfs: add explicit flag to request non-decodeable file handles
>   exportfs: allow exporting non-decodeable file handles to userspace
>   fanotify: support reporting non-decodeable file handles
> 
>  Documentation/filesystems/nfs/exporting.rst |  4 +--
>  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
>  fs/fhandle.c                                | 20 ++++++++------
>  fs/nfsd/nfsfh.c                             |  5 ++--
>  fs/notify/fanotify/fanotify.c               |  4 +--
>  fs/notify/fanotify/fanotify_user.c          |  6 ++---
>  fs/notify/fdinfo.c                          |  2 +-
>  include/linux/exportfs.h                    | 18 ++++++++++---
>  include/uapi/linux/fcntl.h                  |  5 ++++
>  9 files changed, 67 insertions(+), 26 deletions(-)

Hello Amir-

Note that the maintainers of exportfs are Jeff and myself. Please
copy us on future versions of this patch series. Thanks!

[cel@klimt even-releases]$ scripts/get_maintainer.pl fs/exportfs/
Chuck Lever <chuck.lever@oracle.com> (supporter:KERNEL NFSD, SUNRPC, AND LOCKD SERVERS)
Jeff Layton <jlayton@kernel.org> (supporter:KERNEL NFSD, SUNRPC, AND LOCKD SERVERS)
linux-nfs@vger.kernel.org (open list:KERNEL NFSD, SUNRPC, AND LOCKD SERVERS)
linux-kernel@vger.kernel.org (open list)

Re: [RFC][PATCH 1/4] exportfs: change connectable argument to bit flags

[Chuck Lever]

On Tue, Apr 25, 2023 at 04:01:02PM +0300, Amir Goldstein wrote:
> Convert the bool connectable arguemnt into a bit flags argument and
> define the EXPORT_FS_CONNECTABLE flag as a requested property of the
> file handle.
> 
> We are going to add a flag for requesting non-decodeable file handles.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/exportfs/expfs.c      | 11 +++++++++--
>  fs/nfsd/nfsfh.c          |  5 +++--
>  include/linux/exportfs.h |  6 ++++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index ab88d33d106c..bf1b4925fedd 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -393,14 +393,21 @@ int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  }
>  EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
>  
> +/**
> + * exportfs_encode_fh - encode a file handle from dentry
> + * @dentry:  the object to encode
> + * @fid:     where to store the file handle fragment
> + * @max_len: maximum length to store there
> + * @flags:   properties of the requrested file handle

Thanks for adding the KDOC comment!

/requrested/requested

And please add:

 *
 * Returns an enum fid_type or a negative errno

> + */
>  int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
> -		int connectable)
> +		       int flags)
>  {
>  	int error;
>  	struct dentry *p = NULL;
>  	struct inode *inode = dentry->d_inode, *parent = NULL;
>  
> -	if (connectable && !S_ISDIR(inode->i_mode)) {
> +	if ((flags & EXPORT_FH_CONNECTABLE) && !S_ISDIR(inode->i_mode)) {
>  		p = dget_parent(dentry);
>  		/*
>  		 * note that while p might've ceased to be our parent already,
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index ccd8485fee04..31e4505c0df3 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -414,10 +414,11 @@ static void _fh_update(struct svc_fh *fhp, struct svc_export *exp,
>  		struct fid *fid = (struct fid *)
>  			(fhp->fh_handle.fh_fsid + fhp->fh_handle.fh_size/4 - 1);
>  		int maxsize = (fhp->fh_maxsize - fhp->fh_handle.fh_size)/4;
> -		int subtreecheck = !(exp->ex_flags & NFSEXP_NOSUBTREECHECK);
> +		int fh_flags = (exp->ex_flags & NFSEXP_NOSUBTREECHECK) ? 0 :
> +				EXPORT_FH_CONNECTABLE;
>  
>  		fhp->fh_handle.fh_fileid_type =
> -			exportfs_encode_fh(dentry, fid, &maxsize, subtreecheck);
> +			exportfs_encode_fh(dentry, fid, &maxsize, fh_flags);
>  		fhp->fh_handle.fh_size += maxsize * 4;
>  	} else {
>  		fhp->fh_handle.fh_fileid_type = FILEID_ROOT;
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 601700fedc91..2b1048238170 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -135,6 +135,8 @@ struct fid {
>  	};
>  };
>  
> +#define EXPORT_FH_CONNECTABLE	0x1
> +
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
>   * @encode_fh:      encode a file handle fragment from a dentry
> @@ -150,7 +152,7 @@ struct fid {
>   * encode_fh:
>   *    @encode_fh should store in the file handle fragment @fh (using at most
>   *    @max_len bytes) information that can be used by @decode_fh to recover the
> - *    file referred to by the &struct dentry @de.  If the @connectable flag is
> + *    file referred to by the &struct dentry @de.  If @flag has CONNECTABLE bit
>   *    set, the encode_fh() should store sufficient information so that a good
>   *    attempt can be made to find not only the file but also it's place in the
>   *    filesystem.   This typically means storing a reference to de->d_parent in
> @@ -226,7 +228,7 @@ struct export_operations {
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
>  				    int *max_len, struct inode *parent);
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
> -	int *max_len, int connectable);
> +			      int *max_len, int flags);
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>  					     struct fid *fid, int fh_len,
>  					     int fileid_type,
> -- 
> 2.34.1
> 

Re: [RFC][PATCH 2/4] exportfs: add explicit flag to request non-decodeable file handles

[Chuck Lever]

On Tue, Apr 25, 2023 at 04:01:03PM +0300, Amir Goldstein wrote:
> So far, all callers of exportfs_encode_inode_fh(), except for fsnotify's
> show_mark_fhandle(), check that filesystem can decode file handles, but
> we would like to add more callers that do not require a file handle that
> can be decoded.
> 
> Introduce a flag to explicitly request a file handle that may not to be
> decoded later and a wrapper exportfs_encode_fid() that sets this flag
> and convert show_mark_fhandle() to use the new wrapper.
> 
> This will be used to allow adding fanotify support to filesystems that
> do not support NFS export.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/nfs/exporting.rst |  4 ++--
>  fs/exportfs/expfs.c                         | 18 ++++++++++++++++--
>  fs/notify/fanotify/fanotify.c               |  4 ++--
>  fs/notify/fdinfo.c                          |  2 +-
>  include/linux/exportfs.h                    | 12 +++++++++++-
>  5 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
> index 0e98edd353b5..3d97b8d8f735 100644
> --- a/Documentation/filesystems/nfs/exporting.rst
> +++ b/Documentation/filesystems/nfs/exporting.rst
> @@ -122,8 +122,8 @@ are exportable by setting the s_export_op field in the struct
>  super_block.  This field must point to a "struct export_operations"
>  struct which has the following members:
>  
> - encode_fh  (optional)
> -    Takes a dentry and creates a filehandle fragment which can later be used
> +  encode_fh (optional)
> +    Takes a dentry and creates a filehandle fragment which may later be used
>      to find or create a dentry for the same object.  The default
>      implementation creates a filehandle fragment that encodes a 32bit inode
>      and generation number for the inode encoded, and if necessary the
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index bf1b4925fedd..1b35dda5bdda 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -381,11 +381,25 @@ static int export_encode_fh(struct inode *inode, struct fid *fid,
>  	return type;
>  }
>  
> +/**
> + * exportfs_encode_inode_fh - encode a file handle from inode
> + * @inode:   the object to encode
> + * @fid:     where to store the file handle fragment
> + * @max_len: maximum length to store there
> + * @flags:   properties of the requrested file handle
> + */

Same comment here: /requrested/requested and add a " * Returns an ..."

>  int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> -			     int *max_len, struct inode *parent)
> +			     int *max_len, struct inode *parent, int flags)
>  {
>  	const struct export_operations *nop = inode->i_sb->s_export_op;
>  
> +	/*
> +	 * If a decodeable file handle was requested, we need to make sure that
> +	 * filesystem can decode file handles.
> +	 */
> +	if (nop && !(flags & EXPORT_FH_FID) && !nop->fh_to_dentry)
> +		return -EOPNOTSUPP;
> +
>  	if (nop && nop->encode_fh)
>  		return nop->encode_fh(inode, fid->raw, max_len, parent);
>  
> @@ -416,7 +430,7 @@ int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
>  		parent = p->d_inode;
>  	}
>  
> -	error = exportfs_encode_inode_fh(inode, fid, max_len, parent);
> +	error = exportfs_encode_inode_fh(inode, fid, max_len, parent, flags);
>  	dput(p);
>  
>  	return error;
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 29bdd99b29fa..d1a49f5b6e6d 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -380,7 +380,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
>  	if (!inode)
>  		return 0;
>  
> -	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
> +	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL, 0);
>  	fh_len = dwords << 2;
>  
>  	/*
> @@ -443,7 +443,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>  	}
>  
>  	dwords = fh_len >> 2;
> -	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
> +	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL, 0);
>  	err = -EINVAL;
>  	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
>  		goto out_err;
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index 55081ae3a6ec..5c430736ec12 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -50,7 +50,7 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>  	f.handle.handle_bytes = sizeof(f.pad);
>  	size = f.handle.handle_bytes >> 2;
>  
> -	ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
> +	ret = exportfs_encode_fid(inode, (struct fid *)f.handle.f_handle, &size);
>  	if ((ret == FILEID_INVALID) || (ret < 0)) {
>  		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
>  		return;
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 2b1048238170..635e89e1dae7 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -136,6 +136,7 @@ struct fid {
>  };
>  
>  #define EXPORT_FH_CONNECTABLE	0x1
> +#define EXPORT_FH_FID		0x2
>  
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
> @@ -226,9 +227,18 @@ struct export_operations {
>  };
>  
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> -				    int *max_len, struct inode *parent);
> +				    int *max_len, struct inode *parent,
> +				    int flags);
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
>  			      int *max_len, int flags);
> +
> +static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
> +				      int *max_len)
> +{
> +	return exportfs_encode_inode_fh(inode, fid, max_len, NULL,
> +					EXPORT_FH_FID);
> +}
> +
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>  					     struct fid *fid, int fh_len,
>  					     int fileid_type,
> -- 
> 2.34.1
> 

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

On Wed, Apr 26, 2023 at 4:47 PM Chuck Lever <cel@kernel.org> wrote:
>
> On Tue, Apr 25, 2023 at 04:01:01PM +0300, Amir Goldstein wrote:
> > Jan,
> >
> > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > alternative proposal to seamlessly support more filesystems.
> >
> > While fanotify relaxes the requirements for filesystems to support
> > reporting fid to require only the ->encode_fh() operation, there are
> > currently no new filesystems that meet the relaxed requirements.
> >
> > I will shortly post patches that allow overlayfs to meet the new
> > requirements with default overlay configurations.
> >
> > The overlay and vfs/fanotify patch sets are completely independent.
> > The are both available on my github branch [2] and there is a simple
> > LTP test variant that tests reporting fid from overlayfs [3], which
> > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > requesting a non-decodeable file handle by userspace.
> >
> > Thanks,
> > Amir.
> >
> > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> >
> > Amir Goldstein (4):
> >   exportfs: change connectable argument to bit flags
> >   exportfs: add explicit flag to request non-decodeable file handles
> >   exportfs: allow exporting non-decodeable file handles to userspace
> >   fanotify: support reporting non-decodeable file handles
> >
> >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> >  fs/fhandle.c                                | 20 ++++++++------
> >  fs/nfsd/nfsfh.c                             |  5 ++--
> >  fs/notify/fanotify/fanotify.c               |  4 +--
> >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> >  fs/notify/fdinfo.c                          |  2 +-
> >  include/linux/exportfs.h                    | 18 ++++++++++---
> >  include/uapi/linux/fcntl.h                  |  5 ++++
> >  9 files changed, 67 insertions(+), 26 deletions(-)
>
> Hello Amir-
>
> Note that the maintainers of exportfs are Jeff and myself. Please
> copy us on future versions of this patch series. Thanks!

Sorry. Will do.
Thanks for the review!

Amir.

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Jan Kara]

On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> fanotify users do not always need to decode the file handles reported
> with FAN_REPORT_FID.
> 
> Relax the restriction that filesystem needs to support NFS export and
> allow reporting file handles from filesystems that only support ecoding
> unique file handles.
> 
> For such filesystems, users will have to use the AT_HANDLE_FID of
> name_to_handle_at(2) if they want to compare the object in path to the
> object fid reported in an event.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
...
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 8f430bfad487..a5af84cbb30d 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
>  	 * We need to make sure that the file system supports at least
>  	 * encoding a file handle so user can use name_to_handle_at() to
>  	 * compare fid returned with event to the file handle of watched
> -	 * objects. However, name_to_handle_at() requires that the
> -	 * filesystem also supports decoding file handles.
> +	 * objects, but it does not need to support decoding file handles.
>  	 */
> -	if (!dentry->d_sb->s_export_op ||
> -	    !dentry->d_sb->s_export_op->fh_to_dentry)
> +	if (!dentry->d_sb->s_export_op)
>  		return -EOPNOTSUPP;

So AFAICS the only thing you require is that s_export_op is set to
*something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
just fine without any filesystem cooperation. What is the reasoning behind
the dentry->d_sb->s_export_op check? Is there an implicit expectation that
if s_export_op is set to something, the filesystem has sensible
i_generation? Or is it just a caution that you don't want the functionality
to be enabled for unexpected filesystems? In either case it would be good
to capture the reasoning either in a comment or the changelog...

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

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Amir Goldstein]

s_export_op

On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> > fanotify users do not always need to decode the file handles reported
> > with FAN_REPORT_FID.
> >
> > Relax the restriction that filesystem needs to support NFS export and
> > allow reporting file handles from filesystems that only support ecoding
> > unique file handles.
> >
> > For such filesystems, users will have to use the AT_HANDLE_FID of
> > name_to_handle_at(2) if they want to compare the object in path to the
> > object fid reported in an event.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ...
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 8f430bfad487..a5af84cbb30d 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
> >        * We need to make sure that the file system supports at least
> >        * encoding a file handle so user can use name_to_handle_at() to
> >        * compare fid returned with event to the file handle of watched
> > -      * objects. However, name_to_handle_at() requires that the
> > -      * filesystem also supports decoding file handles.
> > +      * objects, but it does not need to support decoding file handles.
> >        */
> > -     if (!dentry->d_sb->s_export_op ||
> > -         !dentry->d_sb->s_export_op->fh_to_dentry)
> > +     if (!dentry->d_sb->s_export_op)
> >               return -EOPNOTSUPP;
>
> So AFAICS the only thing you require is that s_export_op is set to
> *something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
> just fine without any filesystem cooperation. What is the reasoning behind
> the dentry->d_sb->s_export_op check? Is there an implicit expectation that
> if s_export_op is set to something, the filesystem has sensible
> i_generation? Or is it just a caution that you don't want the functionality
> to be enabled for unexpected filesystems?

A little bit of both.
Essentially, I do not want to use the generic encoding unless the filesystem
opted-in to say "This is how objects should be identified".

The current fs that have s_export_op && !s_export_op->encode_fh
practically make that statement because they support NFS export
(i.e. they implement fh_to_dentry()).

I don't like the implicit fallback to generic encoding, especially when
introducing this new functionality of encode_fid().

Before posting this patch set I had two earlier revisions.
One that changed the encode_fh() to mandatory and converted
all the INO32_GEN fs to explicitly set
s_export_op.encode_fh = generic_encode_ino32_fh,
And one that marked all the INO32_GEN fs with
s_export_op.flags = EXPORT_OP_ENCODE_INO32_GEN
in both cases there was no blind fallback to INO32_GEN.

But in the end, these added noise without actual value so
I dropped them, because the d_sb->s_export_op check is anyway
a pretty strong indication for opt-in to export fids.

CC exportfs maintainers in case they have an opinion one
way or the other.

> In either case it would be good
> to capture the reasoning either in a comment or the changelog...
>

Will do.

Thanks,
Amir.

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Amir Goldstein]

On Thu, Apr 27, 2023 at 3:28 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> s_export_op
>
> On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> > > fanotify users do not always need to decode the file handles reported
> > > with FAN_REPORT_FID.
> > >
> > > Relax the restriction that filesystem needs to support NFS export and
> > > allow reporting file handles from filesystems that only support ecoding
> > > unique file handles.
> > >
> > > For such filesystems, users will have to use the AT_HANDLE_FID of
> > > name_to_handle_at(2) if they want to compare the object in path to the
> > > object fid reported in an event.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ...
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index 8f430bfad487..a5af84cbb30d 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
> > >        * We need to make sure that the file system supports at least
> > >        * encoding a file handle so user can use name_to_handle_at() to
> > >        * compare fid returned with event to the file handle of watched
> > > -      * objects. However, name_to_handle_at() requires that the
> > > -      * filesystem also supports decoding file handles.
> > > +      * objects, but it does not need to support decoding file handles.
> > >        */
> > > -     if (!dentry->d_sb->s_export_op ||
> > > -         !dentry->d_sb->s_export_op->fh_to_dentry)
> > > +     if (!dentry->d_sb->s_export_op)
> > >               return -EOPNOTSUPP;
> >
> > So AFAICS the only thing you require is that s_export_op is set to
> > *something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
> > just fine without any filesystem cooperation. What is the reasoning behind
> > the dentry->d_sb->s_export_op check? Is there an implicit expectation that
> > if s_export_op is set to something, the filesystem has sensible
> > i_generation? Or is it just a caution that you don't want the functionality
> > to be enabled for unexpected filesystems?
>
> A little bit of both.
> Essentially, I do not want to use the generic encoding unless the filesystem
> opted-in to say "This is how objects should be identified".
>
> The current fs that have s_export_op && !s_export_op->encode_fh
> practically make that statement because they support NFS export
> (i.e. they implement fh_to_dentry()).
>
> I don't like the implicit fallback to generic encoding, especially when
> introducing this new functionality of encode_fid().
>
> Before posting this patch set I had two earlier revisions.
> One that changed the encode_fh() to mandatory and converted
> all the INO32_GEN fs to explicitly set
> s_export_op.encode_fh = generic_encode_ino32_fh,
> And one that marked all the INO32_GEN fs with
> s_export_op.flags = EXPORT_OP_ENCODE_INO32_GEN
> in both cases there was no blind fallback to INO32_GEN.
>
> But in the end, these added noise without actual value so
> I dropped them, because the d_sb->s_export_op check is anyway
> a pretty strong indication for opt-in to export fids.
>
> CC exportfs maintainers in case they have an opinion one
> way or the other.
>

BTW, the other reason I chose this requirement is circular -
this is the same requirement for exporting fids to user with
AT_HANDLE_FID and the two FAN_REPORT_FID should be
aligned with users ability to get fid with name_to_handle_at().

Another reasonable requirement would have been:
* !AT_HANDLE_FID requires ->fh_to_dentry (as current code)
* AT_HANDLE_FID requires either ->fh_to_dentry or encode_fh

Thanks,
Amir.

Re: [RFC][PATCH 2/4] exportfs: add explicit flag to request non-decodeable file handles

[Jeff Layton]

On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> So far, all callers of exportfs_encode_inode_fh(), except for fsnotify's
> show_mark_fhandle(), check that filesystem can decode file handles, but
> we would like to add more callers that do not require a file handle that
> can be decoded.
> 
> Introduce a flag to explicitly request a file handle that may not to be
> decoded later and a wrapper exportfs_encode_fid() that sets this flag
> and convert show_mark_fhandle() to use the new wrapper.
> 
> This will be used to allow adding fanotify support to filesystems that
> do not support NFS export.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  Documentation/filesystems/nfs/exporting.rst |  4 ++--
>  fs/exportfs/expfs.c                         | 18 ++++++++++++++++--
>  fs/notify/fanotify/fanotify.c               |  4 ++--
>  fs/notify/fdinfo.c                          |  2 +-
>  include/linux/exportfs.h                    | 12 +++++++++++-
>  5 files changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
> index 0e98edd353b5..3d97b8d8f735 100644
> --- a/Documentation/filesystems/nfs/exporting.rst
> +++ b/Documentation/filesystems/nfs/exporting.rst
> @@ -122,8 +122,8 @@ are exportable by setting the s_export_op field in the struct
>  super_block.  This field must point to a "struct export_operations"
>  struct which has the following members:
>  
> - encode_fh  (optional)
> -    Takes a dentry and creates a filehandle fragment which can later be used
> +  encode_fh (optional)
> +    Takes a dentry and creates a filehandle fragment which may later be used
>      to find or create a dentry for the same object.  The default
>      implementation creates a filehandle fragment that encodes a 32bit inode
>      and generation number for the inode encoded, and if necessary the
> diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> index bf1b4925fedd..1b35dda5bdda 100644
> --- a/fs/exportfs/expfs.c
> +++ b/fs/exportfs/expfs.c
> @@ -381,11 +381,25 @@ static int export_encode_fh(struct inode *inode, struct fid *fid,
>  	return type;
>  }
>  
> +/**
> + * exportfs_encode_inode_fh - encode a file handle from inode
> + * @inode:   the object to encode
> + * @fid:     where to store the file handle fragment
> + * @max_len: maximum length to store there
> + * @flags:   properties of the requrested file handle
> + */
>  int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> -			     int *max_len, struct inode *parent)
> +			     int *max_len, struct inode *parent, int flags)
>  {
>  	const struct export_operations *nop = inode->i_sb->s_export_op;
>  
> +	/*
> +	 * If a decodeable file handle was requested, we need to make sure that
> +	 * filesystem can decode file handles.
> +	 */
> +	if (nop && !(flags & EXPORT_FH_FID) && !nop->fh_to_dentry)
> +		return -EOPNOTSUPP;
> +

If you're moving this check into this function, then it might be good to
remove the same check from the callers that are doing this check now.

>  	if (nop && nop->encode_fh)
>  		return nop->encode_fh(inode, fid->raw, max_len, parent);
>  
> @@ -416,7 +430,7 @@ int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
>  		parent = p->d_inode;
>  	}
>  
> -	error = exportfs_encode_inode_fh(inode, fid, max_len, parent);
> +	error = exportfs_encode_inode_fh(inode, fid, max_len, parent, flags);
>  	dput(p);
>  
>  	return error;
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 29bdd99b29fa..d1a49f5b6e6d 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -380,7 +380,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
>  	if (!inode)
>  		return 0;
>  
> -	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
> +	exportfs_encode_inode_fh(inode, NULL, &dwords, NULL, 0);
>  	fh_len = dwords << 2;
>  
>  	/*
> @@ -443,7 +443,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
>  	}
>  
>  	dwords = fh_len >> 2;
> -	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
> +	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL, 0);
>  	err = -EINVAL;
>  	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
>  		goto out_err;
> diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> index 55081ae3a6ec..5c430736ec12 100644
> --- a/fs/notify/fdinfo.c
> +++ b/fs/notify/fdinfo.c
> @@ -50,7 +50,7 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
>  	f.handle.handle_bytes = sizeof(f.pad);
>  	size = f.handle.handle_bytes >> 2;
>  
> -	ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
> +	ret = exportfs_encode_fid(inode, (struct fid *)f.handle.f_handle, &size);
>  	if ((ret == FILEID_INVALID) || (ret < 0)) {
>  		WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
>  		return;
> diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> index 2b1048238170..635e89e1dae7 100644
> --- a/include/linux/exportfs.h
> +++ b/include/linux/exportfs.h
> @@ -136,6 +136,7 @@ struct fid {
>  };
>  
>  #define EXPORT_FH_CONNECTABLE	0x1
> +#define EXPORT_FH_FID		0x2

Please add comments about what these flags are intended to indicate.

>  
>  /**
>   * struct export_operations - for nfsd to communicate with file systems
> @@ -226,9 +227,18 @@ struct export_operations {
>  };
>  
>  extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> -				    int *max_len, struct inode *parent);
> +				    int *max_len, struct inode *parent,
> +				    int flags);
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
>  			      int *max_len, int flags);
> +
> +static inline int exportfs_encode_fid(struct inode *inode, struct fid *fid,
> +				      int *max_len)
> +{
> +	return exportfs_encode_inode_fh(inode, fid, max_len, NULL,
> +					EXPORT_FH_FID);
> +}
> +
>  extern struct dentry *exportfs_decode_fh_raw(struct vfsmount *mnt,
>  					     struct fid *fid, int fh_len,
>  					     int fileid_type,

-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Jeff Layton]

On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> Jan,
> 
> Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> alternative proposal to seamlessly support more filesystems.
> 
> While fanotify relaxes the requirements for filesystems to support
> reporting fid to require only the ->encode_fh() operation, there are
> currently no new filesystems that meet the relaxed requirements.
> 
> I will shortly post patches that allow overlayfs to meet the new
> requirements with default overlay configurations.
> 
> The overlay and vfs/fanotify patch sets are completely independent.
> The are both available on my github branch [2] and there is a simple
> LTP test variant that tests reporting fid from overlayfs [3], which
> also demonstrates the minor UAPI change of name_to_handle_at(2) for
> requesting a non-decodeable file handle by userspace.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> 
> Amir Goldstein (4):
>   exportfs: change connectable argument to bit flags
>   exportfs: add explicit flag to request non-decodeable file handles
>   exportfs: allow exporting non-decodeable file handles to userspace
>   fanotify: support reporting non-decodeable file handles
> 
>  Documentation/filesystems/nfs/exporting.rst |  4 +--
>  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
>  fs/fhandle.c                                | 20 ++++++++------
>  fs/nfsd/nfsfh.c                             |  5 ++--
>  fs/notify/fanotify/fanotify.c               |  4 +--
>  fs/notify/fanotify/fanotify_user.c          |  6 ++---
>  fs/notify/fdinfo.c                          |  2 +-
>  include/linux/exportfs.h                    | 18 ++++++++++---
>  include/uapi/linux/fcntl.h                  |  5 ++++
>  9 files changed, 67 insertions(+), 26 deletions(-)
> 

This set looks fairly benign to me, so ACK on the general concept.

I am starting to dislike how the AT_* flags are turning into a bunch of
flags that only have meanings on certain syscalls. I don't see a cleaner
way to handle it though.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC][PATCH 2/4] exportfs: add explicit flag to request non-decodeable file handles

[Amir Goldstein]

On Thu, Apr 27, 2023 at 6:00 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > So far, all callers of exportfs_encode_inode_fh(), except for fsnotify's
> > show_mark_fhandle(), check that filesystem can decode file handles, but
> > we would like to add more callers that do not require a file handle that
> > can be decoded.
> >
> > Introduce a flag to explicitly request a file handle that may not to be
> > decoded later and a wrapper exportfs_encode_fid() that sets this flag
> > and convert show_mark_fhandle() to use the new wrapper.
> >
> > This will be used to allow adding fanotify support to filesystems that
> > do not support NFS export.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  Documentation/filesystems/nfs/exporting.rst |  4 ++--
> >  fs/exportfs/expfs.c                         | 18 ++++++++++++++++--
> >  fs/notify/fanotify/fanotify.c               |  4 ++--
> >  fs/notify/fdinfo.c                          |  2 +-
> >  include/linux/exportfs.h                    | 12 +++++++++++-
> >  5 files changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
> > index 0e98edd353b5..3d97b8d8f735 100644
> > --- a/Documentation/filesystems/nfs/exporting.rst
> > +++ b/Documentation/filesystems/nfs/exporting.rst
> > @@ -122,8 +122,8 @@ are exportable by setting the s_export_op field in the struct
> >  super_block.  This field must point to a "struct export_operations"
> >  struct which has the following members:
> >
> > - encode_fh  (optional)
> > -    Takes a dentry and creates a filehandle fragment which can later be used
> > +  encode_fh (optional)
> > +    Takes a dentry and creates a filehandle fragment which may later be used
> >      to find or create a dentry for the same object.  The default
> >      implementation creates a filehandle fragment that encodes a 32bit inode
> >      and generation number for the inode encoded, and if necessary the
> > diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c
> > index bf1b4925fedd..1b35dda5bdda 100644
> > --- a/fs/exportfs/expfs.c
> > +++ b/fs/exportfs/expfs.c
> > @@ -381,11 +381,25 @@ static int export_encode_fh(struct inode *inode, struct fid *fid,
> >       return type;
> >  }
> >
> > +/**
> > + * exportfs_encode_inode_fh - encode a file handle from inode
> > + * @inode:   the object to encode
> > + * @fid:     where to store the file handle fragment
> > + * @max_len: maximum length to store there
> > + * @flags:   properties of the requrested file handle
> > + */
> >  int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
> > -                          int *max_len, struct inode *parent)
> > +                          int *max_len, struct inode *parent, int flags)
> >  {
> >       const struct export_operations *nop = inode->i_sb->s_export_op;
> >
> > +     /*
> > +      * If a decodeable file handle was requested, we need to make sure that
> > +      * filesystem can decode file handles.
> > +      */
> > +     if (nop && !(flags & EXPORT_FH_FID) && !nop->fh_to_dentry)
> > +             return -EOPNOTSUPP;
> > +
>
> If you're moving this check into this function, then it might be good to
> remove the same check from the callers that are doing this check now.
>

There are three types of callers:
1. nfsd and fanotify check nop->fh_to_dentry at export/setup time
    so I cannot remove those checks
2. in do_sys_name_to_handle() I could have removed the duplicate
    check but then -EFAULT/-EINVAL could be returned instead of
    -EOPNOTSUPP and I did not want to make that visible API change
3. show_mark_fhandle() does not check anything (*)

(*) The reason that show_mark_fhandle() exists originally is to aid CRIU
to restore inotify/fanotify watches on container restore.
Since CRIU cannot really do that without decoding file handles
it is somewhat questionable that show_mark_fhandle() does not check for
->fh_to_dentry and it looks like an oversight, but it has been like that for
too long to change this behavior IMO.

> >       if (nop && nop->encode_fh)
> >               return nop->encode_fh(inode, fid->raw, max_len, parent);
> >
> > @@ -416,7 +430,7 @@ int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
> >               parent = p->d_inode;
> >       }
> >
> > -     error = exportfs_encode_inode_fh(inode, fid, max_len, parent);
> > +     error = exportfs_encode_inode_fh(inode, fid, max_len, parent, flags);
> >       dput(p);
> >
> >       return error;
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 29bdd99b29fa..d1a49f5b6e6d 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -380,7 +380,7 @@ static int fanotify_encode_fh_len(struct inode *inode)
> >       if (!inode)
> >               return 0;
> >
> > -     exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
> > +     exportfs_encode_inode_fh(inode, NULL, &dwords, NULL, 0);
> >       fh_len = dwords << 2;
> >
> >       /*
> > @@ -443,7 +443,7 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
> >       }
> >
> >       dwords = fh_len >> 2;
> > -     type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
> > +     type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL, 0);
> >       err = -EINVAL;
> >       if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
> >               goto out_err;
> > diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
> > index 55081ae3a6ec..5c430736ec12 100644
> > --- a/fs/notify/fdinfo.c
> > +++ b/fs/notify/fdinfo.c
> > @@ -50,7 +50,7 @@ static void show_mark_fhandle(struct seq_file *m, struct inode *inode)
> >       f.handle.handle_bytes = sizeof(f.pad);
> >       size = f.handle.handle_bytes >> 2;
> >
> > -     ret = exportfs_encode_inode_fh(inode, (struct fid *)f.handle.f_handle, &size, NULL);
> > +     ret = exportfs_encode_fid(inode, (struct fid *)f.handle.f_handle, &size);
> >       if ((ret == FILEID_INVALID) || (ret < 0)) {
> >               WARN_ONCE(1, "Can't encode file handler for inotify: %d\n", ret);
> >               return;
> > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
> > index 2b1048238170..635e89e1dae7 100644
> > --- a/include/linux/exportfs.h
> > +++ b/include/linux/exportfs.h
> > @@ -136,6 +136,7 @@ struct fid {
> >  };
> >
> >  #define EXPORT_FH_CONNECTABLE        0x1
> > +#define EXPORT_FH_FID                0x2
>
> Please add comments about what these flags are intended to indicate.
>

OK.

Thanks,
Amir.

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

On Thu, Apr 27, 2023 at 6:13 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > Jan,
> >
> > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > alternative proposal to seamlessly support more filesystems.
> >
> > While fanotify relaxes the requirements for filesystems to support
> > reporting fid to require only the ->encode_fh() operation, there are
> > currently no new filesystems that meet the relaxed requirements.
> >
> > I will shortly post patches that allow overlayfs to meet the new
> > requirements with default overlay configurations.
> >
> > The overlay and vfs/fanotify patch sets are completely independent.
> > The are both available on my github branch [2] and there is a simple
> > LTP test variant that tests reporting fid from overlayfs [3], which
> > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > requesting a non-decodeable file handle by userspace.
> >
> > Thanks,
> > Amir.
> >
> > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> >
> > Amir Goldstein (4):
> >   exportfs: change connectable argument to bit flags
> >   exportfs: add explicit flag to request non-decodeable file handles
> >   exportfs: allow exporting non-decodeable file handles to userspace
> >   fanotify: support reporting non-decodeable file handles
> >
> >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> >  fs/fhandle.c                                | 20 ++++++++------
> >  fs/nfsd/nfsfh.c                             |  5 ++--
> >  fs/notify/fanotify/fanotify.c               |  4 +--
> >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> >  fs/notify/fdinfo.c                          |  2 +-
> >  include/linux/exportfs.h                    | 18 ++++++++++---
> >  include/uapi/linux/fcntl.h                  |  5 ++++
> >  9 files changed, 67 insertions(+), 26 deletions(-)
> >
>
> This set looks fairly benign to me, so ACK on the general concept.

Thanks!

>
> I am starting to dislike how the AT_* flags are turning into a bunch of
> flags that only have meanings on certain syscalls. I don't see a cleaner
> way to handle it though.

Yeh, it's not great.

There is also a way to extend the existing API with:

Perhstruct file_handle {
        unsigned int handle_bytes:8;
        unsigned int handle_flags:24;
        int handle_type;
        unsigned char f_handle[];
};

AFAICT, this is guaranteed to be backward compat
with old kernels and old applications.

It also may not be a bad idea that the handle_flags could
be used to request specific fh properties (FID) and can also
describe the properties of the returned fh (i.e. non-decodeable)
that could also be respected by open_by_handle_at().

For backward compact, kernel will only set handle_flags in
response if new flags were set in the request.

Do you consider this extension better than AT_HANDLE_FID
or worse? At least it is an API change that is contained within the
exportfs subsystem, without polluting the AT_ flags global namespace.

Thanks,
Amir.

Re: [RFC][PATCH 4/4] fanotify: support reporting non-decodeable file handles

[Jan Kara]

On Thu 27-04-23 15:28:23, Amir Goldstein wrote:
> s_export_op
> 
> On Thu, Apr 27, 2023 at 2:48 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 25-04-23 16:01:05, Amir Goldstein wrote:
> > > fanotify users do not always need to decode the file handles reported
> > > with FAN_REPORT_FID.
> > >
> > > Relax the restriction that filesystem needs to support NFS export and
> > > allow reporting file handles from filesystems that only support ecoding
> > > unique file handles.
> > >
> > > For such filesystems, users will have to use the AT_HANDLE_FID of
> > > name_to_handle_at(2) if they want to compare the object in path to the
> > > object fid reported in an event.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ...
> > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > > index 8f430bfad487..a5af84cbb30d 100644
> > > --- a/fs/notify/fanotify/fanotify_user.c
> > > +++ b/fs/notify/fanotify/fanotify_user.c
> > > @@ -1586,11 +1586,9 @@ static int fanotify_test_fid(struct dentry *dentry)
> > >        * We need to make sure that the file system supports at least
> > >        * encoding a file handle so user can use name_to_handle_at() to
> > >        * compare fid returned with event to the file handle of watched
> > > -      * objects. However, name_to_handle_at() requires that the
> > > -      * filesystem also supports decoding file handles.
> > > +      * objects, but it does not need to support decoding file handles.
> > >        */
> > > -     if (!dentry->d_sb->s_export_op ||
> > > -         !dentry->d_sb->s_export_op->fh_to_dentry)
> > > +     if (!dentry->d_sb->s_export_op)
> > >               return -EOPNOTSUPP;
> >
> > So AFAICS the only thing you require is that s_export_op is set to
> > *something* as exportfs_encode_inode_fh() can deal with NULL ->encode_fh
> > just fine without any filesystem cooperation. What is the reasoning behind
> > the dentry->d_sb->s_export_op check? Is there an implicit expectation that
> > if s_export_op is set to something, the filesystem has sensible
> > i_generation? Or is it just a caution that you don't want the functionality
> > to be enabled for unexpected filesystems?
> 
> A little bit of both.
> Essentially, I do not want to use the generic encoding unless the filesystem
> opted-in to say "This is how objects should be identified".
> 
> The current fs that have s_export_op && !s_export_op->encode_fh
> practically make that statement because they support NFS export
> (i.e. they implement fh_to_dentry()).

Makes sense.

> I don't like the implicit fallback to generic encoding, especially when
> introducing this new functionality of encode_fid().
> 
> Before posting this patch set I had two earlier revisions.
> One that changed the encode_fh() to mandatory and converted
> all the INO32_GEN fs to explicitly set
> s_export_op.encode_fh = generic_encode_ino32_fh,
> And one that marked all the INO32_GEN fs with
> s_export_op.flags = EXPORT_OP_ENCODE_INO32_GEN
> in both cases there was no blind fallback to INO32_GEN.
> 
> But in the end, these added noise without actual value so
> I dropped them, because the d_sb->s_export_op check is anyway
> a pretty strong indication for opt-in to export fids.
> 
> CC exportfs maintainers in case they have an opinion one
> way or the other.

Yeah, I agree with your judgement. I just wanted to make sure I understand
everything properly.

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

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Jeff Layton]

On Thu, 2023-04-27 at 18:52 +0300, Amir Goldstein wrote:
> On Thu, Apr 27, 2023 at 6:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > > Jan,
> > > 
> > > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > > alternative proposal to seamlessly support more filesystems.
> > > 
> > > While fanotify relaxes the requirements for filesystems to support
> > > reporting fid to require only the ->encode_fh() operation, there are
> > > currently no new filesystems that meet the relaxed requirements.
> > > 
> > > I will shortly post patches that allow overlayfs to meet the new
> > > requirements with default overlay configurations.
> > > 
> > > The overlay and vfs/fanotify patch sets are completely independent.
> > > The are both available on my github branch [2] and there is a simple
> > > LTP test variant that tests reporting fid from overlayfs [3], which
> > > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > > requesting a non-decodeable file handle by userspace.
> > > 
> > > Thanks,
> > > Amir.
> > > 
> > > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> > > 
> > > Amir Goldstein (4):
> > >   exportfs: change connectable argument to bit flags
> > >   exportfs: add explicit flag to request non-decodeable file handles
> > >   exportfs: allow exporting non-decodeable file handles to userspace
> > >   fanotify: support reporting non-decodeable file handles
> > > 
> > >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> > >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> > >  fs/fhandle.c                                | 20 ++++++++------
> > >  fs/nfsd/nfsfh.c                             |  5 ++--
> > >  fs/notify/fanotify/fanotify.c               |  4 +--
> > >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> > >  fs/notify/fdinfo.c                          |  2 +-
> > >  include/linux/exportfs.h                    | 18 ++++++++++---
> > >  include/uapi/linux/fcntl.h                  |  5 ++++
> > >  9 files changed, 67 insertions(+), 26 deletions(-)
> > > 
> > 
> > This set looks fairly benign to me, so ACK on the general concept.
> 
> Thanks!
> 
> > 
> > I am starting to dislike how the AT_* flags are turning into a bunch of
> > flags that only have meanings on certain syscalls. I don't see a cleaner
> > way to handle it though.
> 
> Yeh, it's not great.
> 
> There is also a way to extend the existing API with:
> 
> Perhstruct file_handle {
>         unsigned int handle_bytes:8;
>         unsigned int handle_flags:24;
>         int handle_type;
>         unsigned char f_handle[];
> };
> 
> AFAICT, this is guaranteed to be backward compat
> with old kernels and old applications.
> 

That could work. It would probably look cleaner as a union though.
Something like this maybe?

union {
	unsigned int legacy_handle_bytes;
	struct {
		u8	handle_bytes;
		u8	__reserved;
		u16	handle_flags;
	};
}

__reserved must be zeroed (for now). You could consider using it for
some other purpose later.

It's a little ugly as an API but it would be backward compatible, given
that we never use the high bits today anyway.

Callers might need to deal with an -EINVAL when they try to pass non-
zero handle_flags to existing kernels, since you'd trip the
MAX_HANDLE_SZ check that's there today.

> It also may not be a bad idea that the handle_flags could
> be used to request specific fh properties (FID) and can also
> describe the properties of the returned fh (i.e. non-decodeable)
> that could also be respected by open_by_handle_at().
> 
> For backward compact, kernel will only set handle_flags in
> response if new flags were set in the request.
> 
> Do you consider this extension better than AT_HANDLE_FID
> or worse? At least it is an API change that is contained within the
> exportfs subsystem, without polluting the AT_ flags global namespace.
> 

Personally, yes. I think adding a struct file_handle_v2 would be cleaner
and allows for expanding the API later through new flags.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

handle_bytes

On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Thu, 2023-04-27 at 18:52 +0300, Amir Goldstein wrote:
> > On Thu, Apr 27, 2023 at 6:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > >
> > > On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > > > Jan,
> > > >
> > > > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > > > alternative proposal to seamlessly support more filesystems.
> > > >
> > > > While fanotify relaxes the requirements for filesystems to support
> > > > reporting fid to require only the ->encode_fh() operation, there are
> > > > currently no new filesystems that meet the relaxed requirements.
> > > >
> > > > I will shortly post patches that allow overlayfs to meet the new
> > > > requirements with default overlay configurations.
> > > >
> > > > The overlay and vfs/fanotify patch sets are completely independent.
> > > > The are both available on my github branch [2] and there is a simple
> > > > LTP test variant that tests reporting fid from overlayfs [3], which
> > > > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > > > requesting a non-decodeable file handle by userspace.
> > > >
> > > > Thanks,
> > > > Amir.
> > > >
> > > > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > > > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > > > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> > > >
> > > > Amir Goldstein (4):
> > > >   exportfs: change connectable argument to bit flags
> > > >   exportfs: add explicit flag to request non-decodeable file handles
> > > >   exportfs: allow exporting non-decodeable file handles to userspace
> > > >   fanotify: support reporting non-decodeable file handles
> > > >
> > > >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> > > >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> > > >  fs/fhandle.c                                | 20 ++++++++------
> > > >  fs/nfsd/nfsfh.c                             |  5 ++--
> > > >  fs/notify/fanotify/fanotify.c               |  4 +--
> > > >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> > > >  fs/notify/fdinfo.c                          |  2 +-
> > > >  include/linux/exportfs.h                    | 18 ++++++++++---
> > > >  include/uapi/linux/fcntl.h                  |  5 ++++
> > > >  9 files changed, 67 insertions(+), 26 deletions(-)
> > > >
> > >
> > > This set looks fairly benign to me, so ACK on the general concept.
> >
> > Thanks!
> >
> > >
> > > I am starting to dislike how the AT_* flags are turning into a bunch of
> > > flags that only have meanings on certain syscalls. I don't see a cleaner
> > > way to handle it though.
> >
> > Yeh, it's not great.
> >
> > There is also a way to extend the existing API with:
> >
> > Perhstruct file_handle {
> >         unsigned int handle_bytes:8;
> >         unsigned int handle_flags:24;
> >         int handle_type;
> >         unsigned char f_handle[];
> > };
> >
> > AFAICT, this is guaranteed to be backward compat
> > with old kernels and old applications.
> >
>
> That could work. It would probably look cleaner as a union though.
> Something like this maybe?
>
> union {
>         unsigned int legacy_handle_bytes;
>         struct {
>                 u8      handle_bytes;
>                 u8      __reserved;
>                 u16     handle_flags;
>         };
> }

I have no problem with the union, but does this struct
guarantee that the lowest byte of legacy_handle_bytes
is in handle_bytes for all architectures?

That's the reason I went with

struct {
         unsigned int handle_bytes:8;
         unsigned int handle_flags:24;
}

Is there a problem with this approach?

> >         unsigned int handle_bytes:8;
> >         unsigned int handle_flags:24;
>
> __reserved must be zeroed (for now). You could consider using it for
> some other purpose later.
>
> It's a little ugly as an API but it would be backward compatible, given
> that we never use the high bits today anyway.
>
> Callers might need to deal with an -EINVAL when they try to pass non-
> zero handle_flags to existing kernels, since you'd trip the
> MAX_HANDLE_SZ check that's there today.
>

Exactly.

> > It also may not be a bad idea that the handle_flags could
> > be used to request specific fh properties (FID) and can also
> > describe the properties of the returned fh (i.e. non-decodeable)
> > that could also be respected by open_by_handle_at().
> >
> > For backward compact, kernel will only set handle_flags in
> > response if new flags were set in the request.
> >
> > Do you consider this extension better than AT_HANDLE_FID
> > or worse? At least it is an API change that is contained within the
> > exportfs subsystem, without polluting the AT_ flags global namespace.
> >
>
> Personally, yes. I think adding a struct file_handle_v2 would be cleaner
> and allows for expanding the API later through new flags.

I agree.
I will give it a try.

Thanks,
Amir.

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Jeff Layton]

On Thu, 2023-04-27 at 22:11 +0300, Amir Goldstein wrote:
> handle_bytes
> 
> On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Thu, 2023-04-27 at 18:52 +0300, Amir Goldstein wrote:
> > > On Thu, Apr 27, 2023 at 6:13 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > 
> > > > On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > > > > Jan,
> > > > > 
> > > > > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > > > > alternative proposal to seamlessly support more filesystems.
> > > > > 
> > > > > While fanotify relaxes the requirements for filesystems to support
> > > > > reporting fid to require only the ->encode_fh() operation, there are
> > > > > currently no new filesystems that meet the relaxed requirements.
> > > > > 
> > > > > I will shortly post patches that allow overlayfs to meet the new
> > > > > requirements with default overlay configurations.
> > > > > 
> > > > > The overlay and vfs/fanotify patch sets are completely independent.
> > > > > The are both available on my github branch [2] and there is a simple
> > > > > LTP test variant that tests reporting fid from overlayfs [3], which
> > > > > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > > > > requesting a non-decodeable file handle by userspace.
> > > > > 
> > > > > Thanks,
> > > > > Amir.
> > > > > 
> > > > > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > > > > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > > > > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> > > > > 
> > > > > Amir Goldstein (4):
> > > > >   exportfs: change connectable argument to bit flags
> > > > >   exportfs: add explicit flag to request non-decodeable file handles
> > > > >   exportfs: allow exporting non-decodeable file handles to userspace
> > > > >   fanotify: support reporting non-decodeable file handles
> > > > > 
> > > > >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> > > > >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> > > > >  fs/fhandle.c                                | 20 ++++++++------
> > > > >  fs/nfsd/nfsfh.c                             |  5 ++--
> > > > >  fs/notify/fanotify/fanotify.c               |  4 +--
> > > > >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> > > > >  fs/notify/fdinfo.c                          |  2 +-
> > > > >  include/linux/exportfs.h                    | 18 ++++++++++---
> > > > >  include/uapi/linux/fcntl.h                  |  5 ++++
> > > > >  9 files changed, 67 insertions(+), 26 deletions(-)
> > > > > 
> > > > 
> > > > This set looks fairly benign to me, so ACK on the general concept.
> > > 
> > > Thanks!
> > > 
> > > > 
> > > > I am starting to dislike how the AT_* flags are turning into a bunch of
> > > > flags that only have meanings on certain syscalls. I don't see a cleaner
> > > > way to handle it though.
> > > 
> > > Yeh, it's not great.
> > > 
> > > There is also a way to extend the existing API with:
> > > 
> > > Perhstruct file_handle {
> > >         unsigned int handle_bytes:8;
> > >         unsigned int handle_flags:24;
> > >         int handle_type;
> > >         unsigned char f_handle[];
> > > };
> > > 
> > > AFAICT, this is guaranteed to be backward compat
> > > with old kernels and old applications.
> > > 
> > 
> > That could work. It would probably look cleaner as a union though.
> > Something like this maybe?
> > 
> > union {
> >         unsigned int legacy_handle_bytes;
> >         struct {
> >                 u8      handle_bytes;
> >                 u8      __reserved;
> >                 u16     handle_flags;
> >         };
> > }
> 
> I have no problem with the union, but does this struct
> guarantee that the lowest byte of legacy_handle_bytes
> is in handle_bytes for all architectures?
> 

That is a very good point. 

> That's the reason I went with
> 
> struct {
>          unsigned int handle_bytes:8;
>          unsigned int handle_flags:24;
> }
> 
> Is there a problem with this approach?
> 

I just have a natural aversion to bitfields.

What you're proposing would work fine, I think. You won't be able to
take a pointer into the bitfield of course, but that's not necessarily a
showstopper for an "interface struct" like file_handle.



> > >         unsigned int handle_bytes:8;
> > >         unsigned int handle_flags:24;
> > 
> > __reserved must be zeroed (for now). You could consider using it for
> > some other purpose later.
> > 
> > It's a little ugly as an API but it would be backward compatible, given
> > that we never use the high bits today anyway.
> > 
> > Callers might need to deal with an -EINVAL when they try to pass non-
> > zero handle_flags to existing kernels, since you'd trip the
> > MAX_HANDLE_SZ check that's there today.
> > 
> 
> Exactly.
> 
> > > It also may not be a bad idea that the handle_flags could
> > > be used to request specific fh properties (FID) and can also
> > > describe the properties of the returned fh (i.e. non-decodeable)
> > > that could also be respected by open_by_handle_at().
> > > 
> > > For backward compact, kernel will only set handle_flags in
> > > response if new flags were set in the request.
> > > 
> > > Do you consider this extension better than AT_HANDLE_FID
> > > or worse? At least it is an API change that is contained within the
> > > exportfs subsystem, without polluting the AT_ flags global namespace.
> > > 
> > 
> > Personally, yes. I think adding a struct file_handle_v2 would be cleaner
> > and allows for expanding the API later through new flags.
> 
> I agree.
> I will give it a try.

Cool.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Jan Kara]

On Thu 27-04-23 22:11:46, Amir Goldstein wrote:
> On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > There is also a way to extend the existing API with:
> > >
> > > Perhstruct file_handle {
> > >         unsigned int handle_bytes:8;
> > >         unsigned int handle_flags:24;
> > >         int handle_type;
> > >         unsigned char f_handle[];
> > > };
> > >
> > > AFAICT, this is guaranteed to be backward compat
> > > with old kernels and old applications.
> > >
> >
> > That could work. It would probably look cleaner as a union though.
> > Something like this maybe?
> >
> > union {
> >         unsigned int legacy_handle_bytes;
> >         struct {
> >                 u8      handle_bytes;
> >                 u8      __reserved;
> >                 u16     handle_flags;
> >         };
> > }
> 
> I have no problem with the union, but does this struct
> guarantee that the lowest byte of legacy_handle_bytes
> is in handle_bytes for all architectures?
> 
> That's the reason I went with
> 
> struct {
>          unsigned int handle_bytes:8;
>          unsigned int handle_flags:24;
> }
> 
> Is there a problem with this approach?

As I'm thinking about it there are problems with both approaches in the
uAPI. The thing is: A lot of bitfield details (even whether they are packed
to a single int or not) are implementation defined (depends on the
architecture as well as the compiler) so they are not really usable in the
APIs.

With the union, things are well-defined but they would not work for
big-endian architectures. We could make the structure layout depend on the
endianity but that's quite ugly...

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

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Jeff Layton]

On Fri, 2023-04-28 at 13:40 +0200, Jan Kara wrote:
> On Thu 27-04-23 22:11:46, Amir Goldstein wrote:
> > On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > There is also a way to extend the existing API with:
> > > > 
> > > > Perhstruct file_handle {
> > > >         unsigned int handle_bytes:8;
> > > >         unsigned int handle_flags:24;
> > > >         int handle_type;
> > > >         unsigned char f_handle[];
> > > > };
> > > > 
> > > > AFAICT, this is guaranteed to be backward compat
> > > > with old kernels and old applications.
> > > > 
> > > 
> > > That could work. It would probably look cleaner as a union though.
> > > Something like this maybe?
> > > 
> > > union {
> > >         unsigned int legacy_handle_bytes;
> > >         struct {
> > >                 u8      handle_bytes;
> > >                 u8      __reserved;
> > >                 u16     handle_flags;
> > >         };
> > > }
> > 
> > I have no problem with the union, but does this struct
> > guarantee that the lowest byte of legacy_handle_bytes
> > is in handle_bytes for all architectures?
> > 
> > That's the reason I went with
> > 
> > struct {
> >          unsigned int handle_bytes:8;
> >          unsigned int handle_flags:24;
> > }
> > 
> > Is there a problem with this approach?
> 
> As I'm thinking about it there are problems with both approaches in the
> uAPI. The thing is: A lot of bitfield details (even whether they are packed
> to a single int or not) are implementation defined (depends on the
> architecture as well as the compiler) so they are not really usable in the
> APIs.
> 
> With the union, things are well-defined but they would not work for
> big-endian architectures. We could make the structure layout depend on the
> endianity but that's quite ugly...
> 

Good point. Bitfields just have a bad code-smell anyway.

Another idea would be to allow someone to set handle_bytes to a
specified value that's larger than the current max of 128 (maybe ~0 or
something), and use that as an indicator that this is a v2 struct.

So the v2 struct would look something like:

struct file_handle_v2 {
	unsigned int	legacy_handle_bytes;	// always set to ~0 or whatever
	unsigned int	flags;
	int		handle_type;
	unsigned int	handle_bytes;
	unsigned char	f_handle[];
	
};

...but now I'm wondering...why do we return -EINVAL when
f_handle.handle_bytes is > MAX_HANDLE_SZ? Is it really wrong for the
caller to allocate more space for the resulting file_handle than will be
needed? That seems wrong too. In fact, name_to_handle_at(2) says:

"The constant MAX_HANDLE_SZ, defined in <fcntl.h>, specifies the maximum
expected size for a file handle.  It  is  not guaranteed upper limit as
future filesystems may require more space."

So by returning -EINVAL when handle_bytes is too large, we're probably
doing the wrong thing there.

Using an AT_* flag may be the best plan, actually.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Jan Kara]

On Fri 28-04-23 08:15:50, Jeff Layton wrote:
> On Fri, 2023-04-28 at 13:40 +0200, Jan Kara wrote:
> > On Thu 27-04-23 22:11:46, Amir Goldstein wrote:
> > > On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > There is also a way to extend the existing API with:
> > > > > 
> > > > > Perhstruct file_handle {
> > > > >         unsigned int handle_bytes:8;
> > > > >         unsigned int handle_flags:24;
> > > > >         int handle_type;
> > > > >         unsigned char f_handle[];
> > > > > };
> > > > > 
> > > > > AFAICT, this is guaranteed to be backward compat
> > > > > with old kernels and old applications.
> > > > > 
> > > > 
> > > > That could work. It would probably look cleaner as a union though.
> > > > Something like this maybe?
> > > > 
> > > > union {
> > > >         unsigned int legacy_handle_bytes;
> > > >         struct {
> > > >                 u8      handle_bytes;
> > > >                 u8      __reserved;
> > > >                 u16     handle_flags;
> > > >         };
> > > > }
> > > 
> > > I have no problem with the union, but does this struct
> > > guarantee that the lowest byte of legacy_handle_bytes
> > > is in handle_bytes for all architectures?
> > > 
> > > That's the reason I went with
> > > 
> > > struct {
> > >          unsigned int handle_bytes:8;
> > >          unsigned int handle_flags:24;
> > > }
> > > 
> > > Is there a problem with this approach?
> > 
> > As I'm thinking about it there are problems with both approaches in the
> > uAPI. The thing is: A lot of bitfield details (even whether they are packed
> > to a single int or not) are implementation defined (depends on the
> > architecture as well as the compiler) so they are not really usable in the
> > APIs.
> > 
> > With the union, things are well-defined but they would not work for
> > big-endian architectures. We could make the structure layout depend on the
> > endianity but that's quite ugly...
> > 
> 
> Good point. Bitfields just have a bad code-smell anyway.
> 
> Another idea would be to allow someone to set handle_bytes to a
> specified value that's larger than the current max of 128 (maybe ~0 or
> something), and use that as an indicator that this is a v2 struct.
> 
> So the v2 struct would look something like:
> 
> struct file_handle_v2 {
> 	unsigned int	legacy_handle_bytes;	// always set to ~0 or whatever
> 	unsigned int	flags;
> 	int		handle_type;
> 	unsigned int	handle_bytes;
> 	unsigned char	f_handle[];
> 	
> };

Well, there's also always the option of having:

struct file_handle {
	unsigned int handle_bytes_flags;
	int handle_type;
	unsigned char f_handle[];
};

And then helper functions like:

unsigned int file_handle_bytes(struct file_handle *handle)
{
	return handle->handle_bytes_flags & 0xffff;
}

unsigned int file_handle_flags(struct file_handle *handle)
{
	return handle->handle_bytes_flags >> 16;
}

(and similar for forming the handle_bytes_flags value).

That is well defined and compatible across architectures and compilers and
bearable (although a bit clumsy).

> ...but now I'm wondering...why do we return -EINVAL when
> f_handle.handle_bytes is > MAX_HANDLE_SZ? Is it really wrong for the
> caller to allocate more space for the resulting file_handle than will be
> needed? That seems wrong too. In fact, name_to_handle_at(2) says:
> 
> "The constant MAX_HANDLE_SZ, defined in <fcntl.h>, specifies the maximum
> expected size for a file handle.  It  is  not guaranteed upper limit as
> future filesystems may require more space."
> 
> So by returning -EINVAL when handle_bytes is too large, we're probably
> doing the wrong thing there.

Yeah, you're right. But at this point it can serve us well by making sure
there's no userspace passing absurdly high handle_bytes ;).

								Honza

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

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

On Fri, Apr 28, 2023 at 3:15 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2023-04-28 at 13:40 +0200, Jan Kara wrote:
> > On Thu 27-04-23 22:11:46, Amir Goldstein wrote:
> > > On Thu, Apr 27, 2023 at 7:36 PM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > There is also a way to extend the existing API with:
> > > > >
> > > > > Perhstruct file_handle {
> > > > >         unsigned int handle_bytes:8;
> > > > >         unsigned int handle_flags:24;
> > > > >         int handle_type;
> > > > >         unsigned char f_handle[];
> > > > > };
> > > > >
> > > > > AFAICT, this is guaranteed to be backward compat
> > > > > with old kernels and old applications.
> > > > >
> > > >
> > > > That could work. It would probably look cleaner as a union though.
> > > > Something like this maybe?
> > > >
> > > > union {
> > > >         unsigned int legacy_handle_bytes;
> > > >         struct {
> > > >                 u8      handle_bytes;
> > > >                 u8      __reserved;
> > > >                 u16     handle_flags;
> > > >         };
> > > > }
> > >
> > > I have no problem with the union, but does this struct
> > > guarantee that the lowest byte of legacy_handle_bytes
> > > is in handle_bytes for all architectures?
> > >
> > > That's the reason I went with
> > >
> > > struct {
> > >          unsigned int handle_bytes:8;
> > >          unsigned int handle_flags:24;
> > > }
> > >
> > > Is there a problem with this approach?
> >
> > As I'm thinking about it there are problems with both approaches in the
> > uAPI. The thing is: A lot of bitfield details (even whether they are packed
> > to a single int or not) are implementation defined (depends on the
> > architecture as well as the compiler) so they are not really usable in the
> > APIs.
> >
> > With the union, things are well-defined but they would not work for
> > big-endian architectures. We could make the structure layout depend on the
> > endianity but that's quite ugly...
> >
>
> Good point. Bitfields just have a bad code-smell anyway.
>
> Another idea would be to allow someone to set handle_bytes to a
> specified value that's larger than the current max of 128 (maybe ~0 or
> something), and use that as an indicator that this is a v2 struct.
>
> So the v2 struct would look something like:
>
> struct file_handle_v2 {
>         unsigned int    legacy_handle_bytes;    // always set to ~0 or whatever
>         unsigned int    flags;
>         int             handle_type;
>         unsigned int    handle_bytes;
>         unsigned char   f_handle[];
>
> };

The three of us are racing with proposals of crazy solutions ;)

I was going to propose:

struct file_handle_v2 {
        union {
                u32 legacy_handle_bytes;
                struct {
                        u16 handle_bytes;
                        u8 handle_flags;
                        u8 reserved;
                };
        };
        int handle_type;
        unsigned char f_handle[];
};

which is similar to your first proposal, but the way to use it would be:

static inline int file_handle_bytes(struct file_handle_v2 *handle)
{
        return (handle->legacy_handle_bytes < MAX_HANDLE_SZ) ?
                handle->legacy_handle_bytes : handle->handle_bytes;
}

I think this works for both LE and BE, because non zero handle_flags
would taint legacy_handle_bytes either way.

>
> ...but now I'm wondering...why do we return -EINVAL when
> f_handle.handle_bytes is > MAX_HANDLE_SZ? Is it really wrong for the
> caller to allocate more space for the resulting file_handle than will be
> needed? That seems wrong too. In fact, name_to_handle_at(2) says:
>
> "The constant MAX_HANDLE_SZ, defined in <fcntl.h>, specifies the maximum
> expected size for a file handle.  It  is  not guaranteed upper limit as
> future filesystems may require more space."
>
> So by returning -EINVAL when handle_bytes is too large, we're probably
> doing the wrong thing there.

Yeh it is very strange, but now it is very convenient too,
so no reason to fix it retroactively.

With the file_handle_v2 I proposed above, new handle_bytes is 16bit
so we won't need to error on large buffer size when using file_handle_v2.

Another odd thing about struct file_handle is that it is not actually
defined in include/uapi header files, although it is defined in the man page
of name_to_handle_at(2).
That is another thing that we can fix with file_handle_v2.

>
> Using an AT_* flag may be the best plan, actually.

It's true, but perhaps AT_HANDLE_V2 and then the
handle_flags could be easily extended later.

Thanks,
Amir.

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Chuck Lever]

On Thu, Apr 27, 2023 at 11:13:33AM -0400, Jeff Layton wrote:
> On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > Jan,
> > 
> > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > alternative proposal to seamlessly support more filesystems.
> > 
> > While fanotify relaxes the requirements for filesystems to support
> > reporting fid to require only the ->encode_fh() operation, there are
> > currently no new filesystems that meet the relaxed requirements.
> > 
> > I will shortly post patches that allow overlayfs to meet the new
> > requirements with default overlay configurations.
> > 
> > The overlay and vfs/fanotify patch sets are completely independent.
> > The are both available on my github branch [2] and there is a simple
> > LTP test variant that tests reporting fid from overlayfs [3], which
> > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > requesting a non-decodeable file handle by userspace.
> > 
> > Thanks,
> > Amir.
> > 
> > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> > 
> > Amir Goldstein (4):
> >   exportfs: change connectable argument to bit flags
> >   exportfs: add explicit flag to request non-decodeable file handles
> >   exportfs: allow exporting non-decodeable file handles to userspace
> >   fanotify: support reporting non-decodeable file handles
> > 
> >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> >  fs/fhandle.c                                | 20 ++++++++------
> >  fs/nfsd/nfsfh.c                             |  5 ++--
> >  fs/notify/fanotify/fanotify.c               |  4 +--
> >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> >  fs/notify/fdinfo.c                          |  2 +-
> >  include/linux/exportfs.h                    | 18 ++++++++++---
> >  include/uapi/linux/fcntl.h                  |  5 ++++
> >  9 files changed, 67 insertions(+), 26 deletions(-)
> > 
> 
> This set looks fairly benign to me, so ACK on the general concept.

Me also (modulo previous review comments), so

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

I assume either Amir or Jeff will take these when they are ready.
If I'm wrong, please do let me know and I can take them via the
NFSD tree.


> I am starting to dislike how the AT_* flags are turning into a bunch of
> flags that only have meanings on certain syscalls. I don't see a cleaner
> way to handle it though.
> -- 
> Jeff Layton <jlayton@kernel.org>

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

On Sat, Apr 29, 2023 at 5:45 PM Chuck Lever <cel@kernel.org> wrote:
>
> On Thu, Apr 27, 2023 at 11:13:33AM -0400, Jeff Layton wrote:
> > On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > > Jan,
> > >
> > > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > > alternative proposal to seamlessly support more filesystems.
> > >
> > > While fanotify relaxes the requirements for filesystems to support
> > > reporting fid to require only the ->encode_fh() operation, there are
> > > currently no new filesystems that meet the relaxed requirements.
> > >
> > > I will shortly post patches that allow overlayfs to meet the new
> > > requirements with default overlay configurations.
> > >
> > > The overlay and vfs/fanotify patch sets are completely independent.
> > > The are both available on my github branch [2] and there is a simple
> > > LTP test variant that tests reporting fid from overlayfs [3], which
> > > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > > requesting a non-decodeable file handle by userspace.
> > >
> > > Thanks,
> > > Amir.
> > >
> > > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> > >
> > > Amir Goldstein (4):
> > >   exportfs: change connectable argument to bit flags
> > >   exportfs: add explicit flag to request non-decodeable file handles
> > >   exportfs: allow exporting non-decodeable file handles to userspace
> > >   fanotify: support reporting non-decodeable file handles
> > >
> > >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> > >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> > >  fs/fhandle.c                                | 20 ++++++++------
> > >  fs/nfsd/nfsfh.c                             |  5 ++--
> > >  fs/notify/fanotify/fanotify.c               |  4 +--
> > >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> > >  fs/notify/fdinfo.c                          |  2 +-
> > >  include/linux/exportfs.h                    | 18 ++++++++++---
> > >  include/uapi/linux/fcntl.h                  |  5 ++++
> > >  9 files changed, 67 insertions(+), 26 deletions(-)
> > >
> >
> > This set looks fairly benign to me, so ACK on the general concept.
>
> Me also (modulo previous review comments), so
>
>   Acked-by: Chuck Lever <chuck.lever@oracle.com>
>
> I assume either Amir or Jeff will take these when they are ready.
> If I'm wrong, please do let me know and I can take them via the
> NFSD tree.
>

With your and Jeff's ACKs I think it would be best if Jan takes
these changes through the fsnotify tree, because they are only
meant to improve fanotify at this point.

>
> > I am starting to dislike how the AT_* flags are turning into a bunch of
> > flags that only have meanings on certain syscalls. I don't see a cleaner
> > way to handle it though.

With all the various proposals of file_handle_v2, I still think that the
AT_HANDLE_FID is the cleanest in terms of API simplicity.

Just trying to document file_handle_v2 and backward compat with
file_handle_v1 gives me a headache and documenting AT_HANDLE_FID
is a no brainer.

so if nobody objects I will stick with AT_HANDLE_FID.

Thanks,
Amir.

Re: [RFC][PATCH 0/4] Prepare for supporting more filesystems with fanotify

[Amir Goldstein]

On Sat, Apr 29, 2023 at 8:26 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sat, Apr 29, 2023 at 5:45 PM Chuck Lever <cel@kernel.org> wrote:
> >
> > On Thu, Apr 27, 2023 at 11:13:33AM -0400, Jeff Layton wrote:
> > > On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:
> > > > Jan,
> > > >
> > > > Following up on the FAN_REPORT_ANY_FID proposal [1], here is a shot at an
> > > > alternative proposal to seamlessly support more filesystems.
> > > >
> > > > While fanotify relaxes the requirements for filesystems to support
> > > > reporting fid to require only the ->encode_fh() operation, there are
> > > > currently no new filesystems that meet the relaxed requirements.
> > > >
> > > > I will shortly post patches that allow overlayfs to meet the new
> > > > requirements with default overlay configurations.
> > > >
> > > > The overlay and vfs/fanotify patch sets are completely independent.
> > > > The are both available on my github branch [2] and there is a simple
> > > > LTP test variant that tests reporting fid from overlayfs [3], which
> > > > also demonstrates the minor UAPI change of name_to_handle_at(2) for
> > > > requesting a non-decodeable file handle by userspace.
> > > >
> > > > Thanks,
> > > > Amir.
> > > >
> > > > [1] https://lore.kernel.org/linux-fsdevel/20230417162721.ouzs33oh6mb7vtft@quack3/
> > > > [2] https://github.com/amir73il/linux/commits/exportfs_encode_fid
> > > > [3] https://github.com/amir73il/ltp/commits/exportfs_encode_fid
> > > >
> > > > Amir Goldstein (4):
> > > >   exportfs: change connectable argument to bit flags
> > > >   exportfs: add explicit flag to request non-decodeable file handles
> > > >   exportfs: allow exporting non-decodeable file handles to userspace
> > > >   fanotify: support reporting non-decodeable file handles
> > > >
> > > >  Documentation/filesystems/nfs/exporting.rst |  4 +--
> > > >  fs/exportfs/expfs.c                         | 29 ++++++++++++++++++---
> > > >  fs/fhandle.c                                | 20 ++++++++------
> > > >  fs/nfsd/nfsfh.c                             |  5 ++--
> > > >  fs/notify/fanotify/fanotify.c               |  4 +--
> > > >  fs/notify/fanotify/fanotify_user.c          |  6 ++---
> > > >  fs/notify/fdinfo.c                          |  2 +-
> > > >  include/linux/exportfs.h                    | 18 ++++++++++---
> > > >  include/uapi/linux/fcntl.h                  |  5 ++++
> > > >  9 files changed, 67 insertions(+), 26 deletions(-)
> > > >
> > >
> > > This set looks fairly benign to me, so ACK on the general concept.
> >
> > Me also (modulo previous review comments), so
> >
> >   Acked-by: Chuck Lever <chuck.lever@oracle.com>
> >
> > I assume either Amir or Jeff will take these when they are ready.
> > If I'm wrong, please do let me know and I can take them via the
> > NFSD tree.
> >
>
> With your and Jeff's ACKs I think it would be best if Jan takes
> these changes through the fsnotify tree, because they are only
> meant to improve fanotify at this point.
>
> >
> > > I am starting to dislike how the AT_* flags are turning into a bunch of
> > > flags that only have meanings on certain syscalls. I don't see a cleaner
> > > way to handle it though.
>
> With all the various proposals of file_handle_v2, I still think that the
> AT_HANDLE_FID is the cleanest in terms of API simplicity.
>
> Just trying to document file_handle_v2 and backward compat with
> file_handle_v1 gives me a headache and documenting AT_HANDLE_FID
> is a no brainer.
>

To prove my point, here is the man page draft for AT_HANDLE_FID:

https://github.com/amir73il/man-pages/commit/da7e8dc4749ced85ba692073a42724f2bbe5fe3b

Thanks,
Amir.