On Tue, Jun 13, 2023 at 9: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. This response is mostly about composefs and as Alex wrote these details have no place in overlayfs.rst and not related to the proposed feature, which stands alone even without composefs. > > In general the proposed documentation reads like the audience is overlayfs > developers. I never considered that overlayfs.rst is for an audience other than overlayfs developers or people that want to become overlayfs developers. It is not a user guide. If it were, it would have been a very bad one. > 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. > Yeh, that's a valid point. That is what I wanted to know - what exactly is missing. I guess this is the documented motivation: "This may then be used to verify the content of the source file at the time the file is opened" but it does not tell a complete chain of trust story. How about something along the lines of: "In the case that the upper layer can be trusted not to be tampered with while overlayfs is offline and some of the lower layers cannot be trusted not to be tampered with, the "verity" feature can protect against offline modification to lower files, whose metadata has been copied up to the upper layer (a.k.a "metacopy" files) ...." It's generic language that what the patches do, regardless of the trust model of composefs and how it composes an overlayfs layers. > 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 am not a native English speaker and bad at documentation in general, so the only thing I can say is the passive-aggressive "patches welcome" ;-) Thanks, Amir.
