linux-ext4 - Re: [PATCH] tune2fs: warn if the filesystem journal is dirty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <7BF1ADDF-050A-425C-93B8-88F19267A6F6@dilger.ca>
Date:	Tue, 24 Nov 2015 16:42:27 -0700
From:	Andreas Dilger <adilger@...ger.ca>
To:	"Darrick J. Wong" <darrick.wong@...cle.com>
Cc:	Theodore Ts'o <tytso@....edu>,
	linux-ext4 <linux-ext4@...r.kernel.org>,
	Jim Garlick <garlick@...l.gov>
Subject: Re: [PATCH] tune2fs: warn if the filesystem journal is dirty

On Nov 24, 2015, at 4:26 PM, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> 
> On Tue, Nov 24, 2015 at 03:57:14PM -0700, Andreas Dilger wrote:
>> From: Jim Garlick <garlick@...l.gov>
>> 
>> Running tune2fs on a filesystem with an unrecovered journal can
>> cause the tune2fs settings changes in the superblock to be reverted
>> when the journal is replayed if it contains an uncommitted copy of
>> the superblock.  Print a warning if this is detected so that the
>> user isn't surprised if it happens.
>> 
>> Signed-off-by: Jim Garlick <garlick@...l.gov>
>> Signed-off-by: Andreas Dilger <andreas.dilger@...el.com>
>> ---
>> misc/tune2fs.c |   12 ++++++++++++
>> 1 files changed, 12 insertions(+), 0 deletions(-)
>> 
>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
>> index cd1d17f..fcb963a 100644
>> --- a/misc/tune2fs.c
>> +++ b/misc/tune2fs.c
>> @@ -2397,6 +2397,7 @@ retry_open:
>> 		ext2fs_mark_super_dirty(fs);
>> 		printf(_("Setting stripe width to %d\n"), stripe_width);
>> 	}
>> +
>> 	if (ext_mount_opts) {
>> 		strncpy((char *)(fs->super->s_mount_opts), ext_mount_opts,
>> 			sizeof(fs->super->s_mount_opts));
>> @@ -2406,6 +2407,17 @@ retry_open:
>> 		       ext_mount_opts);
>> 		free(ext_mount_opts);
>> 	}
>> +
>> +	/* Warn if file system needs recovery and it is opened for writing. */
>> +	if ((open_flag & EXT2_FLAG_RW) && !(mount_flags & EXT2_MF_MOUNTED) &&
>> +	    (sb->s_feature_compat & EXT3_FEATURE_COMPAT_HAS_JOURNAL) &&
>> +	    (sb->s_feature_incompat & EXT3_FEATURE_INCOMPAT_RECOVER)) {
> 
> ext2fs_has_feature_journal(sb) &&
> ext2fs_has_feature_journal_needs_recovery(sb)

Those patches don't exist on maint or master...  Maybe Ted will consider to
cherry-pick 86f3b6cf98 and dependent patches to those branches?

>> +		fprintf(stderr,
>> +			_("Warning: needs_recovery flag is set. You may wish\n"
>> +			  "replay the journal then rerun this command, or any\n"
> 
> "You may wish to replay the journal..."
> 
>> +			  "changes may be overwritten by journal recovery.\n"));
> 
> I wonder if we ought simply to replay the journal in this situation, since
> debugfs/fuse2fs can do it.
> 
> ...or at least tell the user how to replay? ("e2fsck -E journal_only"/mount)

I'm not sure if there is some reason they may _not_ want the journal to be
replayed in this case?  I agree at least with telling them how to do it.
I'll send a new patch for this update.


We had a bad time with "tune2fs" doing too much stuff to the filesystem
unexpectedly a few weekends ago (related to the "quota update" bug).  The
more automagic added to tune2fs (e.g. huge reorganization of the filesystem
when setting a feature), the more likely it is that there will be a bad
outcome for some user that isn't expecting tune2fs to make major changes.

At a minimum, I think anything in tune2fs that is changing more than a single
flag or field in the superblock (e.g. change inode size, recompute checksums,
etc. that used to require a separate e2fsck run) should pause/prompt like:

   This may take several minutes/hours and cannot be interrupted. Are you sure?

for interactive users ala proceed_question().

That should be done before the 1.43 release I think, since the number of such
actions taken by tune2fs has increased significantly in that release.

Cheers, Andreas

> --D
> 
>> +	}
>> +
>> 	free(device_name);
>> 	remove_error_table(&et_ext2_error_table);
>> 
>> --
>> 1.7.3.4
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)