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. -- =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= Alexander Larsson Red Hat, Inc alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
