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. 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. Thanks, Amir.
