linux-ext4 - Re: Journal under-reservation bug on first >2G file

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141001195954.GD2903@thunk.org>
Date:	Wed, 1 Oct 2014 15:59:54 -0400
From:	Theodore Ts'o <tytso@....edu>
To:	Eric Sandeen <sandeen@...hat.com>
Cc:	Andreas Dilger <adilger@...ger.ca>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: Journal under-reservation bug on first >2G file

On Wed, Oct 01, 2014 at 09:43:32AM -0500, Eric Sandeen wrote:
> > That sounds like a plan.  If we only enable it automatically at mount
> > time (iff we mounted the file system read/write) if any of the ext3 or
> > ext4 specific features are enabled, that should be completely safe.
> 
> Ok, so do that, and don't bump the reservations? I suppose
> the size test & superblock write can be removed, then...
> 
> This does bug me a little; at one point we were very carefully not
> enabling any new features by mounting with a new kernel; that was
> specific to mounting-ext2-with-ext4 etc, but it still feels slightly
> inconsistent.  Although I guess we enable it today by mounting-and-
> writing-a-big-enough-file.

Yeah, this behaviour was one that dates back a *long* time, before we
established the rule that we don't enable any new features
automatically.  If this was a new feature, I wouldn't be advocating
this.  But if we change this now, we could introduce a regression, or
at least a surprising breakage.

> Something like this should fix it too, though, with less unexpected
> behind-your-back behavior:
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 3aa26e9..2f94cd6 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2563,9 +2563,15 @@ retry_grab:
>          * if there is delayed block allocation. But we still need
>          * to journalling the i_disksize update if writes to the end
>          * of file which has an already mapped buffer.
> +        * If this write might need to update the superblock due to the
> +        * filesize adding a new superblock feature flag, add that too.
>          */
>  retry_journal:
> -       handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE, 1);
> +       handle = ext4_journal_start(inode, EXT4_HT_WRITE_PAGE,
> +                                   EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> +                                       EXT4_FEATURE_RO_COMPAT_LARGE_FILE) ?
> +                                   1 : 2);
> +

Yes, I suppose that would work as well.  It means that file systems
which don't have LARGE_FILE will waste a bit more space in the
journal, causing the journal to potentially close prematurely.

The code would be a bit simpler if we removed "set only if i_size has
gotten too big", and replaced it with a "set it unconditionally at
mount time".  So there are tradeoffs with either approach.  At this
point I'm slightly in favor of enabling it by default if ext4 features
are enabled, either in the kernel or in the e2fsck.  And if we're
going to do that, doing it in the kernel is more foolproof, and it
will have the same net result.

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