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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 24 Nov 2015 17:18:58 -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>
Subject: Re: [PATCH] tune2fs: warn if the filesystem journal is dirty

On Nov 24, 2015, at 4:42 PM, Andreas Dilger <adilger@...ger.ca> wrote:
> 
> 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.

Actually, it looks like tune2fs doesn't have any other checks before "dangerous"
changes whether the journal needs to be replayed, which could potentially lead
to significant corruption if e.g. the inode size is changed and then the journal
is replayed afterward.

Some functions call check_fsck_needed(), but that doesn't check if the journal
needs to be replayed.  Definitely something for 1.43 before these features in
tune2fs become generally available.

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
> 
> 
> 
> 
> 


Cheers, Andreas






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