Re: [PATCH 1/4] tune2fs: prevent changing UUID of fs with stable_inodes feature

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]


> On Apr 6, 2020, at 11:32 PM, Eric Biggers <ebiggers@kernel.org> wrote:
> 
> On Wed, Apr 01, 2020 at 08:19:38PM -0600, Andreas Dilger wrote:
>> On Apr 1, 2020, at 2:32 PM, Eric Biggers <ebiggers@kernel.org> wrote:
>>> 
>>> From: Eric Biggers <ebiggers@google.com>
>>> 
>>> The stable_inodes feature is intended to indicate that it's safe to use
>>> IV_INO_LBLK_64 encryption policies, where the encryption depends on the
>>> inode numbers and thus filesystem shrinking is not allowed.  However
>>> since inode numbers are not unique across filesystems, the encryption
>>> also depends on the filesystem UUID, and I missed that there is a
>>> supported way to change the filesystem UUID (tune2fs -U).
>>> 
>>> So, make 'tune2fs -U' report an error if stable_inodes is set.
>>> 
>>> We could add a separate stable_uuid feature flag, but it seems unlikely
>>> it would be useful enough on its own to warrant another flag.
>> 
>> What about having tune2fs walk the inode table checking for any inodes that
>> have this flag, and only refusing to clear the flag if it finds any?  That
>> takes some time on very large filesystems, but since inode table reading is
>> linear it is reasonable on most filesystems.
> 
> I assume you meant to make this comment on patch 2,
> "tune2fs: prevent stable_inodes feature from being cleared"?
> 
> It's a good suggestion, but it also applies equally to the encrypt, verity,
> extents, and ea_inode features.  Currently tune2fs can't clear any of these,
> since any inode might be using them.
> 
> Note that it would actually be slightly harder to implement your suggestion for
> stable_inodes than those four existing features, since clearing stable_inodes
> would require reading xattrs rather than just the inode flags.
> 
> So if I have time, I can certainly look into allowing tune2fs to clear the
> encrypt, verity, extents, stable_inodes, and ea_inode features, by doing an
> inode table scan to verify that it's safe.  IMO it doesn't make sense to hold up
> this patch on it, though.  This patch just makes stable_inodes work like other
> ext4 features.

Sure, I'm OK with this patch, since it avoids accidental breakage.

One question though - for the data checksums it uses s_checksum_seed to generate
checksums, rather than directly using the UUID itself, so that it *is* possible
to change the filesystem UUID after metadata_csum is in use, without the need
to rewrite all of the checksums in the filesystem.  Could the same be done for
stable_inode?

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]