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