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 Wed, Oct 9, 2024 at 11:40 AM Jan Kara <jack@xxxxxxx> wrote:
>
> On Tue 08-10-24 15:11:39, 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.
>
> Thanks. I've actually seen this series on list, went "eww bitfields, let's
> sleep to this" and never got back to it.
>
> > 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)
> > +
> > +/* 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)
>
> FWIW I prefer this variant much over bitfields as their binary format
> depends on the compiler which leads to unpleasant surprises sooner rather
> than later.
>mask
> > +#define FILEID_USER_TYPE_IS_VALID(type) \
> > +       (FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS)
>
> The macro name is confusing

Confusing enough to hide the fact that it was negated in v2...

> because it speaks about type but actually
> checks flags. Frankly, I'd just fold this in the single call site to make
> things obvious.

Agree. but see below...

>
> > 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;
>
> Maybe you want to make handle_type unsigned when you treat it (partially)
> as flags? Otherwise some constructs can lead to surprises with sign
> extension etc...

That seems like a good idea, but when I look at:

        /* we ask for a non connectable maybe decodeable file handle */
        retval = exportfs_encode_fh(path->dentry,
                                    (struct fid *)handle->f_handle,
                                    &handle_dwords, fh_flags);
        handle->handle_type = retval;
        /* convert handle size to bytes */
        handle_bytes = handle_dwords * sizeof(u32);
        handle->handle_bytes = handle_bytes;
        if ((handle->handle_bytes > f_handle.handle_bytes) ||
            (retval == FILEID_INVALID) || (retval < 0)) {
                /* As per old exportfs_encode_fh documentation
                 * we could return ENOSPC to indicate overflow
                 * But file system returned 255 always. So handle
                 * both the values
                 */
                if (retval == FILEID_INVALID || retval == -ENOSPC)
                        retval = -EOVERFLOW;
                /*

I realize that we actually return negative values in handle_type
(-ESTALE would be quite common).

So we probably don't want to make handle_type unsigned now,
but maybe we do want a macro:

#define FILEID_IS_USER_TYPE_VALID(type) \
             ((type) >= 0 && \
              !(FILEID_USER_FLAGS(type) & ~FILEID_VALID_USER_FLAGS))

that will be used for the assertions instead of assuming that
FILEID_USER_FLAGS_MASK includes the sign bit.

huh?

Thanks,
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