Re: [PATCH v2 0/6] ovl: Add support for fs-verity checking of lowerdata

[Eric Biggers <ebiggers@kernel.org>]

Hi Alexander,

On Wed, May 03, 2023 at 10:51:33AM +0200, Alexander Larsson wrote:
> This patchset adds support for using fs-verity to validate lowerdata
> files by specifying an overlay.verity xattr on the metacopy
> files.
> 
> This is primarily motivated by the Composefs usecase, where there will
> be a read-only EROFS layer that contains redirect into a base data
> layer which has fs-verity enabled on all files. However, it is also
> useful in general if you want to ensure that the lowerdata files
> matches the expected content over time.
> 
> This patch series is based on the lazy lowerdata patch series by Amir[1].
> 
> I have also added some tests for this feature to xfstests[2].
> 
> I'm also CC:ing the fsverity list and maintainers because there is one
> (tiny) fsverity change, and there may be interest in this usecase.
> 
> Changes since v1:
>  * Rebased on v2 lazy lowerdata series
>  * Dropped the "validate" mount option variant. We now only support
>    "off", "on" and "require", where "off" is the default.
>  * We now store the digest algorithm used in the overlay.verity xattr.
>  * Dropped ability to configure default verity options, as this could
>    cause problems moving layers between machines.
>  * We now properly resolve dependent mount options by automatically
>    enabling metacopy and redirect_dir if verity is on, or failing
>    if the specified options conflict.
>  * Streamlined and fixed the handling of creds in ovl_ensure_verity_loaded().
>  * Renamed new helpers from ovl_entry_path_ to ovl_e_path_
> 
> [1] https://lore.kernel.org/linux-unionfs/20230427130539.2798797-1-amir73il@gmail.com/T/#m3968bf64a31946e77bdba8e3d07688a34cf79982
> [2] https://github.com/alexlarsson/xfstests/commits/verity-tests
> 
> Alexander Larsson (6):
>   fsverity: Export fsverity_get_digest
>   ovl: Break out ovl_e_path_real() from ovl_i_path_real()
>   ovl: Break out ovl_e_path_lowerdata() from ovl_path_lowerdata()
>   ovl: Add framework for verity support
>   ovl: Validate verity xattr when resolving lowerdata
>   ovl: Handle verity during copy-up
> 
>  Documentation/filesystems/overlayfs.rst |  27 ++++
>  fs/overlayfs/copy_up.c                  |  31 +++++
>  fs/overlayfs/namei.c                    |  42 +++++-
>  fs/overlayfs/overlayfs.h                |  12 ++
>  fs/overlayfs/ovl_entry.h                |   3 +
>  fs/overlayfs/super.c                    |  74 ++++++++++-
>  fs/overlayfs/util.c                     | 165 ++++++++++++++++++++++--
>  fs/verity/measure.c                     |   1 +
>  8 files changed, 343 insertions(+), 12 deletions(-)

Thanks for presenting this topic at LSFMM!

I'm not an expert in overlayfs, but I've been working through this patchset.

One thing that seems to be missing, and has been tripping me up while reviewing
this patchset, is that the overlayfs documentation
(Documentation/filesystems/overlayfs.rst) is not properly up to date with the
use case that is intended here.

For example, the overlayfs documentation says "An overlay filesystem combines
two filesystems - an 'upper' filesystem and a 'lower' filesystem.".

Apparently, that is out of date.  I think a correct statement would be: An
overlay filesystem combines an optional upper directory with one or more lower
directories.

And as I understand it, the use case here actually involves two lower
directories and no upper directory.

There is also the "metacopy" feature, which the documentation describes in the
section "Metadata only copy up".  The documentation makes it sound like an
overlayfs internal optimization.

However, when this patchset introduces the fsverity support, it talks about
"metacopy files".  As I understand it, the user is expected to create a
read-only filesystem that contains these "metacopy files".  It doesn't seem to
be documented what these are, exactly, and how to create them.  I assume that
these are part of the implementation of "Metadata only copy up", but there seems
to be a gap where the documentation goes from describing the behavior of
"metadata only copy up", to expecting users of overlayfs to know what a
"metacopy file" is and how to create them.

I think that it would be easier to understand and review this feature if the
documentation was gotten up to date.

- Eric