f2fs: reserve bits for fs-verity
diff mbox

Message ID 20180328181509.199870-1-ebiggers@google.com
State Not Applicable
Headers show

Commit Message

Eric Biggers March 28, 2018, 6:15 p.m. UTC
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.

[1] https://marc.info/?l=linux-fsdevel&m=151690752225644
[2] https://git.kernel.org/pub/scm/linux/kernel/git/mhalcrow/linux.git/log/?h=fs-verity-dev

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/f2fs.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Chao Yu March 30, 2018, 9:48 a.m. UTC | #1
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?

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.

Thanks,

> 
> [1] https://marc.info/?l=linux-fsdevel&m=151690752225644
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/mhalcrow/linux.git/log/?h=fs-verity-dev
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/f2fs/f2fs.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index ae69dc358401..96d7809c4541 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -146,6 +146,7 @@ struct f2fs_mount_info {
>  #define F2FS_FEATURE_QUOTA_INO		0x0080
>  #define F2FS_FEATURE_INODE_CRTIME	0x0100
>  #define F2FS_FEATURE_LOST_FOUND		0x0200
> +#define F2FS_FEATURE_VERITY		0x0400	/* reserved */
>  
>  #define F2FS_HAS_FEATURE(sb, mask)					\
>  	((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
> @@ -598,6 +599,7 @@ enum {
>  #define FADVISE_ENC_NAME_BIT	0x08
>  #define FADVISE_KEEP_SIZE_BIT	0x10
>  #define FADVISE_HOT_BIT		0x20
> +#define FADVISE_VERITY_BIT	0x40	/* reserved */
>  
>  #define file_is_cold(inode)	is_file(inode, FADVISE_COLD_BIT)
>  #define file_wrong_pino(inode)	is_file(inode, FADVISE_LOST_PINO_BIT)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jaegeuk Kim March 30, 2018, 4:41 p.m. UTC | #2
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.

> 
> 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?

> 
> Thanks,
> 
> > 
> > [1] https://marc.info/?l=linux-fsdevel&m=151690752225644
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/mhalcrow/linux.git/log/?h=fs-verity-dev
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  fs/f2fs/f2fs.h | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index ae69dc358401..96d7809c4541 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -146,6 +146,7 @@ struct f2fs_mount_info {
> >  #define F2FS_FEATURE_QUOTA_INO		0x0080
> >  #define F2FS_FEATURE_INODE_CRTIME	0x0100
> >  #define F2FS_FEATURE_LOST_FOUND		0x0200
> > +#define F2FS_FEATURE_VERITY		0x0400	/* reserved */
> >  
> >  #define F2FS_HAS_FEATURE(sb, mask)					\
> >  	((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
> > @@ -598,6 +599,7 @@ enum {
> >  #define FADVISE_ENC_NAME_BIT	0x08
> >  #define FADVISE_KEEP_SIZE_BIT	0x10
> >  #define FADVISE_HOT_BIT		0x20
> > +#define FADVISE_VERITY_BIT	0x40	/* reserved */
> >  
> >  #define file_is_cold(inode)	is_file(inode, FADVISE_COLD_BIT)
> >  #define file_wrong_pino(inode)	is_file(inode, FADVISE_LOST_PINO_BIT)
> > 
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers March 30, 2018, 6:34 p.m. UTC | #3
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
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.

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?
> 

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.

Thanks,

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chao Yu April 2, 2018, 10:07 a.m. UTC | #4
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?

> 
> 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?

Thanks,

> 
> Thanks,
> 
> Eric
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers April 2, 2018, 6:48 p.m. UTC | #5
[+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
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Chinner April 2, 2018, 9:49 p.m. UTC | #6
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.
Eric Biggers April 2, 2018, 10:58 p.m. UTC | #7
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
--
To unsubscribe from this list: send the line "unsubscribe linux-fscrypt" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index ae69dc358401..96d7809c4541 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -146,6 +146,7 @@  struct f2fs_mount_info {
 #define F2FS_FEATURE_QUOTA_INO		0x0080
 #define F2FS_FEATURE_INODE_CRTIME	0x0100
 #define F2FS_FEATURE_LOST_FOUND		0x0200
+#define F2FS_FEATURE_VERITY		0x0400	/* reserved */
 
 #define F2FS_HAS_FEATURE(sb, mask)					\
 	((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
@@ -598,6 +599,7 @@  enum {
 #define FADVISE_ENC_NAME_BIT	0x08
 #define FADVISE_KEEP_SIZE_BIT	0x10
 #define FADVISE_HOT_BIT		0x20
+#define FADVISE_VERITY_BIT	0x40	/* reserved */
 
 #define file_is_cold(inode)	is_file(inode, FADVISE_COLD_BIT)
 #define file_wrong_pino(inode)	is_file(inode, FADVISE_LOST_PINO_BIT)