Patchwork [v7,1/2] e2fsck: Correct ext4 dates generated by old kernels.

login
register
mail settings
Submitter David Turner
Date Dec. 7, 2013, 8:02 p.m.
Message ID <1386446560.10748.5.camel@chiang>
Download mbox | patch
Permalink /patch/298716/
State New
Headers show

Comments

David Turner - Dec. 7, 2013, 8:02 p.m.
I went ahead and wrote some tests, and they seem to confirm that
my patch to e2fsck works as expected (once I added crtime).

However, as Andreas notes, "we want to verify .. that "debugfs -R
'stat testfile'" decodes the times correctly."  Unfortunately, it
does not, and it is not trivial to fix.  debugfs uses an internal
function time_to_string(__u32), which calls gmtime or localtime.
These functions are defined on time_t, which is 32-bits on 32-bit
Linux systems.  In addition, because time_to_string takes
an unsigned int, it returns bad results for pre-1970 dates.

One fix for this would be to include our own gmtime/localtime
defined on 64-bit integers.  But this is likely to be
complicated, given the dependence of localtime on timezone data.

Instead, I decided to simply adjust time_to_string to take a
time_t.  This means that on 32-bit time_t systems, we will give
wrong results for post-2038 dates, but correct results for
pre-1970 dates. (see patch 2/2)

For the tests, I am looking at the raw value of ctime and
ctime_extra, which will work regardless of the size of time_t

Is using time_t acceptable, or do you want me to copy over
gmtime/localtime from glibc?  Or is there a third approach
that would be better?

Also, while I was making this change, I happened to notice that
there is no dtime_extra field into which to put the extra bits.
This means that there is still a year 2038 problem with tools
that deal with deleted files (including the corner case that the
system is shut down cleanly at the exact second that the bottom
32 bits of the current time are zero, leading fsck to believe
that there was an unclean shutdown).

I want to make an additional change to correct this.  Since I
assume it is impossible to add an additional field, I propose to
use ctime_extra to store the extra bits of dtime on deletion.
This is a bit hackish, since it does destroy a bit of data, but
it is significantly better than dtime being totally broken after
2038.  What do people think?
--
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 2242).

Includes tests for this, as well as changes to debugfs to correctly
set crtimes.

Signed-off-by: David Turner <novalis@novalis.org>
---
 debugfs/set_fields.c                  |  2 +-
 e2fsck/pass1.c                        | 43 ++++++++++++++++
 e2fsck/problem.c                      |  4 ++
 e2fsck/problem.h                      |  4 ++
 lib/extra_epoch.h                     |  2 +
 tests/f_pre_1970_date_encoding/expect | 45 ++++++++++++++++
 tests/f_pre_1970_date_encoding/name   |  1 +
 tests/f_pre_1970_date_encoding/script | 96 +++++++++++++++++++++++++++++++++++
 8 files changed, 196 insertions(+), 1 deletion(-)
 create mode 100644 lib/extra_epoch.h
 create mode 100644 tests/f_pre_1970_date_encoding/expect
 create mode 100644 tests/f_pre_1970_date_encoding/name
 create mode 100644 tests/f_pre_1970_date_encoding/script
Andreas Dilger - Dec. 7, 2013, 10:33 p.m.
I suspect that any 32-bit systems running at that time will have been updated to have 64-bit time_t or otherwise have windowed the 32-bit time_t to have a new starting epoch.

So I'm willing to punt on decoding the 64-bit value correctly to libc and just assign our time to the system time_t. 

Cheers, Andreas

> On Dec 7, 2013, at 13:02, David Turner <novalis@novalis.org> wrote:
> 
> I went ahead and wrote some tests, and they seem to confirm that
> my patch to e2fsck works as expected (once I added crtime).
> 
> However, as Andreas notes, "we want to verify .. that "debugfs -R
> 'stat testfile'" decodes the times correctly."  Unfortunately, it
> does not, and it is not trivial to fix.  debugfs uses an internal
> function time_to_string(__u32), which calls gmtime or localtime.
> These functions are defined on time_t, which is 32-bits on 32-bit
> Linux systems.  In addition, because time_to_string takes
> an unsigned int, it returns bad results for pre-1970 dates.
> 
> One fix for this would be to include our own gmtime/localtime
> defined on 64-bit integers.  But this is likely to be
> complicated, given the dependence of localtime on timezone data.
> 
> Instead, I decided to simply adjust time_to_string to take a
> time_t.  This means that on 32-bit time_t systems, we will give
> wrong results for post-2038 dates, but correct results for
> pre-1970 dates. (see patch 2/2)
> 
> For the tests, I am looking at the raw value of ctime and
> ctime_extra, which will work regardless of the size of time_t
> 
> Is using time_t acceptable, or do you want me to copy over
> gmtime/localtime from glibc?  Or is there a third approach
> that would be better?
> 
> Also, while I was making this change, I happened to notice that
> there is no dtime_extra field into which to put the extra bits.
> This means that there is still a year 2038 problem with tools
> that deal with deleted files (including the corner case that the
> system is shut down cleanly at the exact second that the bottom
> 32 bits of the current time are zero, leading fsck to believe
> that there was an unclean shutdown).
> 
> I want to make an additional change to correct this.  Since I
> assume it is impossible to add an additional field, I propose to
> use ctime_extra to store the extra bits of dtime on deletion.
> This is a bit hackish, since it does destroy a bit of data, but
> it is significantly better than dtime being totally broken after
> 2038.  What do people think?
> --
> 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 2242).
> 
> Includes tests for this, as well as changes to debugfs to correctly
> set crtimes.
> 
> Signed-off-by: David Turner <novalis@novalis.org>
> ---
> debugfs/set_fields.c                  |  2 +-
> e2fsck/pass1.c                        | 43 ++++++++++++++++
> e2fsck/problem.c                      |  4 ++
> e2fsck/problem.h                      |  4 ++
> lib/extra_epoch.h                     |  2 +
> tests/f_pre_1970_date_encoding/expect | 45 ++++++++++++++++
> tests/f_pre_1970_date_encoding/name   |  1 +
> tests/f_pre_1970_date_encoding/script | 96 +++++++++++++++++++++++++++++++++++
> 8 files changed, 196 insertions(+), 1 deletion(-)
> create mode 100644 lib/extra_epoch.h
> create mode 100644 tests/f_pre_1970_date_encoding/expect
> create mode 100644 tests/f_pre_1970_date_encoding/name
> create mode 100644 tests/f_pre_1970_date_encoding/script
> 
> diff --git a/debugfs/set_fields.c b/debugfs/set_fields.c
> index aad1cd8..f7c55a7 100644
> --- a/debugfs/set_fields.c
> +++ b/debugfs/set_fields.c
> @@ -200,7 +200,7 @@ static struct field_set_info inode_fields[] = {
>        4, parse_uint },
>    { "atime_extra", &set_inode.i_atime_extra, NULL,
>        4, parse_uint },
> -    { "crtime", &set_inode.i_crtime, NULL, 4, parse_uint },
> +    { "crtime", &set_inode.i_crtime, NULL, 4, parse_time },
>    { "crtime_extra", &set_inode.i_crtime_extra, NULL,
>        4, parse_uint },
>    { "bmap", NULL, NULL, 4, parse_bmap, FLAG_ARRAY },
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index ab23e42..ecbd79e 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -50,6 +50,8 @@
> 
> #include "problem.h"
> 
> +#include "extra_epoch.h"
> +
> #ifdef NO_INLINE_FUNCS
> #define _INLINE_
> #else
> @@ -348,6 +350,21 @@ fix:
>                EXT2_INODE_SIZE(sb), "pass1");
> }
> 
> +static int check_inode_extra_negative_epoch(__u32 xtime, __u32 extra) {
> +    return (xtime & (1 << 31)) != 0 &&
> +        (extra & EXT4_EPOCH_MASK) == EXT4_EPOCH_MASK;
> +}
> +
> +#define CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, xtime) \
> +    check_inode_extra_negative_epoch(inode->i_##xtime, \
> +                     inode->i_##xtime##_extra)
> +
> +/* When today's date is earlier than 2242, we assume that atimes,
> + * ctimes, crtimes, and mtimes with years in the range 2310..2378 are
> + * actually pre-1970 dates mis-encoded.
> + */
> +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 2 * (1LL << 32)
> +
> static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
> {
>    struct ext2_super_block *sb = ctx->fs->super;
> @@ -388,6 +405,32 @@ 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, crtime, mtime) is stored in
> +     * the old, invalid format, repair it.
> +     */
> +    if (sizeof(time_t) > 4 && ctx->now < EXT4_EXTRA_NEGATIVE_DATE_CUTOFF &&
> +        (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, atime) ||
> +         CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, ctime) ||
> +         CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, crtime) ||
> +         CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, mtime))) {
> +
> +        if (!fix_problem(ctx, PR_1_EA_TIME_OUT_OF_RANGE, pctx))
> +            return;
> +
> +        if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, atime))
> +            inode->i_atime_extra &= ~EXT4_EPOCH_MASK;
> +        if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, ctime))
> +            inode->i_ctime_extra &= ~EXT4_EPOCH_MASK;
> +        if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, crtime))
> +            inode->i_crtime_extra &= ~EXT4_EPOCH_MASK;
> +        if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, mtime))
> +            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..b212d00 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -1018,6 +1018,10 @@ 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 },
> 
> +  /* Timestamp(s) on inode beyond 2310-04-04 are likely pre-1970 dates. */
> +    { PR_1_EA_TIME_OUT_OF_RANGE,
> +        N_("Timestamp(s) on @i %i beyond 2310-04-04 are likely pre-1970 dates.\n"),
> +        PROMPT_FIX | PR_PREEN_OK | PR_NO_OK, 0 },
> 
>    /* Pass 1b errors */
> 
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index ae1ed26..3710638 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -593,6 +593,10 @@ struct problem_context {
> #define PR_1_EXTENT_INDEX_START_INVALID    0x01006D
> 
> #define PR_1_EXTENT_END_OUT_OF_BOUNDS    0x01006E
> +
> +/* Timestamp(s) on inode beyond 2310-04-04 are likely pre-1970 dates. */
> +#define PR_1_EA_TIME_OUT_OF_RANGE    0x01006F
> +
> /*
>  * Pass 1b errors
>  */
> diff --git a/lib/extra_epoch.h b/lib/extra_epoch.h
> new file mode 100644
> index 0000000..465c43f
> --- /dev/null
> +++ b/lib/extra_epoch.h
> @@ -0,0 +1,2 @@
> +#define EXT4_EPOCH_BITS 2
> +#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1)
> diff --git a/tests/f_pre_1970_date_encoding/expect b/tests/f_pre_1970_date_encoding/expect
> new file mode 100644
> index 0000000..1a71571
> --- /dev/null
> +++ b/tests/f_pre_1970_date_encoding/expect
> @@ -0,0 +1,45 @@
> +times for year-1909 =
> + ctime: 0x8e475440:00000003
> + atime: 0x8e475440:00000003
> + mtime: 0x8e475440:00000003
> +crtime: 0x8e475440:00000003
> +times for year-1979 =
> + ctime: 0x11db6940:00000000
> + atime: 0x11db6940:00000000
> + mtime: 0x11db6940:00000000
> +crtime: 0x11db6940:00000000
> +times for year-2039 =
> + ctime: 0x82a37b40:00000001
> + atime: 0x82a37b40:00000001
> + mtime: 0x82a37b40:00000001
> +crtime: 0x82a37b40:00000001
> +times for year-2139 =
> + ctime: 0x3e9b9940:00000001
> + atime: 0x3e9b9940:00000001
> + mtime: 0x3e9b9940:00000001
> +crtime: 0x3e9b9940:00000001
> +times for year-1909 =
> + ctime: 0x8e475440:00000000
> + atime: 0x8e475440:00000000
> + mtime: 0x8e475440:00000000
> +crtime: 0x8e475440:00000000
> +times for year-1979 =
> + ctime: 0x11db6940:00000000
> + atime: 0x11db6940:00000000
> + mtime: 0x11db6940:00000000
> +crtime: 0x11db6940:00000000
> +times for year-2039 =
> + ctime: 0x82a37b40:00000001
> + atime: 0x82a37b40:00000001
> + mtime: 0x82a37b40:00000001
> +crtime: 0x82a37b40:00000001
> +times for year-2139 =
> + ctime: 0x3e9b9940:00000001
> + atime: 0x3e9b9940:00000001
> + mtime: 0x3e9b9940:00000001
> +crtime: 0x3e9b9940:00000001
> +times for year-1909 =
> + ctime: 0x8e475440:00000003
> + atime: 0x8e475440:00000003
> + mtime: 0x8e475440:00000003
> +crtime: 0x8e475440:00000003
> diff --git a/tests/f_pre_1970_date_encoding/name b/tests/f_pre_1970_date_encoding/name
> new file mode 100644
> index 0000000..9805324
> --- /dev/null
> +++ b/tests/f_pre_1970_date_encoding/name
> @@ -0,0 +1 @@
> +correct mis-encoded pre-1970 dates
> diff --git a/tests/f_pre_1970_date_encoding/script b/tests/f_pre_1970_date_encoding/script
> new file mode 100644
> index 0000000..c3e12f5
> --- /dev/null
> +++ b/tests/f_pre_1970_date_encoding/script
> @@ -0,0 +1,96 @@
> +if test -x $DEBUGFS_EXE; then
> +
> +OUT=$test_name.log
> +TIMESTAMPS=$test_name.timestamps.log
> +EXP=$test_dir/expect
> +FSCK_OPT=-yf
> +
> +create_file_with_xtime_and_extra() {
> +    name=$1
> +    time=$2
> +    extra=$3
> +    $DEBUGFS -w -R "write /dev/null $name" $TMPFILE > $OUT 2>&1
> +    for xtime in atime ctime mtime crtime
> +    do
> +        $DEBUGFS -w -R "set_inode_field $name $xtime @$time" $TMPFILE > $OUT 2>&1
> +
> +        $DEBUGFS -w -R "set_inode_field $name ${xtime}_extra $extra" $TMPFILE > $OUT 2>&1
> +    done
> +}
> +
> +get_file_xtime_and_extra() {
> +    name=$1
> +    echo "times for $name =" >> $TIMESTAMPS
> +    $DEBUGFS -R "stat $name" $TMPFILE 2>&1 | egrep '^( a| c| m|cr)time: ' |sed 's/ --.*//' >> $TIMESTAMPS
> +}
> +
> +rm -f $OUT
> +rm -f $TIMESTAMPS
> +
> +#create an empty ext4 filesystem with 256-byte inodes for testing
> +dd status=none if=/dev/zero of=$TMPFILE bs=1024 count=5000
> +echo  mkfs.ext4 -q -I 256 $TMPFILE >> $OUT
> +yes | mkfs.ext4 -q -I 256 $TMPFILE >> $OUT 2>&1
> +
> +# this is a pre-1970 file encoded with the old encoding.
> +# fsck should repair this
> +create_file_with_xtime_and_extra year-1909 -1907928000 3
> +
> +# these are all already encoded correctly
> +create_file_with_xtime_and_extra year-1979   299592000 0
> +create_file_with_xtime_and_extra year-2039  2191752000 1
> +create_file_with_xtime_and_extra year-2139  5345352000 1
> +
> +# confirm that the xtime is wrong on the pre-1970 file
> +get_file_xtime_and_extra year-1909
> +
> +# and confirm that it is right on the remaining files
> +get_file_xtime_and_extra year-1979
> +get_file_xtime_and_extra year-2039
> +get_file_xtime_and_extra year-2139
> +
> +# before we repair the filesystem, save off a copy so that
> +# we can use it later
> +
> +cp $TMPFILE $TMPFILE.backup
> +
> +# repair the filesystem
> +E2FSCK_TIME=1386393539 $FSCK $FSCK_OPT $TMPFILE >> $OUT 2>&1
> +
> +# check that the dates and xtime_extra on the file is now correct
> +get_file_xtime_and_extra year-1909
> +
> +# check that the remaining dates have not been altered
> +get_file_xtime_and_extra year-1979
> +get_file_xtime_and_extra year-2039
> +get_file_xtime_and_extra year-2139
> +
> +# now we need to check that after the year 2242, e2fsck does not
> +# modify dates with extra_xtime=3
> +
> +# restore the unrepaired filesystem
> +mv $TMPFILE.backup $TMPFILE
> +
> +#retry the repair
> +E2FSCK_TIME=9270393539 $FSCK $FSCK_OPT $TMPFILE >> $OUT 2>&1
> +
> +# check that the 1909 file is unaltered (i.e. it has a post-2378 date)
> +get_file_xtime_and_extra year-1909
> +
> +cmp -s $TIMESTAMPS $EXP
> +status=$?
> +
> +if [ "$status" = 0 ] ; then
> +    echo "$test_name: $test_description: ok"
> +    touch $test_name.ok
> +else
> +    echo "$test_name: $test_description: failed"
> +    diff $DIFF_OPTS $EXP $TIMESTAMPS > $test_name.failed
> +fi
> +
> +unset OUT TIMESTAMPS EXP FSCK_OPT
> +
> +else #if test -x $DEBUGFS_EXE; then
> +    echo "$test_name: $test_description: skipped"
> +fi
> +
> -- 
> 1.8.1.2
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Dec. 8, 2013, 12:53 a.m.
On Sat, Dec 07, 2013 at 03:02:40PM -0500, David Turner wrote:
> 
> However, as Andreas notes, "we want to verify .. that "debugfs -R
> 'stat testfile'" decodes the times correctly."  Unfortunately, it
> does not, and it is not trivial to fix.  debugfs uses an internal
> function time_to_string(__u32), which calls gmtime or localtime.
> These functions are defined on time_t, which is 32-bits on 32-bit
> Linux systems.  In addition, because time_to_string takes
> an unsigned int, it returns bad results for pre-1970 dates.

We can and should change time_to_string to take an unsigned 64-bit
type; it's an internal interface to debugfs.  The question of what to
do if the system time_t is only 32-bit.

What I'd probably suggest is to have a mode (set either by an
environment variable, or a debugfs command) which causes
time_to_string to use an internal version of a 64-bit capable gmtime
function.  This will allow for better regression testing, and it in a
way that will be have stable results regardless of whether a
particular platform, or a future version of glibc has a 64-bit time_t
or not.

> Also, while I was making this change, I happened to notice that
> there is no dtime_extra field into which to put the extra bits.
> This means that there is still a year 2038 problem with tools
> that deal with deleted files (including the corner case that the
> system is shut down cleanly at the exact second that the bottom
> 32 bits of the current time are zero, leading fsck to believe
> that there was an unclean shutdown).

I'm not sure we care, since nothing is actually using the dtime.  This
was originally intended to make it easy to recover from accident file
deletions, using debugfs's lsdel command.  However, ever since ext3,
in order to make sure the file system is consistent if it crashes
during an unlink, we end up truncating the file before we unlink it,
so i_blocks[] is completely cleared for deleted inodes.  This makes it
essentially impossible for lsdel to work.

> I want to make an additional change to correct this.  Since I
> assume it is impossible to add an additional field, I propose to
> use ctime_extra to store the extra bits of dtime on deletion.
> This is a bit hackish, since it does destroy a bit of data, but
> it is significantly better than dtime being totally broken after
> 2038.  What do people think?

Given that we always set ctime when delete a file (which makes sense,
since we are decrementing i_links_count), I think changing debugfs
(which is the only thing that even looks at dtime, BTW) makes a lot of
sense.

Regards,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Turner - Dec. 8, 2013, 2:58 a.m.
On Sat, 2013-12-07 at 19:53 -0500, Theodore Ts'o wrote:
> On Sat, Dec 07, 2013 at 03:02:40PM -0500, David Turner wrote:
> > 
> > However, as Andreas notes, "we want to verify .. that "debugfs -R
> > 'stat testfile'" decodes the times correctly."  Unfortunately, it
> > does not, and it is not trivial to fix.  debugfs uses an internal
> > function time_to_string(__u32), which calls gmtime or localtime.
> > These functions are defined on time_t, which is 32-bits on 32-bit
> > Linux systems.  In addition, because time_to_string takes
> > an unsigned int, it returns bad results for pre-1970 dates.
> 
> We can and should change time_to_string to take an unsigned 64-bit
> type; it's an internal interface to debugfs.  

Shouldn't this be a signed 64-bit type, since we have to support times
before 1970?

> The question of what to do if the system time_t is only 32-bit.
> 
> What I'd probably suggest is to have a mode (set either by an
> environment variable, or a debugfs command) which causes
> time_to_string to use an internal version of a 64-bit capable gmtime
> function.  This will allow for better regression testing, and it in a
> way that will be have stable results regardless of whether a
> particular platform, or a future version of glibc has a 64-bit time_t
> or not.

Presently, the TZ environment variable selects between gmtime and
localtime.  How about the following:

We supply a 64-bit version of gmtime (but not localtime).  If time_t is
32 bits, and the date being printed is after 2038, and TZ is not GMT,
then we return an error message instead of calling localtime.  Then, in
any of the tests that depend on debugfs date printing we can simply set
TZ=GMT, so that they will continue to work regardless of the size of
time_t.

> > Also, while I was making this change, I happened to notice that
> > there is no dtime_extra field into which to put the extra bits.
> > This means that there is still a year 2038 problem with tools
> > that deal with deleted files (including the corner case that the
> > system is shut down cleanly at the exact second that the bottom
> > 32 bits of the current time are zero, leading fsck to believe
> > that there was an unclean shutdown).
> 
> I'm not sure we care, since nothing is actually using the dtime.  This
> was originally intended to make it easy to recover from accident file
> deletions, using debugfs's lsdel command.  However, ever since ext3,
> in order to make sure the file system is consistent if it crashes
> during an unlink, we end up truncating the file before we unlink it,
> so i_blocks[] is completely cleared for deleted inodes.  This makes it
> essentially impossible for lsdel to work.

I just did a quick grep, and it looks like
fs/ext4/ialloc.c:recently_deleted is using it.  That's not relevant to
e2fsprogs, but we should correct it anyway, because it will probably
break in 2038.

Also fs/ext4/inode.c:ext4_do_update_inode -- I'm not sure quite what's
going on there, but it seems like it should be fixed as well.

I'll volunteer to make these changes to the kernel and to e2fsprogs, but
I think my current set of patches can be safely merged before I do that.

> > I want to make an additional change to correct this.  Since I
> > assume it is impossible to add an additional field, I propose to
> > use ctime_extra to store the extra bits of dtime on deletion.
> > This is a bit hackish, since it does destroy a bit of data, but
> > it is significantly better than dtime being totally broken after
> > 2038.  What do people think?
> 
> Given that we always set ctime when delete a file (which makes sense,
> since we are decrementing i_links_count), I think changing debugfs
> (which is the only thing that even looks at dtime, BTW) makes a lot of
> sense.

I'm not really very familiar with the ext4 internals.  Are you saying
that it's safe to just read ctime_extra (without explicitly writing it
when we write dtime)?  Or just that it is safe to overwrite it when we
write dtime? 



--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - Dec. 8, 2013, 3:21 a.m.
On Sat, Dec 07, 2013 at 09:58:17PM -0500, David Turner wrote:
> > We can and should change time_to_string to take an unsigned 64-bit
> > type; it's an internal interface to debugfs.  
> 
> Shouldn't this be a signed 64-bit type, since we have to support times
> before 1970?

That depends on whether we make time_to_string() and string_to_time()
handle the ext4-specific encoding, or whether we make these routines
take an offset in terms of "seconds since the Epoch".  I was assuming
the former, but it does make sense to have ext2fs library functions
which handle the conversion --- but in that case, said library
functions will need to use an explicit 64-bit libext2fs type, since we
can't count on the width of the system-supplied time_t.

> > What I'd probably suggest is to have a mode (set either by an
> > environment variable, or a debugfs command) which causes
> > time_to_string to use an internal version of a 64-bit capable gmtime
> > function.  This will allow for better regression testing, and it in a
> > way that will be have stable results regardless of whether a
> > particular platform, or a future version of glibc has a 64-bit time_t
> > or not.
> 
> Presently, the TZ environment variable selects between gmtime and
> localtime.  How about the following:
> 
> We supply a 64-bit version of gmtime (but not localtime).  If time_t is
> 32 bits, and the date being printed is after 2038, and TZ is not GMT,
> then we return an error message instead of calling localtime.  Then, in
> any of the tests that depend on debugfs date printing we can simply set
> TZ=GMT, so that they will continue to work regardless of the size of
> time_t.

Well, the reason why I wanted a way to explicitly specify the built-in
gmtime was to isolate problems caused by differences in how future
gmtime()'s might be implemented.  Maybe I'm being too paranoid here,
and all of the confusion over leap seconds should be resolved by now.
(e.g., https://groups.google.com/forum/#!topic/comp.unix.programmer/zTXAaa5lnFg)

But we could do this via a special version of TZ (__libext2fs_GMT),
which should be safe since other programs which are using the stock
localtime() function will be fine, since unrecognized TZ values is
interpreted by libc as GMT.

> > Given that we always set ctime when delete a file (which makes sense,
> > since we are decrementing i_links_count), I think changing debugfs
> > (which is the only thing that even looks at dtime, BTW) makes a lot of
> > sense.
> 
> I'm not really very familiar with the ext4 internals.  Are you saying
> that it's safe to just read ctime_extra (without explicitly writing it
> when we write dtime)?  Or just that it is safe to overwrite it when we
> write dtime?

We actually set dtime in ext4_evict_inode(); this gets called when
i_nlink goes to zero, and there are no longer any open file
descriptors referencing the inode.  It is possible for ctime != dtime
if there is still a fd open on the inode at the time of the last
unlink(), and the fd could keep the inode from being actually released
for hours, and in theory, years or decades.

In practice though, the high bits of ctime and dtime should be the
same, unless the above period happens to span one of the encoding
gaps.  The simplest thing to do would be to simply set ctime and dtime
in ext4_evict_inode().  The slightly more complicated thing to do
would be to check to see if the high bits of dtime (if we had a
dtime_extra) are different from ctime_extra, and if so, in that case,
set ctime and dtime.

It doesn't really matter since by the time the inode is unlinked,
nothing else will see the contents of the inode except for a file
system developer who is poking at the file system using debugfs.  It
might be useful in some circumstances to see when the file was
unlinked versus when it was actually finally relased, but that's a
pretty minor edge case.

Regards,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/debugfs/set_fields.c b/debugfs/set_fields.c
index aad1cd8..f7c55a7 100644
--- a/debugfs/set_fields.c
+++ b/debugfs/set_fields.c
@@ -200,7 +200,7 @@  static struct field_set_info inode_fields[] = {
 		4, parse_uint },
 	{ "atime_extra", &set_inode.i_atime_extra, NULL,
 		4, parse_uint },
-	{ "crtime", &set_inode.i_crtime, NULL, 4, parse_uint },
+	{ "crtime", &set_inode.i_crtime, NULL, 4, parse_time },
 	{ "crtime_extra", &set_inode.i_crtime_extra, NULL,
 		4, parse_uint },
 	{ "bmap", NULL, NULL, 4, parse_bmap, FLAG_ARRAY },
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index ab23e42..ecbd79e 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -50,6 +50,8 @@ 
 
 #include "problem.h"
 
+#include "extra_epoch.h"
+
 #ifdef NO_INLINE_FUNCS
 #define _INLINE_
 #else
@@ -348,6 +350,21 @@  fix:
 				EXT2_INODE_SIZE(sb), "pass1");
 }
 
+static int check_inode_extra_negative_epoch(__u32 xtime, __u32 extra) {
+	return (xtime & (1 << 31)) != 0 &&
+		(extra & EXT4_EPOCH_MASK) == EXT4_EPOCH_MASK;
+}
+
+#define CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, xtime) \
+	check_inode_extra_negative_epoch(inode->i_##xtime, \
+					 inode->i_##xtime##_extra)
+
+/* When today's date is earlier than 2242, we assume that atimes,
+ * ctimes, crtimes, and mtimes with years in the range 2310..2378 are
+ * actually pre-1970 dates mis-encoded.
+ */
+#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 2 * (1LL << 32)
+
 static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx)
 {
 	struct ext2_super_block *sb = ctx->fs->super;
@@ -388,6 +405,32 @@  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, crtime, mtime) is stored in
+	 * the old, invalid format, repair it.
+	 */
+	if (sizeof(time_t) > 4 && ctx->now < EXT4_EXTRA_NEGATIVE_DATE_CUTOFF &&
+	    (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, atime) ||
+	     CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, ctime) ||
+	     CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, crtime) ||
+	     CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, mtime))) {
+
+		if (!fix_problem(ctx, PR_1_EA_TIME_OUT_OF_RANGE, pctx))
+			return;
+
+		if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, atime))
+			inode->i_atime_extra &= ~EXT4_EPOCH_MASK;
+		if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, ctime))
+			inode->i_ctime_extra &= ~EXT4_EPOCH_MASK;
+		if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, crtime))
+			inode->i_crtime_extra &= ~EXT4_EPOCH_MASK;
+		if (CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, mtime))
+			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..b212d00 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -1018,6 +1018,10 @@  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 },
 
+  /* Timestamp(s) on inode beyond 2310-04-04 are likely pre-1970 dates. */
+	{ PR_1_EA_TIME_OUT_OF_RANGE,
+		N_("Timestamp(s) on @i %i beyond 2310-04-04 are likely pre-1970 dates.\n"),
+		PROMPT_FIX | PR_PREEN_OK | PR_NO_OK, 0 },
 
 	/* Pass 1b errors */
 
diff --git a/e2fsck/problem.h b/e2fsck/problem.h
index ae1ed26..3710638 100644
--- a/e2fsck/problem.h
+++ b/e2fsck/problem.h
@@ -593,6 +593,10 @@  struct problem_context {
 #define PR_1_EXTENT_INDEX_START_INVALID	0x01006D
 
 #define PR_1_EXTENT_END_OUT_OF_BOUNDS	0x01006E
+
+/* Timestamp(s) on inode beyond 2310-04-04 are likely pre-1970 dates. */
+#define PR_1_EA_TIME_OUT_OF_RANGE	0x01006F
+
 /*
  * Pass 1b errors
  */
diff --git a/lib/extra_epoch.h b/lib/extra_epoch.h
new file mode 100644
index 0000000..465c43f
--- /dev/null
+++ b/lib/extra_epoch.h
@@ -0,0 +1,2 @@ 
+#define EXT4_EPOCH_BITS 2
+#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1)
diff --git a/tests/f_pre_1970_date_encoding/expect b/tests/f_pre_1970_date_encoding/expect
new file mode 100644
index 0000000..1a71571
--- /dev/null
+++ b/tests/f_pre_1970_date_encoding/expect
@@ -0,0 +1,45 @@ 
+times for year-1909 =
+ ctime: 0x8e475440:00000003
+ atime: 0x8e475440:00000003
+ mtime: 0x8e475440:00000003
+crtime: 0x8e475440:00000003
+times for year-1979 =
+ ctime: 0x11db6940:00000000
+ atime: 0x11db6940:00000000
+ mtime: 0x11db6940:00000000
+crtime: 0x11db6940:00000000
+times for year-2039 =
+ ctime: 0x82a37b40:00000001
+ atime: 0x82a37b40:00000001
+ mtime: 0x82a37b40:00000001
+crtime: 0x82a37b40:00000001
+times for year-2139 =
+ ctime: 0x3e9b9940:00000001
+ atime: 0x3e9b9940:00000001
+ mtime: 0x3e9b9940:00000001
+crtime: 0x3e9b9940:00000001
+times for year-1909 =
+ ctime: 0x8e475440:00000000
+ atime: 0x8e475440:00000000
+ mtime: 0x8e475440:00000000
+crtime: 0x8e475440:00000000
+times for year-1979 =
+ ctime: 0x11db6940:00000000
+ atime: 0x11db6940:00000000
+ mtime: 0x11db6940:00000000
+crtime: 0x11db6940:00000000
+times for year-2039 =
+ ctime: 0x82a37b40:00000001
+ atime: 0x82a37b40:00000001
+ mtime: 0x82a37b40:00000001
+crtime: 0x82a37b40:00000001
+times for year-2139 =
+ ctime: 0x3e9b9940:00000001
+ atime: 0x3e9b9940:00000001
+ mtime: 0x3e9b9940:00000001
+crtime: 0x3e9b9940:00000001
+times for year-1909 =
+ ctime: 0x8e475440:00000003
+ atime: 0x8e475440:00000003
+ mtime: 0x8e475440:00000003
+crtime: 0x8e475440:00000003
diff --git a/tests/f_pre_1970_date_encoding/name b/tests/f_pre_1970_date_encoding/name
new file mode 100644
index 0000000..9805324
--- /dev/null
+++ b/tests/f_pre_1970_date_encoding/name
@@ -0,0 +1 @@ 
+correct mis-encoded pre-1970 dates
diff --git a/tests/f_pre_1970_date_encoding/script b/tests/f_pre_1970_date_encoding/script
new file mode 100644
index 0000000..c3e12f5
--- /dev/null
+++ b/tests/f_pre_1970_date_encoding/script
@@ -0,0 +1,96 @@ 
+if test -x $DEBUGFS_EXE; then
+
+OUT=$test_name.log
+TIMESTAMPS=$test_name.timestamps.log
+EXP=$test_dir/expect
+FSCK_OPT=-yf
+
+create_file_with_xtime_and_extra() {
+    name=$1
+    time=$2
+    extra=$3
+    $DEBUGFS -w -R "write /dev/null $name" $TMPFILE > $OUT 2>&1
+    for xtime in atime ctime mtime crtime
+    do
+        $DEBUGFS -w -R "set_inode_field $name $xtime @$time" $TMPFILE > $OUT 2>&1
+
+        $DEBUGFS -w -R "set_inode_field $name ${xtime}_extra $extra" $TMPFILE > $OUT 2>&1
+    done
+}
+
+get_file_xtime_and_extra() {
+    name=$1
+    echo "times for $name =" >> $TIMESTAMPS
+    $DEBUGFS -R "stat $name" $TMPFILE 2>&1 | egrep '^( a| c| m|cr)time: ' |sed 's/ --.*//' >> $TIMESTAMPS
+}
+
+rm -f $OUT
+rm -f $TIMESTAMPS
+
+#create an empty ext4 filesystem with 256-byte inodes for testing
+dd status=none if=/dev/zero of=$TMPFILE bs=1024 count=5000
+echo  mkfs.ext4 -q -I 256 $TMPFILE >> $OUT
+yes | mkfs.ext4 -q -I 256 $TMPFILE >> $OUT 2>&1
+
+# this is a pre-1970 file encoded with the old encoding.
+# fsck should repair this
+create_file_with_xtime_and_extra year-1909 -1907928000 3
+
+# these are all already encoded correctly
+create_file_with_xtime_and_extra year-1979   299592000 0
+create_file_with_xtime_and_extra year-2039  2191752000 1
+create_file_with_xtime_and_extra year-2139  5345352000 1
+
+# confirm that the xtime is wrong on the pre-1970 file
+get_file_xtime_and_extra year-1909
+
+# and confirm that it is right on the remaining files
+get_file_xtime_and_extra year-1979
+get_file_xtime_and_extra year-2039
+get_file_xtime_and_extra year-2139
+
+# before we repair the filesystem, save off a copy so that
+# we can use it later
+
+cp $TMPFILE $TMPFILE.backup
+
+# repair the filesystem
+E2FSCK_TIME=1386393539 $FSCK $FSCK_OPT $TMPFILE >> $OUT 2>&1
+
+# check that the dates and xtime_extra on the file is now correct
+get_file_xtime_and_extra year-1909
+
+# check that the remaining dates have not been altered
+get_file_xtime_and_extra year-1979
+get_file_xtime_and_extra year-2039
+get_file_xtime_and_extra year-2139
+
+# now we need to check that after the year 2242, e2fsck does not
+# modify dates with extra_xtime=3
+
+# restore the unrepaired filesystem
+mv $TMPFILE.backup $TMPFILE
+
+#retry the repair
+E2FSCK_TIME=9270393539 $FSCK $FSCK_OPT $TMPFILE >> $OUT 2>&1
+
+# check that the 1909 file is unaltered (i.e. it has a post-2378 date)
+get_file_xtime_and_extra year-1909
+
+cmp -s $TIMESTAMPS $EXP
+status=$?
+
+if [ "$status" = 0 ] ; then
+	echo "$test_name: $test_description: ok"
+	touch $test_name.ok
+else
+	echo "$test_name: $test_description: failed"
+	diff $DIFF_OPTS $EXP $TIMESTAMPS > $test_name.failed
+fi
+
+unset OUT TIMESTAMPS EXP FSCK_OPT
+
+else #if test -x $DEBUGFS_EXE; then
+	echo "$test_name: $test_description: skipped"
+fi
+