Re: [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels.
On Nov 13, 2013, at 12:00 AM, David Turner 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
> ---
> 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
Re: [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels.
On Nov 13, 2013, at 12:00 AM, David Turner nova...@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 nova...@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
