Date:   Tue, 18 Jun 2019 09:31:18 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Theodore Ts'o <tytso@....edu>
Cc:     linux-fscrypt@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net,
        linux-fsdevel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-integrity@...r.kernel.org, Jaegeuk Kim <jaegeuk@...nel.org>,
        Victor Hsieh <victorhsieh@...gle.com>,
        Dave Chinner <david@...morbit.com>,
        Christoph Hellwig <hch@....de>,
        "Darrick J . Wong" <darrick.wong@...cle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v4 01/16] fs-verity: add a documentation file

Hi Ted,

On Sat, Jun 15, 2019 at 08:39:20AM -0400, Theodore Ts'o wrote:
> On Thu, Jun 06, 2019 at 08:51:50AM -0700, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@...gle.com>
> > 
> > Add a documentation file for fs-verity, covering....
> > 
> > Signed-off-by: Eric Biggers <ebiggers@...gle.com>
> 
> Looks good; you can add:
> 
> Reviewed-by: Theodore Ts'o <tytso@....edu>
> 
> 
> One minor design point below:
> 
> > +ext4 stores the verity metadata (Merkle tree and fsverity_descriptor)
> > +past the end of the file, starting at the first page fully beyond
>                                                    ^^^^
> > +i_size.  This approach works because (a) verity files are readonly,
> > +and (b) pages fully beyond i_size aren't visible to userspace but can
> > +be read/written internally by ext4 with only some relatively small
> > +changes to ext4.  This approach avoids having to depend on the
> > +EA_INODE feature and on rearchitecturing ext4's xattr support to
> > +support paging multi-gigabyte xattrs into memory, and to support
> > +encrypting xattrs.  Note that the verity metadata *must* be encrypted
> > +when the file is, since it contains hashes of the plaintext data.
> 
> If we ever want to support mounting, say, a file system with 4k blocks
> and fsverity enabled on a architecture with a 16k or 64k page size,
> then "page" in that first sentence will need to become "block".  At
> the moment we only support fsverity when page size == block size, so
> it's not an issue.
> 
> However, it's worth reflecting on what this means.  In order to
> satisfy this requirement (from the mmap man page):
> 
>        A file is mapped in multiples of the page size.  For a file
>        that is not a multiple of the page size, the remaining memory
>        is zeroed when mapped...
> 
> we're going to have to special case how the last page gets mmaped.
> The simplest way to do this will be to map in an anonymous page which
> just has the blocks that are part of the data block copied in, and the
> rest of the page can be zero'ed.
> 
> One thing we might consider doing just to make life much easier for
> ourselves (should we ever want to support page size != block size ---
> which I could imagine some folks like Chandan might find desirable) is
> to specify that the fsverity metadata begins at an offset which begins
> at i_size rounded up to the next 64k binary, which should handle all
> current and future architectures' page sizes.
> 

Thanks for the review.  Good point; I think we should just go with the "always
round up to the next 64K boundary" method.  Special-casing how the last page
gets mmap()ed seems it would be really painful.

Since there can be a hole between the end of the file and the start of the
verity metadata, this doesn't even necessarily use any additional disk space.

For consistency and since there is little downside I think I'll do the same for
f2fs too, though f2fs doesn't currently support PAGE_SIZE != 4096 at all anyway.

- Eric

Powered by blists - more mailing lists