On Tue, Jun 13, 2023 at 8:37 AM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > On Tue, Jun 13, 2023 at 08:18:50AM +0300, Amir Goldstein wrote: > > On Mon, Jun 12, 2023 at 7:32 PM Eric Biggers <ebiggers@xxxxxxxxxx> wrote: > > > > > > On Mon, Jun 12, 2023 at 12:27:17PM +0200, Alexander Larsson wrote: > > > > +fs-verity support > > > > +---------------------- > > > > + > > > > +When metadata copy up is used for a file, then the xattr > > > > +"trusted.overlay.verity" may be set on the metacopy file. This > > > > +specifies the expected fs-verity digest of the lowerdata file. This > > > > +may then be used to verify the content of the source file at the time > > > > +the file is opened. During metacopy copy up overlayfs can also set > > > > +this xattr. > > > > + > > > > +This is controlled by the "verity" mount option, which supports > > > > +these values: > > > > + > > > > +- "off": > > > > + The verity xattr is never used. This is the default if verity > > > > + option is not specified. > > > > +- "on": > > > > + Whenever a metacopy files specifies an expected digest, the > > > > + corresponding data file must match the specified digest. > > > > + When generating a metacopy file the verity xattr will be set > > > > + from the source file fs-verity digest (if it has one). > > > > +- "require": > > > > + Same as "on", but additionally all metacopy files must specify a > > > > + verity xattr. This means metadata copy up will only be used if > > > > + the data file has fs-verity enabled, otherwise a full copy-up is > > > > + used. > > > > > > It looks like my request for improved documentation was not taken, which is > > > unfortunate and makes this patchset difficult to review. > > > > > > > Which one? > > IIRC, you had two requests. > > One very broad to get the overlayfs.rst document up-to-date: > > [1] https://lore.kernel.org/linux-unionfs/20230514190903.GC9528@sol.localdomain/ > > That isn't an accurate summary of what I said. I actually pointed out two > specific things that are confusing specifically in the context of this feature. > > > But I assume you mean the specific request about this sentence: > > [2] https://lore.kernel.org/linux-unionfs/20230514192227.GE9528@sol.localdomain/ > > And that was a third specific thing. I got a detailed response back > (https://lore.kernel.org/linux-unionfs/CAL7ro1GGAfdZG9cHDWE2vnhY5tSE=9MxYi_n_gJHRfaw7zMSgg@xxxxxxxxxxxxxx), > which was helpful. Unfortunately, the information in that response hasn't yet > found its way into the documentation that is being proposed. > > In general the proposed documentation reads like the audience is overlayfs > developers. It doesn't describe the motivation for the feature or how to use it > in each of the two use cases. Maybe that is intended, but it's not what I had > expected to see. I think your complaint is mostly related to the scope/goal of the overlayfs.rst file. The way it is currently written, and how the patch adds to it, it describes purely how overlayfs works "natively". For example, when you change just the permissions of a file from the lower layer, then overlayfs generates a metacopy file with some special xattrs in the upper dir (rather than a full copy). It can also do things like set a redirect xattr on the metacopy file if the file changes name in the upper (due to a rename for example). With the patch it may (depending on options) also set a verity xattr on the metacopy file so that later uses of the upper layer can ensure that the lower layer content file hasn't changed. This allows you to improve the robustness of overlayfs layers, such that if at a later time the lower directories accidentally change we will detect this when using the metacopy file rather than delivering the wrong data. These kinds of uses are completely standalone, you just mount overlay with the right options and the kernel will do the right thing. There is no need to know the internal details of the xattrs. The problem is that I also have a different usecase with composefs. This involves a completely different way of using overlayfs where you construct manually a filesystem with the xattrs that overlayfs reads, and then you mount this in a very special way (using a read-only filesystem on loopback). A combination of overlayfs features will give certain properties that are useful. I feel like you expect that the overlayfs.rst document should describe the details of the composefs setup, but instead it (both the patch and the existing document) mainly describes the other kind of use. In other words, if you are interested in using overlayfs with verity to increase the robustness against layer changes, then the document is probably enough. However, If you're interested in using overlayfs to implement more complex features like composefs, then the document is very weak in all sorts of details, not limited to the fs-verity part. So, I don't think the current document is bad for what it does. But it should perhaps be amended with a more detailed description of the internals of the xattrs and their behaviour so that advanced users (i.e. developers) don't have to read the source code for such details. That seems like a general critique though, and not necessarily related to this particular patchset. > Side note: the use of the passive voice, e.g. "the xattr may be set" and "the xattr may then be used to verify", should be avoided since it makes it unclear who/what is doing these actions. I'm not a native english speaker so I'm sure this can be written in a better way, but when it says: When metadata copy up is used for a file, then the xattr "trusted.overlay.verity" may be set on the metacopy file. It really means: In certain circumstances, depending on mount options and other details, it may be the case that when a metacopy file is being created by overlayfs (as a side effect of a filesystem operation) the xattr "trusted.overlay.verity" will be set on the new metacopy file. So, this is not really using "may" in the passive voice, but I see that it can be read that way. I'll try to reword this in a way that is more precise. -- =-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-= Alexander Larsson Red Hat, Inc alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
