On Thu, Apr 27, 2023 at 6:00 PM Jeff Layton [off-list ref] wrote:

On Tue, 2023-04-25 at 16:01 +0300, Amir Goldstein wrote:

quoted

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.

quoted

      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.