| Message ID | 20180328181509.199870-1-ebiggers@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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
[+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
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.
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
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)
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(+)