| Message ID | 1386446560.10748.5.camel@chiang |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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 +
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