Re: [PATCH] f2fs: reserve bits for fs-verity

Re: [PATCH] f2fs: reserve bits for fs-verity

[Eric Biggers]

[+Cc linux-ext4, linux-fsdevel]

On Mon, Apr 02, 2018 at 06:07:10PM +0800, Chao Yu wrote:
> Hi Eric and Jaegeuk,
> 
> On 2018/3/31 2:34, Eric Biggers wrote:
> > Hi Chao and Jaegeuk,
> > 
> > On Fri, Mar 30, 2018 at 09:41:36AM -0700, Jaegeuk Kim wrote:
> >> On 03/30, Chao Yu wrote:
> >>> Hi Eric,
> >>>
> >>> On 2018/3/29 2:15, Eric Biggers wrote:
> >>>> Reserve an F2FS feature flag and inode flag for fs-verity.  This is an
> >>>> in-development feature that is planned be discussed at LSF/MM 2018 [1].
> >>>> It will provide file-based integrity and authenticity for read-only
> >>>> files.  Most code will be in a filesystem-independent module, with
> >>>> smaller changes needed to individual filesystems that opt-in to
> >>>> supporting the feature.  An early prototype supporting F2FS is available
> >>>> [2].  Reserving the F2FS on-disk bits for fs-verity will prevent users
> >>>> of the prototype from conflicting with other new F2FS features.
> >>>>
> >>>> Note that we're reserving the inode flag in f2fs_inode.i_advise, which
> >>>> isn't really appropriate since it's not a hint or advice.  But
> >>>> ->i_advise is already being used to hold the 'encrypt' flag; and F2FS's
> >>>> ->i_flags uses the generic FS_* values, so it seems ->i_flags can't be
> >>>> used for an F2FS-specific flag without additional work to remove the
> >>>> assumption that ->i_flags uses the generic flags namespace.
> >>>
> >>> At a glance, this is a VFS feature, can we search free slot, and define
> >>> FS_VERITY_FL like other generic flags, so we can intergrate this flag into
> >>> f2fs_inode::i_flags?
> >>
> >> Do we need to get/set this bit of i_flags to user? And, f2fs doesn't synchronize
> >> it with inode block update. I think this should be set by internal f2fs
> >> operations likewise fscrypt.
> >>
> > 
> > The fs-verity inode flag won't be modifiable using FS_IOC_SETFLAGS.  Like
> 
> Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for FS_ENCRYPT_FL?
> 
> > fscrypt, it will only be possible to set it using a dedicated ioctl (tentatively
> > called FS_IOC_ENABLE_VERITY), and it won't be supported to clear the bit once
> > set, short of deleting and re-creating the file.  So it doesn't really matter
> > where the bit goes in the on-disk inode, it just needs to go somewhere.  I'm
> > just hesitant to reserve a flag in the UAPI flags namespace which is really more
> > "owned" by ext4 than by f2fs, so has more implications than just f2fs as we
> > would effectively be reserving the flag for ext4's on-disk format too.
> 
> IMO, because this is a VFS feature, it will be better that we can put it in more
> generic place, also user can check this bit in generic way (via
> FS_IOC_GETFLAGS), and then for other filesystem who wants add this feature, that
> will be simple to place this bit.
> 
> What I can see is, for encryption feature:
> 
> vfs::i_flags
> #define FS_ENCRYPT_FL                   0x00000800 /* Encrypted file */
> 
> ext4:i_flags
> #define EXT4_ENCRYPT_FL			0x00000800 /* encrypted file */
> 
> f2fs::i_advice
> #define FADVISE_ENCRYPT_BIT	0x04
> 
> It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit position,
> result in that we leave a hole in on-disk i_flags, and if we want to show the
> same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more codes.
> 
> Anyway, I just ask why not let generic status goes into i_flags, and private
> status goes into i_advices?

Ted and others, what would you say about allocating FS_VERITY_FL as one of the
unused ext4 / "VFS" inode flags like 0x01000000, or maybe 0x00000400 if the
compression flags aren't being used anymore?

I do wish that we separated the on-disk flags namespaces from the
FS_IOC_GETFLAGS/FS_IOC_SETFLAGS namespace though...  Then adding the flag to
each namespace could be done separately which would make more sense.  As-is, the
flags are all being conflated, so by allocating a flag in f2fs ->i_flags we
would effectively also be allocating it for ext4 and for the UAPI, which we
don't necessarily need to do yet.

> 
> > 
> > I do think the flag *should* go in i_flags rather than i_advise, but I think the
> > assumption that f2fs's inode flags namespace matches ext4's would first need to
> > be removed so as to not tie the on-disk formats of different filesystems
> > together.
> > 
> >>>
> >>> And how about applying this patch inside the patchset of new fsverity feature?
> >>> Since once fsverity feature has some design modification, I worry about that may
> >>> be we need to change this bit? result in disk layout incompatibility.
> >>
> >> The whole body is not fully mergeable, so once reserving the bits first, we can
> >> support it f2fs-tools and prepare the feature in advance. Based on previous
> >> fscrypt experience, I don't expect we need to modify or drop these dramatically
> >> later.
> >>
> >> Any thoughts?
> 
> Since I don't know current progress of this feature development, I hope this
> feature will not be against by vfs developers or suffer design change after we
> reserved bit for it. :)
> 
> >>
> > 
> > Yep, the full patchset isn't ready to be merged upstream yet.  Other parts of
> > the API/ABI such as the fs-verity metadata format, the ioctl numbers, and the
> > semantics of accessing such files is subject to change.  We know we'll need a
> > superblock feature flag and a per-inode bit in any case, though.  Personally I'd
> > prefer to wait for the full patchset too, but we have people who want to start
> > using the prototype of the feature already, so having f2fs-tools support the
> > feature flag and having the bits not conflict with other f2fs features will be
> > helpful.
> 
> Oh, so we want a stable on-disk layout, so image for experience will contain
> fsverity bit in stable position, after formal android released, image with
> fsverity feature can still be valid, right?
> 

The fs-verity file format is not finalized, but in any case there will need to
be a superblock flag and a per-inode flag that indicates whether it is enabled.
There will also be a version number built into the fs-verity metadata, so the
file format can be updated without having to change to using a different
per-inode flag.

Eric

Re: [PATCH] f2fs: reserve bits for fs-verity

[Dave Chinner]

On Mon, Apr 02, 2018 at 11:48:37AM -0700, Eric Biggers wrote:
> [+Cc linux-ext4, linux-fsdevel]
> 
> On Mon, Apr 02, 2018 at 06:07:10PM +0800, Chao Yu wrote:
> > Hi Eric and Jaegeuk,
> > 
> > On 2018/3/31 2:34, Eric Biggers wrote:
> > > Hi Chao and Jaegeuk,
> > > 
> > > On Fri, Mar 30, 2018 at 09:41:36AM -0700, Jaegeuk Kim wrote:
> > >> On 03/30, Chao Yu wrote:
> > >>> Hi Eric,
> > >>>
> > >>> On 2018/3/29 2:15, Eric Biggers wrote:
> > >>>> Reserve an F2FS feature flag and inode flag for fs-verity.  This is an
> > >>>> in-development feature that is planned be discussed at LSF/MM 2018 [1].
> > >>>> It will provide file-based integrity and authenticity for read-only
> > >>>> files.  Most code will be in a filesystem-independent module, with
> > >>>> smaller changes needed to individual filesystems that opt-in to
> > >>>> supporting the feature.  An early prototype supporting F2FS is available
> > >>>> [2].  Reserving the F2FS on-disk bits for fs-verity will prevent users
> > >>>> of the prototype from conflicting with other new F2FS features.
> > >>>>
> > >>>> Note that we're reserving the inode flag in f2fs_inode.i_advise, which
> > >>>> isn't really appropriate since it's not a hint or advice.  But
> > >>>> ->i_advise is already being used to hold the 'encrypt' flag; and F2FS's
> > >>>> ->i_flags uses the generic FS_* values, so it seems ->i_flags can't be
> > >>>> used for an F2FS-specific flag without additional work to remove the
> > >>>> assumption that ->i_flags uses the generic flags namespace.
> > >>>
> > >>> At a glance, this is a VFS feature, can we search free slot, and define
> > >>> FS_VERITY_FL like other generic flags, so we can intergrate this flag into
> > >>> f2fs_inode::i_flags?
> > >>
> > >> Do we need to get/set this bit of i_flags to user? And, f2fs doesn't synchronize
> > >> it with inode block update. I think this should be set by internal f2fs
> > >> operations likewise fscrypt.
> > >>
> > > 
> > > The fs-verity inode flag won't be modifiable using FS_IOC_SETFLAGS.  Like
> > 
> > Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for FS_ENCRYPT_FL?
> > 
> > > fscrypt, it will only be possible to set it using a dedicated ioctl (tentatively
> > > called FS_IOC_ENABLE_VERITY), and it won't be supported to clear the bit once
> > > set, short of deleting and re-creating the file.  So it doesn't really matter
> > > where the bit goes in the on-disk inode, it just needs to go somewhere.  I'm
> > > just hesitant to reserve a flag in the UAPI flags namespace which is really more
> > > "owned" by ext4 than by f2fs, so has more implications than just f2fs as we
> > > would effectively be reserving the flag for ext4's on-disk format too.
> > 
> > IMO, because this is a VFS feature, it will be better that we can put it in more
> > generic place, also user can check this bit in generic way (via
> > FS_IOC_GETFLAGS), and then for other filesystem who wants add this feature, that
> > will be simple to place this bit.
> > 
> > What I can see is, for encryption feature:
> > 
> > vfs::i_flags
> > #define FS_ENCRYPT_FL                   0x00000800 /* Encrypted file */
> > 
> > ext4:i_flags
> > #define EXT4_ENCRYPT_FL			0x00000800 /* encrypted file */
> > 
> > f2fs::i_advice
> > #define FADVISE_ENCRYPT_BIT	0x04
> > 
> > It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit position,
> > result in that we leave a hole in on-disk i_flags, and if we want to show the
> > same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more codes.
> > 
> > Anyway, I just ask why not let generic status goes into i_flags, and private
> > status goes into i_advices?
> 
> Ted and others, what would you say about allocating FS_VERITY_FL as one of the
> unused ext4 / "VFS" inode flags like 0x01000000, or maybe 0x00000400 if the
> compression flags aren't being used anymore?
> 
> I do wish that we separated the on-disk flags namespaces from the
> FS_IOC_GETFLAGS/FS_IOC_SETFLAGS namespace though...  Then adding the flag to

Use FS_IOC_{GS}ETXATTR instead?. I'ts not tied to an on-disk format.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] f2fs: reserve bits for fs-verity

[Eric Biggers]

On Tue, Apr 03, 2018 at 07:49:59AM +1000, Dave Chinner wrote:
> On Mon, Apr 02, 2018 at 11:48:37AM -0700, Eric Biggers wrote:
> > [+Cc linux-ext4, linux-fsdevel]
> > 
> > On Mon, Apr 02, 2018 at 06:07:10PM +0800, Chao Yu wrote:
> > > Hi Eric and Jaegeuk,
> > > 
> > > On 2018/3/31 2:34, Eric Biggers wrote:
> > > > Hi Chao and Jaegeuk,
> > > > 
> > > > On Fri, Mar 30, 2018 at 09:41:36AM -0700, Jaegeuk Kim wrote:
> > > >> On 03/30, Chao Yu wrote:
> > > >>> Hi Eric,
> > > >>>
> > > >>> On 2018/3/29 2:15, Eric Biggers wrote:
> > > >>>> Reserve an F2FS feature flag and inode flag for fs-verity.  This is an
> > > >>>> in-development feature that is planned be discussed at LSF/MM 2018 [1].
> > > >>>> It will provide file-based integrity and authenticity for read-only
> > > >>>> files.  Most code will be in a filesystem-independent module, with
> > > >>>> smaller changes needed to individual filesystems that opt-in to
> > > >>>> supporting the feature.  An early prototype supporting F2FS is available
> > > >>>> [2].  Reserving the F2FS on-disk bits for fs-verity will prevent users
> > > >>>> of the prototype from conflicting with other new F2FS features.
> > > >>>>
> > > >>>> Note that we're reserving the inode flag in f2fs_inode.i_advise, which
> > > >>>> isn't really appropriate since it's not a hint or advice.  But
> > > >>>> ->i_advise is already being used to hold the 'encrypt' flag; and F2FS's
> > > >>>> ->i_flags uses the generic FS_* values, so it seems ->i_flags can't be
> > > >>>> used for an F2FS-specific flag without additional work to remove the
> > > >>>> assumption that ->i_flags uses the generic flags namespace.
> > > >>>
> > > >>> At a glance, this is a VFS feature, can we search free slot, and define
> > > >>> FS_VERITY_FL like other generic flags, so we can intergrate this flag into
> > > >>> f2fs_inode::i_flags?
> > > >>
> > > >> Do we need to get/set this bit of i_flags to user? And, f2fs doesn't synchronize
> > > >> it with inode block update. I think this should be set by internal f2fs
> > > >> operations likewise fscrypt.
> > > >>
> > > > 
> > > > The fs-verity inode flag won't be modifiable using FS_IOC_SETFLAGS.  Like
> > > 
> > > Verity flag can also be wrapped by FS_FL_USER_MODIFIABLE like for FS_ENCRYPT_FL?
> > > 
> > > > fscrypt, it will only be possible to set it using a dedicated ioctl (tentatively
> > > > called FS_IOC_ENABLE_VERITY), and it won't be supported to clear the bit once
> > > > set, short of deleting and re-creating the file.  So it doesn't really matter
> > > > where the bit goes in the on-disk inode, it just needs to go somewhere.  I'm
> > > > just hesitant to reserve a flag in the UAPI flags namespace which is really more
> > > > "owned" by ext4 than by f2fs, so has more implications than just f2fs as we
> > > > would effectively be reserving the flag for ext4's on-disk format too.
> > > 
> > > IMO, because this is a VFS feature, it will be better that we can put it in more
> > > generic place, also user can check this bit in generic way (via
> > > FS_IOC_GETFLAGS), and then for other filesystem who wants add this feature, that
> > > will be simple to place this bit.
> > > 
> > > What I can see is, for encryption feature:
> > > 
> > > vfs::i_flags
> > > #define FS_ENCRYPT_FL                   0x00000800 /* Encrypted file */
> > > 
> > > ext4:i_flags
> > > #define EXT4_ENCRYPT_FL			0x00000800 /* encrypted file */
> > > 
> > > f2fs::i_advice
> > > #define FADVISE_ENCRYPT_BIT	0x04
> > > 
> > > It's very wired that f2fs didn't use well defined FS_ENCRYPT_FL bit position,
> > > result in that we leave a hole in on-disk i_flags, and if we want to show the
> > > same 'encrypted' flags status in FS_IOC_GETFLAGS, we need to change more codes.
> > > 
> > > Anyway, I just ask why not let generic status goes into i_flags, and private
> > > status goes into i_advices?
> > 
> > Ted and others, what would you say about allocating FS_VERITY_FL as one of the
> > unused ext4 / "VFS" inode flags like 0x01000000, or maybe 0x00000400 if the
> > compression flags aren't being used anymore?
> > 
> > I do wish that we separated the on-disk flags namespaces from the
> > FS_IOC_GETFLAGS/FS_IOC_SETFLAGS namespace though...  Then adding the flag to
> 
> Use FS_IOC_{GS}ETXATTR instead?. I'ts not tied to an on-disk format.
> 

Yes, that API is better -- though, *setting* the fs-verity bit will likely be
done through its own ioctl (tentatively called FS_IOC_ENABLE_VERITY) as it will
involve more work than simply flipping the bit.  So for *getting* it we maybe
should just add it as a statx attribute, and not use the GETFLAGS or GETXATTR
ioctls at all.

But right now the real problem is that f2fs is using the FS_* namespace for its
on-disk ->i_flags (other than a few in the f2fs-specific ->i_advise), which
gives the impression that all new f2fs on-disk flags need to be exposed through
FS_IOC_GETFLAGS/SETFLAGS, and also reserved for ext4 as well...

Eric