Handing xfs fsverity development back to you

Handing xfs fsverity development back to you

[Darrick J. Wong]

Hi Andrey,

Yesterday during office hours I mentioned that I was going to hand the
xfs fsverity patchset back to you once I managed to get a clean fstests
run on my 6.10 tree.  I've finally gotten there, so I'm ready to
transfer control of this series back to you:

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fsverity_2024-06-12
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=fsverity_2024-06-12
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fsverity_2024-06-12

At this point, we have a mostly working implementation of fsverity
that's still based on your original design of stuffing merkle data into
special ATTR_VERITY extended attributes, and a lightweight buffer cache
for merkle data that can track verified status.  No contiguously
allocated bitmap required, etc.  At this point I've done all the design
and coding work that I care to do, EXCEPT:

Unfortunately, the v5.6 review produced a major design question that has
not been resolved, and that is the question of where to store the ondisk
merkle data.  Someone (was it hch?) pointed out that if xfs were to
store that fsverity data in some post-eof range of the file (ala
ext4/f2fs) then the xfs fsverity port wouldn't need the large number of
updates to fs/verity; and that a future xfs port to fscrypt could take
advantage of the encryption without needing to figure out how to encrypt
the verity xattrs.

On the other side of the fence, I'm guessing you and Dave are much more
in favor of the xattr method since that was (and still is) the original
design of the ondisk metadata.  I could be misremembering this, but I
think willy isn't a fan of the post-eof pagecache use either.

I don't have the expertise to make this decision because I don't know
enough (or anything) about cryptography to know just how difficult it
actually would be to get fscrypt to encrypt merkle tree data that's not
simply located in the posteof range of a file.  I'm aware that btrfs
uses the pagecache for caching merkle data but stores that data
elsewhere, and that they are contemplating an fscrypt implementation,
which is why Sweet Tea is on the cc list.  Any thoughts?

(This is totally separate from fscrypt'ing regular xattrs.)

If it's easy to adapt fscrypt to encrypt fsverity data stored in xattrs
then I think we can keep the current design of the patchset and try to
merge it for 6.11.  If not, then I think the rest of you need to think
hard about the tradeoffs and make a decision.  Either way, the depth of
my knowledge about this decision is limited to thinking that I have a
good enough idea about whom to cc.

Other notes about the branches I linked to:

I think it's safe to skip all the patches that mention disabling
fsverity because that's likely DOA anyway.

Christoph also has a patch to convert the other fsverity implementations
(btrfs/ext4/f2fs) to use the read/drop_merkle_tree_block interfaces:
https://lore.kernel.org/linux-xfs/ZjMZnxgFZ_X6c9aB@infradead.org/

I'm not sure if it actually handles PageChecked for the case that the
merkle tree block size != base page size.

If you prefer I can patchbomb the list with this v5.7 series.

--Darrick

Re: Handing xfs fsverity development back to you

[Andrey Albershteyn]

On 2024-06-12 12:06:44, Darrick J. Wong wrote:
> Hi Andrey,
> 
> Yesterday during office hours I mentioned that I was going to hand the
> xfs fsverity patchset back to you once I managed to get a clean fstests
> run on my 6.10 tree.  I've finally gotten there, so I'm ready to
> transfer control of this series back to you:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fsverity_2024-06-12
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=fsverity_2024-06-12
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fsverity_2024-06-12
> 
> At this point, we have a mostly working implementation of fsverity
> that's still based on your original design of stuffing merkle data into
> special ATTR_VERITY extended attributes, and a lightweight buffer cache
> for merkle data that can track verified status.  No contiguously
> allocated bitmap required, etc.  At this point I've done all the design
> and coding work that I care to do, EXCEPT:
> 
> Unfortunately, the v5.6 review produced a major design question that has
> not been resolved, and that is the question of where to store the ondisk
> merkle data.  Someone (was it hch?) pointed out that if xfs were to
> store that fsverity data in some post-eof range of the file (ala
> ext4/f2fs) then the xfs fsverity port wouldn't need the large number of
> updates to fs/verity; and that a future xfs port to fscrypt could take
> advantage of the encryption without needing to figure out how to encrypt
> the verity xattrs.
> 
> On the other side of the fence, I'm guessing you and Dave are much more
> in favor of the xattr method since that was (and still is) the original
> design of the ondisk metadata.  I could be misremembering this, but I
> think willy isn't a fan of the post-eof pagecache use either.
> 
> I don't have the expertise to make this decision because I don't know
> enough (or anything) about cryptography to know just how difficult it
> actually would be to get fscrypt to encrypt merkle tree data that's not
> simply located in the posteof range of a file.  I'm aware that btrfs
> uses the pagecache for caching merkle data but stores that data
> elsewhere, and that they are contemplating an fscrypt implementation,
> which is why Sweet Tea is on the cc list.  Any thoughts?
> 
> (This is totally separate from fscrypt'ing regular xattrs.)
> 
> If it's easy to adapt fscrypt to encrypt fsverity data stored in xattrs
> then I think we can keep the current design of the patchset and try to
> merge it for 6.11.  If not, then I think the rest of you need to think
> hard about the tradeoffs and make a decision.  Either way, the depth of
> my knowledge about this decision is limited to thinking that I have a
> good enough idea about whom to cc.
> 
> Other notes about the branches I linked to:
> 
> I think it's safe to skip all the patches that mention disabling
> fsverity because that's likely DOA anyway.
> 
> Christoph also has a patch to convert the other fsverity implementations
> (btrfs/ext4/f2fs) to use the read/drop_merkle_tree_block interfaces:
> https://lore.kernel.org/linux-xfs/ZjMZnxgFZ_X6c9aB@infradead.org/
> 
> I'm not sure if it actually handles PageChecked for the case that the
> merkle tree block size != base page size.
> 
> If you prefer I can patchbomb the list with this v5.7 series.
> 
> --Darrick
> 

Thanks, I will look into fscrypt and if it's feasible to make it
work with xattrs in XFS or not.

-- 
- Andrey

Re: Handing xfs fsverity development back to you

[Eric Biggers]

On Mon, Jun 17, 2024 at 11:34:13AM +0200, Andrey Albershteyn wrote:
> On 2024-06-12 12:06:44, Darrick J. Wong wrote:
> > Hi Andrey,
> > 
> > Yesterday during office hours I mentioned that I was going to hand the
> > xfs fsverity patchset back to you once I managed to get a clean fstests
> > run on my 6.10 tree.  I've finally gotten there, so I'm ready to
> > transfer control of this series back to you:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fsverity_2024-06-12
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=fsverity_2024-06-12
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fsverity_2024-06-12
> > 
> > At this point, we have a mostly working implementation of fsverity
> > that's still based on your original design of stuffing merkle data into
> > special ATTR_VERITY extended attributes, and a lightweight buffer cache
> > for merkle data that can track verified status.  No contiguously
> > allocated bitmap required, etc.  At this point I've done all the design
> > and coding work that I care to do, EXCEPT:
> > 
> > Unfortunately, the v5.6 review produced a major design question that has
> > not been resolved, and that is the question of where to store the ondisk
> > merkle data.  Someone (was it hch?) pointed out that if xfs were to
> > store that fsverity data in some post-eof range of the file (ala
> > ext4/f2fs) then the xfs fsverity port wouldn't need the large number of
> > updates to fs/verity; and that a future xfs port to fscrypt could take
> > advantage of the encryption without needing to figure out how to encrypt
> > the verity xattrs.
> > 
> > On the other side of the fence, I'm guessing you and Dave are much more
> > in favor of the xattr method since that was (and still is) the original
> > design of the ondisk metadata.  I could be misremembering this, but I
> > think willy isn't a fan of the post-eof pagecache use either.
> > 
> > I don't have the expertise to make this decision because I don't know
> > enough (or anything) about cryptography to know just how difficult it
> > actually would be to get fscrypt to encrypt merkle tree data that's not
> > simply located in the posteof range of a file.  I'm aware that btrfs
> > uses the pagecache for caching merkle data but stores that data
> > elsewhere, and that they are contemplating an fscrypt implementation,
> > which is why Sweet Tea is on the cc list.  Any thoughts?
> > 
> > (This is totally separate from fscrypt'ing regular xattrs.)
> > 
> > If it's easy to adapt fscrypt to encrypt fsverity data stored in xattrs
> > then I think we can keep the current design of the patchset and try to
> > merge it for 6.11.  If not, then I think the rest of you need to think
> > hard about the tradeoffs and make a decision.  Either way, the depth of
> > my knowledge about this decision is limited to thinking that I have a
> > good enough idea about whom to cc.

Assuming that you'd like to make the Merkle tree be totally separate from the
file contents stream (which would preclude encrypting it as part of that stream
even if it was stored separately), I think the logical way to encrypt it would
be to derive a Merkle tree encryption key for each file and encrypt the Merkle
tree using that key, using the same algorithm as file contents.  These days
fscrypt uses HKDF, so it's relatively straightforward to derive new keys.

> > 
> > Other notes about the branches I linked to:
> > 
> > I think it's safe to skip all the patches that mention disabling
> > fsverity because that's likely DOA anyway.
> > 
> > Christoph also has a patch to convert the other fsverity implementations
> > (btrfs/ext4/f2fs) to use the read/drop_merkle_tree_block interfaces:
> > https://lore.kernel.org/linux-xfs/ZjMZnxgFZ_X6c9aB@infradead.org/
> > 
> > I'm not sure if it actually handles PageChecked for the case that the
> > merkle tree block size != base page size.
> > 

Note that I worked on this more at
https://lore.kernel.org/fsverity/20240515015320.323443-1-ebiggers@kernel.org/

As I said: my reworked patch seems good to me, but it only makes sense to apply
it if XFS is going to use it.

Though, now I'm wondering if ->read_merkle_tree_block should hand back a (page,
offset) pair instead of a virtual address, and let fs/verity/ handle the
kmap_local_page() and kunmap_local().  Currently verify_data_block() does want
the kmap of each block to live as long as the reference to the block itself, for
up to FS_VERITY_MAX_LEVELS blocks at a time.  The virtual address based
interface works well for that.  But I realized recently that there's a slightly
more efficient way to implement verify_data_block() that would also have the
side effect of there being only one kmap needed at a time.

- Eric

Re: Handing xfs fsverity development back to you

[Darrick J. Wong]

On Thu, Jun 20, 2024 at 09:35:11PM -0700, Eric Biggers wrote:
> On Mon, Jun 17, 2024 at 11:34:13AM +0200, Andrey Albershteyn wrote:
> > On 2024-06-12 12:06:44, Darrick J. Wong wrote:
> > > Hi Andrey,
> > > 
> > > Yesterday during office hours I mentioned that I was going to hand the
> > > xfs fsverity patchset back to you once I managed to get a clean fstests
> > > run on my 6.10 tree.  I've finally gotten there, so I'm ready to
> > > transfer control of this series back to you:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=fsverity_2024-06-12
> > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=fsverity_2024-06-12
> > > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfstests-dev.git/log/?h=fsverity_2024-06-12
> > > 
> > > At this point, we have a mostly working implementation of fsverity
> > > that's still based on your original design of stuffing merkle data into
> > > special ATTR_VERITY extended attributes, and a lightweight buffer cache
> > > for merkle data that can track verified status.  No contiguously
> > > allocated bitmap required, etc.  At this point I've done all the design
> > > and coding work that I care to do, EXCEPT:
> > > 
> > > Unfortunately, the v5.6 review produced a major design question that has
> > > not been resolved, and that is the question of where to store the ondisk
> > > merkle data.  Someone (was it hch?) pointed out that if xfs were to
> > > store that fsverity data in some post-eof range of the file (ala
> > > ext4/f2fs) then the xfs fsverity port wouldn't need the large number of
> > > updates to fs/verity; and that a future xfs port to fscrypt could take
> > > advantage of the encryption without needing to figure out how to encrypt
> > > the verity xattrs.
> > > 
> > > On the other side of the fence, I'm guessing you and Dave are much more
> > > in favor of the xattr method since that was (and still is) the original
> > > design of the ondisk metadata.  I could be misremembering this, but I
> > > think willy isn't a fan of the post-eof pagecache use either.
> > > 
> > > I don't have the expertise to make this decision because I don't know
> > > enough (or anything) about cryptography to know just how difficult it
> > > actually would be to get fscrypt to encrypt merkle tree data that's not
> > > simply located in the posteof range of a file.  I'm aware that btrfs
> > > uses the pagecache for caching merkle data but stores that data
> > > elsewhere, and that they are contemplating an fscrypt implementation,
> > > which is why Sweet Tea is on the cc list.  Any thoughts?
> > > 
> > > (This is totally separate from fscrypt'ing regular xattrs.)
> > > 
> > > If it's easy to adapt fscrypt to encrypt fsverity data stored in xattrs
> > > then I think we can keep the current design of the patchset and try to
> > > merge it for 6.11.  If not, then I think the rest of you need to think
> > > hard about the tradeoffs and make a decision.  Either way, the depth of
> > > my knowledge about this decision is limited to thinking that I have a
> > > good enough idea about whom to cc.
> 
> Assuming that you'd like to make the Merkle tree be totally separate from the
> file contents stream (which would preclude encrypting it as part of that stream
> even if it was stored separately), I think the logical way to encrypt it would
> be to derive a Merkle tree encryption key for each file and encrypt the Merkle
> tree using that key, using the same algorithm as file contents.  These days
> fscrypt uses HKDF, so it's relatively straightforward to derive new keys.
> 
> > > 
> > > Other notes about the branches I linked to:
> > > 
> > > I think it's safe to skip all the patches that mention disabling
> > > fsverity because that's likely DOA anyway.
> > > 
> > > Christoph also has a patch to convert the other fsverity implementations
> > > (btrfs/ext4/f2fs) to use the read/drop_merkle_tree_block interfaces:
> > > https://lore.kernel.org/linux-xfs/ZjMZnxgFZ_X6c9aB@infradead.org/
> > > 
> > > I'm not sure if it actually handles PageChecked for the case that the
> > > merkle tree block size != base page size.
> > > 
> 
> Note that I worked on this more at
> https://lore.kernel.org/fsverity/20240515015320.323443-1-ebiggers@kernel.org/
> 
> As I said: my reworked patch seems good to me, but it only makes sense to apply
> it if XFS is going to use it.
> 
> Though, now I'm wondering if ->read_merkle_tree_block should hand back a (page,
> offset) pair instead of a virtual address, and let fs/verity/ handle the
> kmap_local_page() and kunmap_local().  Currently verify_data_block() does want
> the kmap of each block to live as long as the reference to the block itself, for
> up to FS_VERITY_MAX_LEVELS blocks at a time.  The virtual address based
> interface works well for that.  But I realized recently that there's a slightly
> more efficient way to implement verify_data_block() that would also have the
> side effect of there being only one kmap needed at a time.

(I'm only responding to the part for which I think I can answer
reasonably.)

That sounds like a good approach for ext4 which will likely be feeding
you folios from the pagecache anyway.  If Andrey sticks with the buffer
cache that I wrote, then the virtual address is a slab allocation which
is already mapped.

I guess you could return (folio, offset_in_folio, vaddr) and have
fs/verity do kmap_local if !vaddr.

--D

> 
> - Eric
> 

Re: Handing xfs fsverity development back to you

[Christoph Hellwig]

On Fri, Jun 21, 2024 at 10:27:20AM -0700, Darrick J. Wong wrote:
> That sounds like a good approach for ext4 which will likely be feeding
> you folios from the pagecache anyway.  If Andrey sticks with the buffer
> cache that I wrote, then the virtual address is a slab allocation which
> is already mapped.

... yet another reasons to not be pointleslly different code and create
special cases and extra code in every layer of the stack..