Re: Immutable vs read-only for Windows compatibility

[Amir Goldstein]

On Tue, Feb 4, 2025 at 12:34 AM Pali Rohár <pali@xxxxxxxxxx> wrote:
>
> On Tuesday 04 February 2025 00:02:44 Amir Goldstein wrote:
> > On Mon, Feb 3, 2025 at 11:20 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
> > >
> > > On Monday 03 February 2025 22:59:46 Amir Goldstein wrote:
> > > > On Sun, Feb 2, 2025 at 4:23 PM Pali Rohár <pali@xxxxxxxxxx> wrote:
> > > > > And there is still unresolved issue with FILE_ATTRIBUTE_READONLY.
> > > > > Its meaning is similar to existing Linux FS_IMMUTABLE_FL, just
> > > > > FILE_ATTRIBUTE_READONLY does not require root / CAP_LINUX_IMMUTABLE.
> > > > >
> > > > > I think that for proper support, to enforce FILE_ATTRIBUTE_READONLY
> > > > > functionality, it is needed to introduce new flag e.g.
> > > > > FS_IMMUTABLE_FL_USER to allow setting / clearing it also for normal
> > > > > users without CAP_LINUX_IMMUTABLE. Otherwise it would be unsuitable for
> > > > > any SMB client, SMB server or any application which would like to use
> > > > > it, for example wine.
> > > > >
> > > > > Just to note that FreeBSD has two immutable flags SF_IMMUTABLE and
> > > > > UF_IMMUTABLE, one settable only by superuser and second for owner.
> > > > >
> > > > > Any opinion?
> > > >
> > > > For filesystems that already support FILE_ATTRIBUTE_READONLY,
> > > > can't you just set S_IMMUTABLE on the inode and vfs will do the correct
> > > > enforcement?
> > > >
> > > > The vfs does not control if and how S_IMMUTABLE is set by filesystems,
> > > > so if you want to remove this vfs flag without CAP_LINUX_IMMUTABLE
> > > > in smb client, there is nothing stopping you (I think).
> > >
> > > Function fileattr_set_prepare() checks for CAP_LINUX_IMMUTABLE when
> > > trying to change FS_IMMUTABLE_FL bit. This function is called from
> > > ioctl(FS_IOC_SETFLAGS) and also from ioctl(FS_IOC_FSSETXATTR).
> > > And when function fileattr_set_prepare() fails then .fileattr_set
> > > callback is not called at all. So I think that it is not possible to
> > > remove the IMMUTABLE flag from userspace without capability for smb
> > > client.
> > >
> >
> > You did not understand what I meant.
> >
> > You cannot relax the CAP_LINUX_IMMUTABLE for setting FS_IMMUTABLE_FL
> > and there is no reason that you will need to relax it.
> >
> > The vfs does NOT enforce permissions according to FS_IMMUTABLE_FL
> > The vfs enforces permissions according to the S_IMMUTABLE in-memory
> > inode flag.
> >
> > There is no generic vfs code that sets S_IMMUTABLE inode flags, its
> > the filesystems that translate the on-disk FS_IMMUTABLE_FL to
> > in-memory S_IMMUTABLE inode flag.
> >
> > So if a filesystem already has an internal DOSATTRIB flags set, this
> > filesystem can set the in-memory S_IMMUTABLE inode flag according
> > to its knowledge of the DOSATTRIB_READONLY flag and the
> > CAP_LINUX_IMMUTABLE rules do not apply to the DOSATTRIB_READONLY
> > flag, which is NOT the same as the FS_IMMUTABLE_FL flag.
> >
> > > And it would not solve this problem for local filesystems (ntfs or ext4)
> > > when Samba server or wine would want to set this bit.
> > >
> >
> > The Samba server would use the FS_IOC_FS[GS]ETXATTR ioctl
> > API to get/set dosattrib, something like this:
> >
> > struct fsxattr fsxattr;
> > ret = ioctl_get_fsxattr(fd, &fsxattr);
> > if (!ret && fsxattr.fsx_xflags & FS_XFLAG_HASDOSATTR) {
> >     fsxattr.fsx_dosattr |= fs_dosattrib_readonly;
> >     ret = ioctl_set_fsxattr(fd, &fsxattr);
> > }
>
> Thanks for more explanation. First time I really did not understood it.
> But now I think I understood it. So basically there would be two flags
> which would result in setting S_IMMUTABLE on inode. One is the existing
> FS_IMMUTABLE_FL which requires the capability and some new flag (e.g.
> FS_XFLAG_HASDOSATTR) which would not require it and can be implemented
> for cifs, vfat, ntfs, ... Right?
>

Well, almost right.
The flag that would correspond to FILE_ATTRIBUTE_READONLY
is FS_DOSATTRIB_READONLY from the new field fsx_dosattrib
(see below)

--- 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) */
+       unsigned char   fsx_pad[4];
 };

 /*
@@ -167,7 +168,16 @@ struct fsxattr {
 #define FS_XFLAG_FILESTREAM    0x00004000      /* use filestream allocator */
 #define FS_XFLAG_DAX           0x00008000      /* use DAX for IO */
 #define FS_XFLAG_COWEXTSIZE    0x00010000      /* CoW extent size
allocator hint */
-#define FS_XFLAG_HASATTR       0x80000000      /* no DIFLAG for this   */
+#define FS_XFLAG_HASATTR       0x80000000      /* has extended attributes */
+
+/*
+ * Flags for the fsx_dosattrib field
+ */
+#define FS_DOSATTRIB_READONLY  0x00000001      /* R - read-only */
+#define FS_DOSATTRIB_HIDDEN    0x00000002      /* H - hidden */
+#define FS_DOSATTRIB_SYSTEM    0x00000004      /* S - system */
+#define FS_DOSATTRIB_ARCHIVE   0x00000020      /* A - archive */
+#define FS_DOSATTRIB_HASATTR   0x80000000      /* has dos attributes */

This last special flag is debatable and I am not really sure that we need it.

It is needed for proper backward compat with existing userspace tools.
For example, if there was a backup tool storing the fsxattr blob result of
FS_IOC_FSGETXATTR and sets it later during restore with
FS_IOC_FSSETXATTR, then it would be better to ignore a zero
value of fsx_dosattrib instead of resetting all of the on-disk dosattrib flags
if the restore happens after ntfs gained support for setting dosattrib flags
via FS_IOC_FSSETXATTR.

When using the standard tools to set fsxattr (chattr and xfs_io -c chattr)
the tool does FS_IOC_FSGETXATTR + modify + FS_IOC_FSSETXATTR,
so those tools are expected to leave new bits in fsx_dosattrib at their
value if ntfs gains support for get/set fsx_dosattrib.

Setting the auxiliary FS_DOSATTRIB_HASATTR flag can help the
kernel/fs to explicitly state that the values returned in fsx_dosattrib
are valid and the tool to state that values set in fsx_dosattrib are valid.
But using a single flag will not help expanding ntfs support for more
fsx_dosattrib flags later, so I am not sure if it is useful (?).


> > For ntfs/ext4, you will need to implement on-disk support for
> > set/get the dosattrib flags.
>
> ntfs has already on-disk support for FILE_ATTRIBUTE_READONLY.
>

This is interesting.

fat/ntfs both already have a mount option sys_immutable to map
FILE_ATTRIBUTE_SYSTEM to S_IMMUTABLE in-memory.

fat does not support fileattr_set(), but has a proprietary ioctl
FAT_IOCTL_SET_ATTRIBUTES which enforces
CAP_LINUX_IMMUTABLE for changing S_IMMUTABLE.

ntfs also maps FILE_ATTRIBUTE_SYSTEM to S_IMMUTABLE
and it allows changing FILE_ATTRIBUTE_SYSTEM via ntfs_setxattr
of system.{dos,ntfs}_attrib without enforcing CAP_LINUX_IMMUTABLE,
or any other permissions at all (?)
This does not change S_IMMUTABLE in-memory, so change will
only apply on the next time inode is loaded from disk.
Bottom line: seems like *any user at all* can change the READONLY
and SYSTEM attributes on ntfs.

OTOH, ntfs does support fileattr_set() - it allows changing
S_IMMUTABLE and S_APPEND in-memory, but as far as I can
tell, this change is not stored on-disk (?).

Also in ntfs, FILE_ATTRIBUTE_READONLY is mapped
to not having posix write permissions on-disk:
                /* Linux 'w' -> Windows 'ro'. */
                if (0222 & inode->i_mode)
                        ni->std_fa &= ~FILE_ATTRIBUTE_READONLY;
                else
                        ni->std_fa |= FILE_ATTRIBUTE_READONLY;

So for ntfs, S_IMMUTABLE could be updated depending on three
independent flags: SYSTEM, READONLY and  FS_XFLAG_IMMUTABLE.

Having ntfs treat FILE_ATTRIBUTE_READONLY as S_IMMUTABLE
internally, is completely confined to ntfs and has nothing to do with vfs
or with a new standard API.

> On-disk support for ext4 and other linux filesystems can be discussed
> later. I think that this could be more controversial.
>

Obviously there are existing users that need this.
Samba has its own xattr user.DOSATTRIB and if people really want
to be able to export those attributes in a standard way, I doubt there
will be objection to adding on-disk support (e.g. to ext4/xfs).
But somebody has to do the work and adding new on-disk support
is not so easy.

I can help with that when the time comes.
First thing first, try to propose patches to extend fsx_dosattrib and
support them in ntfs/fat/smb.

> > I can certainly not change the meaning of existing on-disk
> > flag of FS_IMMUTABLE_FL to a flag that can be removed
> > without CAP_LINUX_IMMUTABLE. that changes the meaning
> > of the flag.
> >
> > If ext4 maintainers agrees, you may be able to reuse some
> > old unused on-disk flags (e.g.  EXT4_UNRM_FL) as storage
> > place for FS_DOSATTRIB_READONLY, but that would be
> > quite hackish.
> >
> > > > How about tackling this one small step at a time, not in that order
> > > > necessarily:
> > > >
> > > > 1. Implement the standard API with FS_IOC_FS[GS]ETXATTR ioctl
> > > >     and with statx to get/set some non-controversial dosattrib flags on
> > > >     ntfs/smb/vfat
> > > > 2. Wire some interesting dosattrib flags (e.g. compr/enrypt) to local
> > > >     filesystems that already support storing those bits
> > > > 3. Wire network servers (e.g. Samba) to use the generic API if supported
> > > > 4. Add on-disk support for storing the dosattrib flags to more local fs
> > > > 5. Update S_IMMUTABLE inode flag if either FS_XFLAG_IMMUTABLE
> > > >     or FS_DOSATTRIB_READONLY are set on the file
> > > >
> > > > Thoughts?
> > > >
> >
> > Anything wrong with the plan above?
> > It seems that you are looking for shortcuts and I don't think that
> > it is a good way to make progress.
> >
> > Thanks,
> > Amir.
>
> If other developers agree that the FS_IOC_FS[GS]ETXATTR ioctl is the
> right direction then for me it looks good.

This thread has been going on for a while.
I did not see any objections to this idea that Darrick proposed,
so I think next step for you is to post patches, because some
developers will only engage when there are patches to discuss.

Thanks,
Amir.