On Mon, Jun 12, 2023 at 1:33 PM Alexander Larsson <alexl@xxxxxxxxxx> wrote: > > On Sun, Jun 11, 2023 at 1:20 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Sat, Jun 10, 2023 at 6:02 PM Alexander Larsson <alexl@xxxxxxxxxx> wrote: > > > > > > On Fri, Jun 9, 2023 at 3:03 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > > > On Mon, May 15, 2023 at 9:15 AM Alexander Larsson <alexl@xxxxxxxxxx> wrote: > > > > > > > > > > On Sun, May 14, 2023 at 11:00 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > > > > > > > On Sun, May 14, 2023 at 10:16 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > On Wed, May 03, 2023 at 10:51:38AM +0200, Alexander Larsson wrote: > > > > > > > > When resolving lowerdata (lazily or non-lazily) we check the > > > > > > > > overlay.verity xattr on the metadata inode, and if set verify that the > > > > > > > > source lowerdata inode matches it (according to the verity options > > > > > > > > enabled). > > > > > > > > > > > > > > Keep in mind that the lifetime of an inode's fsverity digest is from when it is > > > > > > > first opened to when the inode is evicted from the inode cache. > > > > > > > > > > > > > > If the inode gets evicted from cache and re-instantiated, it could have been > > > > > > > arbitrarily changed. > > > > > > > > > > > > > > Given that, does this verification happen in the right place? I would have > > > > > > > expected it to happen whenever the file is opened, but it seems you do it when > > > > > > > the dentry is looked up instead. Maybe that works too, but I'd appreciate an > > > > > > > explanation. > > > > > > > > > > > > Hmm. I do not think it is wrong because the overlay file cannot be opened before > > > > > > the inode overlay is looked up and fsverity is verified on lookup. > > > > > > In theory, overlay inode with lower could have been instantiated by decode_fh(), > > > > > > but verity=on and nfs_export=on are conflicting options. > > > > > > > > > > > > However, I agree that doing verify check on lookup is a bit too early, as > > > > > > ls -lR will incur the overhead of verifying all file's data even > > > > > > though their data > > > > > > is not accessed in a non-lazy-lower-data scenario. > > > > > > > > > > > > The intuition of doing verity check before file is opened (or copied up) > > > > > > when there is a realfile open is not wrong, it would have gotten rid of the > > > > > > dodgy ovl_ensure_verity_loaded(), but I think that will be a bit harder to > > > > > > implement (not sure). > > > > > > > > > > > > My suggestion for Alexander: > > > > > > - Use ovl_set/test_flag(OVL_VERIFIED, inode) for lazy verify > > > > > > - Implement ovl_maybe_validate_verity() similar to > > > > > > ovl_maybe_lookup_lowerdata() > > > > > > - Implement a helper ovl_verify_lowerdata() > > > > > > that calls them both > > > > > > - Replace the ovl_maybe_lookup_lowerdata() calls with > > > > > > ovl_verify_lowerdata() calls > > > > > > > > > > > > Then before opening (or copy up) a file, it could have either > > > > > > lazy lower data lookup or lazy lower data validate or both (or none). > > > > > > > > > > > > This will not avoid ovl_ensure_verity_loaded(), but it will load fsverity > > > > > > just before it is needed and it is a bit easier to take ovl_inode_lock > > > > > > unconditionally, in those call sites then deeper within copy_up, where > > > > > > ovl_inode_lock is already taken. > > > > > > > > > > > > I *think* this is a good idea, but we won't know until you try it, > > > > > > so please take my suggestion with a grain of salt. > > > > > > > > > > I'll have a look at it in a bit. It would make performance of > > > > > verity=on in the non-lazy-lookup case better. > > > > > > > > > > > > > Hi Alex, > > > > > > > > Now that lazy lookup is queued for next, I wanted to ask about the > > > > status of your patches. > > > > > > > > Is the issue above the only thing you still need to look at? > > > > No rush on my end, just wanted to be in sync. > > > > > > I spent some time on friday doing the initial work on this rework. In > > > case you want to look at it I just pushed it to: > > > > > > https://github.com/alexlarsson/linux/tree/overlay-verity > > > > > > The first 3 commits are the same as before, and the last one is the > > > new approach. It's turned out pretty nice, simplifying things > > > considerable. > > > > It does look nicer :) > > I gave you a small review comment on github, but overall looks good. > > > > > But I really haven't had time to do a full re-review and > > > test of it, so please don't merge it yet. I'll spend Monday ensuring > > > it is in a good state for upstream. > > > > > > > When you are done testing, please post v3 patches. > > > > Note that I pushed a new version of ovl-lazy-lowerdata branch to > > fstests, with the minor :: syntax change. > > I posted the patches, and I also had to make some small changes to the > xfstest commit: > > https://github.com/alexlarsson/xfstests/commits/verity-tests > > > Not sure if Miklos will have time to review them this cycle or wait > > for the next one. > > > > There are also the mount api changes from Christian that conflict > > with the new verify option. > > > > If we want to have all of these changes for this cycle, some collaboration > > will be required. > > I would really like to get the overlay.verity xattr support in > upstream pretty soon, because without it I can't release a stable > version of the composefs userspace code. I don't want to release > something that is using an xattr format that is not guaranteed to be > stable. > Alex, Pondering about this last sentence. The overlay.verity xattr format is already in its 3rd revision since the beginning of development. When it was bare digest, it might have made sense to have no header that describes the format. When the algo byte was added, that was already a very big hint that a proper header was in order. Now that you had to change the meaning of the byte, it is very hard to argue that the xattr format is guaranteed to be stable - in the sense that it can never change again. Please add a minimal header to the overlay.verity xattr format, similar to ovl_fb, that will allow composefs/overlayfs to be maintained as the separate projects that they are. Something like this? /* On-disk format for "verity" xattr */ struct ovl_verity { u8 version; /* 0 */ u8 len; /* size of this header + size of digest */ u8 flags; u8 algo; /* digest algo */ u8 digest[]; }; I realize that digest size may be inferred from xattr size, but it is better to state the stored digest size explicitly and verify that it matches expected xattr size. Eric, Does the digest buffer passed to fsnotify helpers have any memory alignment requirements? Thanks, Amir.
