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