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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Sure, I added:

+/*
+ * Filesystems use only lower 8 bits of file_handle type for fid_type.
+ * name_to_handle_at() uses upper 16 bits of type as user flags to be
+ * interpreted by open_by_handle_at().
+ */

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

Me too :)

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

--- a/fs/exportfs/expfs.c
+++ b/fs/exportfs/expfs.c
@@ -382,14 +382,21 @@ int exportfs_encode_inode_fh(struct inode
*inode, struct fid *fid,
                             int *max_len, struct inode *parent, int flags)
 {
        const struct export_operations *nop = inode->i_sb->s_export_op;
+       enum fid_type type;

        if (!exportfs_can_encode_fh(nop, flags))
                return -EOPNOTSUPP;

        if (!nop && (flags & EXPORT_FH_FID))
-               return exportfs_encode_ino64_fid(inode, fid, max_len);
+               type = exportfs_encode_ino64_fid(inode, fid, max_len);
+       else
+               type = nop->encode_fh(inode, fid->raw, max_len, parent);
+
+       if (WARN_ON_ONCE(FILEID_USER_FLAGS(type)))
+               return -EINVAL;
+
+       return type;

-       return nop->encode_fh(inode, fid->raw, max_len, parent);
 }
 EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);

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

+       if (WARN_ON_ONCE(FILEID_USER_FLAGS(fileid_type)))
+               return -EINVAL;
+

I will post it in v3 after testing.

Thanks for the review!
Amir.





[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux