Message-ID: <CAOQ4uxgmc+p4-MtFp5WZGCE2DOzcTH22_5X0=phhsbEO9v57HQ@mail.gmail.com>
Date: Mon, 17 Feb 2025 09:27:02 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Pali Rohár <pali@...nel.org>, 
	"Darrick J. Wong" <djwong@...nel.org>, Theodore Tso <tytso@....edu>, Eric Biggers <ebiggers@...nel.org>
Cc: ronnie sahlberg <ronniesahlberg@...il.com>, Chuck Lever <chuck.lever@...cle.com>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Steve French <sfrench@...ba.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, linux-fsdevel@...r.kernel.org, 
	linux-cifs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/4] fs: Add FS_XFLAG_COMPRESSED & FS_XFLAG_ENCRYPTED
 for FS_IOC_FS[GS]ETXATTR API

On Sun, Feb 16, 2025 at 10:17 PM Pali Rohár <pali@...nel.org> wrote:
>
> On Sunday 16 February 2025 21:43:02 Amir Goldstein wrote:
> > On Sun, Feb 16, 2025 at 9:24 PM Pali Rohár <pali@...nel.org> wrote:
> > >
> > > On Sunday 16 February 2025 21:17:55 Amir Goldstein wrote:
> > > > On Sun, Feb 16, 2025 at 7:34 PM Eric Biggers <ebiggers@...nel.org> wrote:
> > > > >
> > > > > On Sun, Feb 16, 2025 at 05:40:26PM +0100, Pali Rohár wrote:
> > > > > > This allows to get or set FS_COMPR_FL and FS_ENCRYPT_FL bits via FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR API.
> > > > > >
> > > > > > Signed-off-by: Pali Rohár <pali@...nel.org>
> > > > >
> > > > > Does this really allow setting FS_ENCRYPT_FL via FS_IOC_FSSETXATTR, and how does
> > > > > this interact with the existing fscrypt support in ext4, f2fs, ubifs, and ceph
> > > > > which use that flag?
> > > >
> > > > As far as I can tell, after fileattr_fill_xflags() call in
> > > > ioctl_fssetxattr(), the call
> > > > to ext4_fileattr_set() should behave exactly the same if it came some
> > > > FS_IOC_FSSETXATTR or from FS_IOC_SETFLAGS.
> > > > IOW, EXT4_FL_USER_MODIFIABLE mask will still apply.
> > > >
> > > > However, unlike the legacy API, we now have an opportunity to deal with
> > > > EXT4_FL_USER_MODIFIABLE better than this:
> > > >         /*
> > > >          * chattr(1) grabs flags via GETFLAGS, modifies the result and
> > > >          * passes that to SETFLAGS. So we cannot easily make SETFLAGS
> > > >          * more restrictive than just silently masking off visible but
> > > >          * not settable flags as we always did.
> > > >          */
> > > >
> > > > if we have the xflags_mask in the new API (not only the xflags) then
> > > > chattr(1) can set EXT4_FL_USER_MODIFIABLE in xflags_mask
> > > > ext4_fileattr_set() can verify that
> > > > (xflags_mask & ~EXT4_FL_USER_MODIFIABLE == 0).
> > > >
> > > > However, Pali, this is an important point that your RFC did not follow -
> > > > AFAICT, the current kernel code of ext4_fileattr_set() and xfs_fileattr_set()
> > > > (and other fs) does not return any error for unknown xflags, it just
> > > > ignores them.
> > > >
> > > > This is why a new ioctl pair FS_IOC_[GS]ETFSXATTR2 is needed IMO
> > > > before adding support to ANY new xflags, whether they are mapped to
> > > > existing flags like in this patch or are completely new xflags.
> > > >
> > > > Thanks,
> > > > Amir.
> > >
> > > But xflags_mask is available in this new API. It is available if the
> > > FS_XFLAG_HASEXTFIELDS flag is set. So I think that the ext4 improvement
> > > mentioned above can be included into this new API.
> > >
> > > Or I'm missing something?
> >
> > Yes, you are missing something very fundamental to backward compat API -
> > You cannot change the existing kernels.
> >
> > You should ask yourself one question:
> > What happens if I execute the old ioctl FS_IOC_FSSETXATTR
> > on an existing old kernel with the new extended flags?
> >
> > The answer, to the best of my code emulation abilities is that
> > old kernel will ignore the new xflags including FS_XFLAG_HASEXTFIELDS
> > and this is suboptimal, because it would be better for the new chattr tool
> > to get -EINVAL when trying to set new xflags and mask on an old kernel.
> >
> > It is true that the new chattr can call the old FS_IOC_FSGETXATTR
> > ioctl and see that it has no FS_XFLAG_HASEXTFIELDS,
>
> Yes, this was my intention how the backward and forward compatibility
> will work. I thought that reusing existing IOCTL is better than creating
> new IOCTL and duplicating functionality.
>
> > so I agree that a new ioctl is not absolutely necessary,
> > but I still believe that it is a better API design.
>
> If it is a bad idea then for sure I can prepare new IOCTL and move all
> new functionality only into the new IOCTL, no problem.
>

Well, there is at least one flaw in using the get ioctl for support detection,
as Eric pointed out -
the settable xflags set is a subset of the gettable flags set.

Let's ask some stake holders shall we?

Ted, Darrick, Eric,

What is your opinion on the plan presented in this patch set to extend the API:

1. Add some of the *_FL flags to FS_XFLAG_COMMON [*]
2. Add fsx_xflags2 for more xflags
3. Add fsx_xflags{,2}_mask to declare the supported in/out xflags
4. Should we use the existing FS_IOC_FSSETXATTR ioctl which ignores
   setting unknown flags or define a new v2 ioctl FS_IOC_SETFSXATTR_V2
   which properly fails on unknown flags [**]

[*] we can consider adding all of the flags used by actively maintained fs to
    FS_XFLAGS_COMMON, something like the set of F2FS_GETTABLE_FS_FL,
    maybe even split them to FS_XFLAGS_COMMON_[GS]ETTABLE
[**] we can either return EINVAL for unknown flags or make the ioctl _IOWR
     and return the set of flags that were not ignored

Thanks,
Amir.

Powered by blists - more mailing lists