Re: Immutable vs read-only for Windows compatibility

[Amir Goldstein]

> > > > What could be useful for userspace is also ability to figure out which
> > > > FS_DOSATTRIB_* are supported by the filesystem. Because for example UDF
> > > > on-disk format supports only FS_DOSATTRIB_HIDDEN bit. And FAT only those
> > > > attributes which are in the lowest byte.
> > > >
> > >
> > > Exactly.
> > > statx has this solved with the stx_attributes_mask field.
> > >
> > > We could do the same for FS_IOC_FS[GS]ETXATTR, but because
> > > right now, this API does not verify that fsx_pad is zero, we will need to
> > > define a new set of ioctl consants FS_IOC_[GS]ETFSXATTR2
> > > with the exact same functionality but that userspace knows that they
> > > publish and respect the dosattrib mask:
> >
> > I understand and this is a problem.
> >
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -868,9 +868,11 @@ static int do_vfs_ioctl(struct file *filp, unsigned int fd,
> > >         case FS_IOC_SETFLAGS:
> > >                 return ioctl_setflags(filp, argp);
> > >
> > > +       case FS_IOC_GETFSXATTR2:
> > >         case FS_IOC_FSGETXATTR:
> > >                 return ioctl_fsgetxattr(filp, argp);
> > >
> > > +       case FS_IOC_SETFSXATTR2:
> > >         case FS_IOC_FSSETXATTR:
> > >                 return ioctl_fssetxattr(filp, argp);
> > > --- a/include/uapi/linux/fs.h
> > > +++ b/include/uapi/linux/fs.h
> > > @@ -145,7 +145,8 @@ struct fsxattr {
> > >         __u32           fsx_nextents;   /* nextents field value (get)   */
> > >         __u32           fsx_projid;     /* project identifier (get/set) */
> > >         __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > -       unsigned char   fsx_pad[8];
> > > +       __u32           fsx_dosattrib;  /* dosattrib field value (get/set) */
> > > +       __u32           fsx_dosattrib_mask; /* dosattrib field validity mask */
> > >  };
> > >
> > >  /*
> > > @@ -238,6 +248,9 @@ struct fsxattr {
> > >  #define FS_IOC32_SETFLAGS              _IOW('f', 2, int)
> > >  #define FS_IOC32_GETVERSION            _IOR('v', 1, int)
> > >  #define FS_IOC32_SETVERSION            _IOW('v', 2, int)
> > > +#define FS_IOC_GETFSXATTR2              _IOR('x', 31, struct fsxattr)
> > > +#define FS_IOC_SETFSXATTR2              _IOW('x', 32, struct fsxattr)
> > > +/* Duplicate legacy ioctl numbers for backward compact */
> > >  #define FS_IOC_FSGETXATTR              _IOR('X', 31, struct fsxattr)
> > >  #define FS_IOC_FSSETXATTR              _IOW('X', 32, struct fsxattr)
> > >  #define FS_IOC_GETFSLABEL              _IOR(0x94, 49, char[FSLABEL_MAX])
> > >
> > > We could also use this opportunity to define a larger fsxattr2 struct
> > > that also includes an fsx_xflags_mask field, so that the xflags namespace
> > > could also be extended in a backward compat way going forward:
> > >
> > > @@ -145,7 +145,21 @@ struct fsxattr {
> > >         __u32           fsx_nextents;   /* nextents field value (get)   */
> > >         __u32           fsx_projid;     /* project identifier (get/set) */
> > >         __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > >         unsigned char   fsx_pad[8];
> > >
> > >  };
> > > +
> > > +/*
> > > + * Structure for FS_IOC_[GS]ETFSXATTR2.
> > > + */
> > > +struct fsxattr2 {
> > > +       __u32           fsx_xflags;     /* xflags field value (get/set) */
> > > +       __u32           fsx_extsize;    /* extsize field value (get/set)*/
> > > +       __u32           fsx_nextents;   /* nextents field value (get)   */
> > > +       __u32           fsx_projid;     /* project identifier (get/set) */
> > > +       __u32           fsx_cowextsize; /* CoW extsize field value (get/set)*/
> > > +       __u32           fsx_xflags_mask; /* xflags field validity mask */
> > > +       __u32           fsx_dosattrib;  /* dosattrib field value (get/set) */
> > > +       __u32           fsx_dosattrib_mask; /* dosattrib field validity mask */
> > > +};
> > >
> > > And you'd also need to flug those new mask and dosattrib
> > > via struct fileattr into filesystems - too much to explain.
> > > try to figure it out (unless someone objects) and if you can't figure
> > > it out let me know.
> >
> > Yea, I think that this is thing which I should be able to figure out
> > once I start changing it.
> >
> > Anyway, I have alternative idea to the problem with fsx_pad. What about
> > introducing new fsx_xflags flag which would say that fsx_pad=fsx_dosattrib
> > is present? E.g.
> >
> > #define FS_XFLAG_HASDOSATTRIB 0x40000000
> >
> > Then we would not need new FS_IOC_GETFSXATTR2/FS_IOC_SETFSXATTR2 ioctls.
> >
> > Also fsx_pad has 8 bytes, which can store both attrib and mask, so new
> > struct fsxattr2 would not be needed too.
>
> In case we decide to not do 1-to-1 mapping of Windows FILE_ATTRIBUTE_*

The 1-to-1 is definitely not a requirement of the API.

> constants to these new Linux DOSATTRIB_* constants then we do not need
> 32-bit variable, but just 16-bit variable (I counted that we need just
> 10 bits for now).

The "for now" part is what worries me in this sentence.

> fsx_pad has currently 64 bits and we could use it for:
> - 32 bits for fsx_xflags_mask
> - 16 bits for fsx_dosattrib
> - 16 bits for fsx_dosattrib_mask

This is possible.

> And therefore no need for FS_IOC_GETFSXATTR2/FS_IOC_SETFSXATTR2 iocl or
> struct fsxattr2. Just an idea how to simplify new API. Maybe together

no need for struct fsxattr2.
but regarding no new ioctl I am not so sure.

> with new fsx_xflags bit to indicate that fsx_xflags_mask field is present:
> #define FS_XFLAG_HASXFLAGSMASK 0x20000000
>

I don't think that this flag is needed because there is no filesystem
with empty xflags_mask, so any non zero value of xflags_mask
is equivalent to setting this flag.

This is for the get side. For the set side things are more complicated.
A proper backward compat API should reject (-EINVAL) unknown flags.
As far as I can tell ioctl_fssetxattr() does not at any time verify that
fsx_pad is zero and as far as I can tell xfs_fileattr_set() does not
check for unsupported fsx_xflags.

So unless I am missing something, FS_IOC_FSSETXATTR will silently
ignore dosattrib flags (and mask) when called on an old kernel.
This is the justification of FS_IOC_SETFSXATTR2 - it will fail on an
old kernel, so application can trust that if it works - it also respects
dosattib and the masks.

Yes, applications can call FS_IOC_FSGETXATTR, check non zero
mask and deduce that FS_IOC_FSSETXATTR will respect the mask
This will probably work, but it is not a very clean API IMO.

Thanks,
Amir.