Re: [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels.

[Andreas Dilger <adilger@dilger.ca>]

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

On Nov 13, 2013, at 12:00 AM, David Turner <novalis@novalis.org> wrote:
> This patch is against e2fsprogs.
> 
> ---
> Older kernels on 64-bit machines would incorrectly encode pre-1970
> ext4 dates as post-2311 dates.  Detect and correct this (assuming the
> current date is before 2311).
> 
> Signed-off-by: David Turner <novalis@novalis.org>
> ---
> e2fsck/pass1.c   | 37 +++++++++++++++++++++++++++++++++++++
> e2fsck/problem.c |  7 +++++++
> e2fsck/problem.h |  6 ++++++
> 3 files changed, 50 insertions(+)
> 
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index ab23e42..cb72964 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -348,6 +348,23 @@ fix:
> 				EXT2_INODE_SIZE(sb), "pass1");
> }
> 
> +#define EXT4_EPOCH_BITS 2
> +#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1)
> +
> +static int large_inode_extra(__u32 xtime, __u32 extra) {

“large_inode_extra()” doesn’t really describe the purpose of this function very
well, which is checking the timestamps to have large future (or past) dates.
How about a more descriptive name “check_old_ext4_negative_epoch()” or
maybe “check_inode_extra_negative_epoch()” or something like that?

> +	return (xtime & (1 << 31)) != 0 &&
> +		(extra & EXT4_EPOCH_MASK) == EXT4_EPOCH_MASK;
> +}
> +
> +#define LARGE_INODE_EXTRA(inode, xtime) \

Ditto.

> +	large_inode_extra(inode->i_##xtime, \
> +			  inode->i_##xtime##_extra)
> +
> +/* When the date is earlier than 2311, we assume that atimes, ctimes,
> + * and mtimes greater than 2311 are actually pre-1970 dates mis-encoded.

I like the idea of checking the current date, so that there isn’t a need to
revert this code at some point in the future.  I’m wondering if there should
be a margin, like “When the current date is earlier than 2240 we assume ...
times greater than 2311 are actually ...”?   I’d hope that old versions of
pre-3.14 kernels are not still running by then.

> +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 6 * (1UL << 32)

That would make this (5 * (1ULL << 32)).  I think this should be ULL so that
if there are 64-bit timestamps on 32-bit systems it will still work correctly.

> static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
> {
> 	struct ext2_super_block *sb = ctx->fs->super;
> @@ -388,6 +405,26 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
> 		/* it seems inode has an extended attribute(s) in body */
> 		check_ea_in_inode(ctx, pctx);
> 	}
> +
> +	/*
> +	 * If the inode's extended atime (ctime, mtime) is stored in
> +	 * the old, invalid format, the inode is corrupt.
> +	 */
> +	if (sizeof(time_t) > 4 && ctx->now < EXT4_EXTRA_NEGATIVE_DATE_CUTOFF &&
> +		LARGE_INODE_EXTRA(inode, atime) ||
> +		LARGE_INODE_EXTRA(inode, ctime) ||
> +		LARGE_INODE_EXTRA(inode, mtime)) {

(style) please align continued line after ‘(‘ of previous line, otherwise it isn’t
easy to see if these are continuations of the condition or if they are part of the
body of the condition like the lines below.

> +		if (!fix_problem(ctx, PR_1_EA_TIME_OUT_OF_RANGE, pctx))
> +			return;
> +
> +		inode->i_atime_extra &= ~EXT4_EPOCH_MASK;
> +		inode->i_ctime_extra &= ~EXT4_EPOCH_MASK;
> +		inode->i_mtime_extra &= ~EXT4_EPOCH_MASK;
> +		e2fsck_write_inode_full(ctx, pctx->ino, pctx->inode,
> +					EXT2_INODE_SIZE(sb), "pass1");
> +	}
> +
> }
> 
> /*
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index 897693a..51fa7c3 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -1018,6 +1018,13 @@ static struct e2fsck_problem problem_table[] = {
> 	  N_("@i %i, end of extent exceeds allowed value\n\t(logical @b %c, physical @b %b, len %N)\n"),
> 	  PROMPT_CLEAR, 0 },
> 
> +/* The extended a, c, or mtime on this inode is in the far future,
> +   indicating that it was written with an older, buggy version of the
> +   kernel on a 64-bit machine */

Please make the comment match the expanded text as closely as possible.
Otherwise, it is hard to track down some problem that prints one message,
but uses the crazy @foo encodings and it isn’t clear what part of e2fsck
generated it.  It is fine if it contains more text.

> +	{ PR_1_EA_TIME_OUT_OF_RANGE,
> +	  N_("Extended time on @i %i is in the far future.\n"
> +		"Assume that it is in fact a pre-1970 date written by an older, buggy version of Linux?\n"), 

(style) please align after ‘(‘ from previous line and under 80 columns.

That said, I think this message is a bit harsh, since those older, buggy versions
of Linux include all versions running today.  I’d probably make a more succinct
message like:

           N_(“Timestamp(s) on @i %i beyond 2033 are likely pre-1970 dates.\n”)

> +		PROMPT_FIX, 0 },

I’d probably also make this error code “PROMPT_FIX | PREEN_OK | PR_NO_OK”.

> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index ae1ed26..a44f6dd 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -593,6 +593,12 @@ struct problem_context {
> #define PR_1_EXTENT_INDEX_START_INVALID	0x01006D
> 
> #define PR_1_EXTENT_END_OUT_OF_BOUNDS	0x01006E
> +
> +/* The extended a, c, or mtime on this inode is in the far future,
> +   indicating that it was written with an older, buggy version of
> +	 the kernel on a 64-bit machine */
> +#define PR_1_EA_TIME_OUT_OF_RANGE	0x01006F

Same goes for the comment here - it should contain the exact text of the printed
error message.

Cheers, Andreas






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