On Sun, May 14, 2023 at 10:09 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > 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@xxxxxxxxx/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. > What may confuse you is that Alexander has a tool mkfs.composefs that creates hand crafted overlayfs, but it is not up to overlayfs.rst to document this tool. This patch set and documentation of the feature are standalone to overlayfs and they affect and explain standard user workloads. A "metacopy file" is one that has metadata changes in upper layer but the data is still read from the lower layer. The classic example, for which metacopy feature was developed is chwon -R on a mounted overlay without having to copy up the data. I agree that it would be good to clarify this term before using it. With mount options metacopy=on,verity=on, once a file's metadata was copied up, and its owner changed for the sake of the story, the lower data file cannot be replaced with another file with different data or without fsverity, while overlayfs is offline. > I think that it would be easier to understand and review this feature if the > documentation was gotten up to date. > Those are all valid comments, but I don't think it would be fair to require Alexander to fix the out of date documentation of overlayfs. IMO, the documentation of verity feature should be judged for its own clarity (and I myself find it pretty clear) assuming that the reader is familiar with overlayfs and metacopy, but I will surely not object if Alexander wants to help improve overlayfs.rst. Thanks, Amir.
