[PATCH] tune2fs: don't change UUID on a mounted metadata_csum fs

[PATCH] tune2fs: don't change UUID on a mounted metadata_csum fs

[Darrick J. Wong]

It's not safe to change the UUID on a mounted filesystem when the
metadata_csum feature is enabled because there's (currently) no
way to update the kernel's CRC seeds, which precompute the UUID
component of the checksum for all metadata blocks.  Not to mention
that we'd have to rewrite /all/ metadata blocks, and any kernel disk
accesses will race with tune2fs, thus destroying the filesystem.

Note that it's fine to do the online UUID update if *only* group
descriptor checksums are enabled.

This is a regression introduced by 2334bd3a ("tune2fs: allow changing
the UUID mounted file systems with the -f option") which does not
seem to have been reviewed on the mailing list.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 misc/tune2fs.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 2f27d6f..ad833de 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -2960,7 +2960,9 @@ retry_open:
 			 * Changing the UUID requires rewriting all metadata,
 			 * which can race with a mounted fs.  Don't allow that.
 			 */
-			if ((mount_flags & EXT2_MF_MOUNTED) && !f_flag) {
+			if ((mount_flags & EXT2_MF_MOUNTED) && (!f_flag ||
+			     EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
+				       EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))) {
 				fputs(_("The UUID may only be "
 					"changed when the filesystem is "
 					"unmounted.\n"), stderr);

Re: [PATCH] tune2fs: don't change UUID on a mounted metadata_csum fs

[Theodore Ts'o]

On Tue, Sep 29, 2015 at 08:31:22AM -0700, Darrick J. Wong wrote:
> It's not safe to change the UUID on a mounted filesystem when the
> metadata_csum feature is enabled because there's (currently) no
> way to update the kernel's CRC seeds, which precompute the UUID
> component of the checksum for all metadata blocks.  Not to mention
> that we'd have to rewrite /all/ metadata blocks, and any kernel disk
> accesses will race with tune2fs, thus destroying the filesystem.

It's not safe, but if you know what you're doing, it can work if the
root file system is quiscent (or preferably, mounted read-only) and if
you reboot immediately afterwards.

In point of fact, the use case that inspired me to add this was
wanting to change the UUID right before creating a GCE image such that
it has a different UUID from the UUID from the base OS image that you
used to create the image.  And with metadata_csum, you can't even use
'debugfs -R "ssv uuid random"' to reset the uuid, since it won't
update the all of the metadata blocks.  The only way to update the
uuid while the root file system is mounted is to use "tune2fs -f -U
random".

If the concern is that it's too dangerous to do this even with the -f
option, maybe we could require two -f option --- i.e., "tune2fs -f -f
-U random /dev/sda1".  Or maybe we could require that the root file
system be mounted read-only, and adding a warning that this the
sytstem should be rebooted immediately afterwards.

In retrospect, we should have used a separate superblock field
containing the UUID for the metadata checksum seed.  This would allow
us to change the UUID used to find the root file system without
needing to jump through hoops.  Unfortunately, as in many things,
hindsight is 20-20.

						- Ted

Re: [PATCH] tune2fs: don't change UUID on a mounted metadata_csum fs

[Andreas Dilger]

On Sep 29, 2015, at 16:46, Theodore Ts'o <tytso@mit.edu> wrote:
> 
>> On Tue, Sep 29, 2015 at 08:31:22AM -0700, Darrick J. Wong wrote:
>> It's not safe to change the UUID on a mounted filesystem when the
>> metadata_csum feature is enabled because there's (currently) no
>> way to update the kernel's CRC seeds, which precompute the UUID
>> component of the checksum for all metadata blocks.  Not to mention
>> that we'd have to rewrite /all/ metadata blocks, and any kernel disk
>> accesses will race with tune2fs, thus destroying the filesystem.
> 
> It's not safe, but if you know what you're doing, it can work if the
> root file system is quiscent (or preferably, mounted read-only) and if
> you reboot immediately afterwards.
> 
> In point of fact, the use case that inspired me to add this was
> wanting to change the UUID right before creating a GCE image such that
> it has a different UUID from the UUID from the base OS image that you
> used to create the image.

I don't see how changing the UUID for mounted file systems can work
for metadata_csum file systems?

Wouldn't this cause a checksum error for every piece of metadata?

For regular file systems it wouldn't be fatal so long as the group descriptors
are written out to disk again with the new checksum. 

> And with metadata_csum, you can't even use
> 'debugfs -R "ssv uuid random"' to reset the uuid, since it won't
> update the all of the metadata blocks.  The only way to update the
> uuid while the root file system is mounted is to use "tune2fs -f -U
> random".
> 
> If the concern is that it's too dangerous to do this even with the -f
> option, maybe we could require two -f option --- i.e., "tune2fs -f -f
> -U random /dev/sda1".

I'd rather something like --force-uuid or similar so that it is clear what is
being forced, since "-f" is too overloaded to mean different things. 

> Or maybe we could require that the root file
> system be mounted read-only, and adding a warning that this the
> sytstem should be rebooted immediately afterwards.

I still don't think this can help for metadata_csum?

Cheers, Andreas

Re: [PATCH] tune2fs: don't change UUID on a mounted metadata_csum fs

[Theodore Ts'o]

On Tue, Sep 29, 2015 at 07:21:08PM -0600, Andreas Dilger wrote:
> 
> I don't see how changing the UUID for mounted file systems can work
> for metadata_csum file systems?
> 
> Wouldn't this cause a checksum error for every piece of metadata?

Tune2fs tries to update every piece of metadata when you run a command
like "tune2fs -U random /dev/sda1".  The danger is in the fact that
tune2fs is modifying every bit of metadata while the file system is
mounted.

If you do this with "debugfs -w -R "ssv uuid random" /dev/sda1", it
will break every metadata in the system, because it doesn't try to
update every bit of metadata.

I suppose a better choice might be to add a debugfs command which does
update the UUID, and use that command as an emergency override when
there's no other clean way of handling things.  It's more understood
by users that using debugfs is inherently dangerous, where as it's not
quite as obvious that -f means "here be dragons".  (Hence the
rationale of using "-f -f" which we've used in other cases.)

The problem with --force-uuid is that it means dragging in
getopt_long(), and having a fallback when compiling e2fsprogs on a
system that doesn't support getopt_long(), or if you are using a libc,
such as Bionic, dietlibc, etc., that doesn't have getopt_long().

							- Ted