Re: [PATCH v6 2/3] btrfs: initial fsverity support

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Sep 15, 2021 at 02:01:12PM -0700, Boris Burkov wrote:
> On Wed, Sep 15, 2021 at 01:45:23PM -0700, Eric Biggers wrote:
> > On Tue, Sep 14, 2021 at 11:34:29AM -0700, Boris Burkov wrote:
> > > > Okay, so it is used.  (Due to the macro, it didn't show up when grepping.)
> > > > 
> > > > Doesn't it defeat the purpose of a ro_compat inode flag if the whole filesystem
> > > > is marked with a ro_compat feature flag, though?  I thought that the point of
> > > > the ro_compat inode flag is to allow old kernels to mount the filesystem
> > > > read-write, with only verity files being forced to read-only.  That would be
> > > > more flexible than ext4's implementation of fs-verity which forces the whole
> > > > filesystem to read-only.  But it seems you're forcing the whole filesystem to
> > > > read-only anyway?
> > > > 
> > > > - Eric
> > > 
> > > I was thinking of it in terms of "RO compat is the goal" and having new
> > > inode flags totally broke that and was treated as a corruption of the
> > > inode regardless of the fs being ro/rw. I think a check on a live fs
> > > would just flip the fs ro, which was the goal anyway, but a check that
> > > happened during mount would fail the mount, even for a read-only fs. 
> > > 
> > > Making it fully per file would be pretty cool! The only thing
> > > really missing as far as I can tell is a way to mark a file read only
> > > with the same semantics fsverity uses from within btrfs.
> > 
> > I don't understand.  Why are you bothering with the ro_compat inode flag at all
> > if it doesn't actually work?
> > 
> > - Eric
> 
> Sorry I explained that really badly.
> 
> My first try was ro-compat bit only, that failed because btrfs couldn't
> add an inode flag in a ro-compat way before my changes, as it could
> fail to mount.
> 
> To fix that, I had to work on the inode flag compatibility, which
> evolved into this notion of inode ro-compat flags, which does work as
> expected: if you see a file with an unknown ro-compat flag it's an error
> if you aren't read-only. Read-only mount will never fail.
> 
> I think changing the semantics of the ro-compat inodes from:
> "an unknown ro inode flag -> fs ro" to
> "an unknown ro inode flag -> file ro"
> could be a big win. I don't think there is a rush to do that, though?

If you're forcing the filesystem to read-only anyway, why not just rely on the
filesystem-wide ro_compat flag, which you already implemented and which already
does that?  What benefit does the per-file ro_compat flag have, if it doesn't
actually make just the file read-only (which would be the expected behavior)?
You might as well just use a "regular" inode flag in that case.

- Eric