* [PATCH] ext4: Fix reading of extended tv_sec (bug 23732) @ 2013-11-07 7:16 David Turner 2013-11-07 16:03 ` Jan Kara 0 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-07 7:16 UTC (permalink / raw) To: linux-ext4; +Cc: linux-kernel, Andreas Dilger, Theodore Ts'o In ext4, the bottom two bits of {a,c,m}time_extra are used to extend the {a,c,m}time fields, deferring the year 2038 problem to the year 2446. The representation (which this patch does not alter) is a bit hackish, in that the most-significant bit is no longer (alone) sufficient to indicate the sign. That's because we're representing an asymmetric range, with seven times as many positive values as negative. When decoding these extended fields, for times whose bottom 32 bits would represent a negative number, sign extension causes the 64-bit extended timestamp to be negative as well, which is not what's intended. This patch corrects that issue, so that the only negative {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed timestamps). Signed-off-by: David Turner <novalis@novalis.org> Reported-by: Mark Harris <mh8928@yahoo.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 --- fs/ext4/ext4.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index af815ea..7b73c26 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -722,10 +722,15 @@ static inline __le32 ext4_encode_extra_time(struct timespec *time) static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) { - if (sizeof(time->tv_sec) > 4) - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) - << 32; - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; + if (sizeof(time->tv_sec) > 4) { + u64 extra_bits = (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK); + if (time->tv_sec > 0 || extra_bits != EXT4_EPOCH_MASK) { + time->tv_sec &= 0xFFFFFFFF; + time->tv_sec |= extra_bits << 32; + } + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> + EXT4_EPOCH_BITS; + } } #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: Fix reading of extended tv_sec (bug 23732) 2013-11-07 7:16 [PATCH] ext4: Fix reading of extended tv_sec (bug 23732) David Turner @ 2013-11-07 16:03 ` Jan Kara 2013-11-07 22:54 ` [PATCH v2] " David Turner 0 siblings, 1 reply; 39+ messages in thread From: Jan Kara @ 2013-11-07 16:03 UTC (permalink / raw) To: David Turner; +Cc: linux-ext4, linux-kernel, Andreas Dilger, Theodore Ts'o On Thu 07-11-13 02:16:30, David Turner wrote: > In ext4, the bottom two bits of {a,c,m}time_extra are used to extend > the {a,c,m}time fields, deferring the year 2038 problem to the year > 2446. The representation (which this patch does not alter) is a bit > hackish, in that the most-significant bit is no longer (alone) > sufficient to indicate the sign. That's because we're representing an > asymmetric range, with seven times as many positive values as > negative. > > When decoding these extended fields, for times whose bottom 32 bits > would represent a negative number, sign extension causes the 64-bit > extended timestamp to be negative as well, which is not what's > intended. This patch corrects that issue, so that the only negative > {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed > timestamps). > > Signed-off-by: David Turner <novalis@novalis.org> > Reported-by: Mark Harris <mh8928@yahoo.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 > --- > fs/ext4/ext4.h | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index af815ea..7b73c26 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -722,10 +722,15 @@ static inline __le32 ext4_encode_extra_time(struct timespec *time) > > static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) > { > - if (sizeof(time->tv_sec) > 4) > - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > - << 32; > - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; > + if (sizeof(time->tv_sec) > 4) { > + u64 extra_bits = (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK); The extra cast to (__u64) looks useless here. > + if (time->tv_sec > 0 || extra_bits != EXT4_EPOCH_MASK) { > + time->tv_sec &= 0xFFFFFFFF; > + time->tv_sec |= extra_bits << 32; > + } > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> > + EXT4_EPOCH_BITS; > + } > } So I'm somewhat wondering: Previously we decoded tv_nsec regardless of tv_sec size. After your patch we do it only if sizeof(time->tv_sec) > 4. Is this an intended change? Why is it OK? Honza -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v2] ext4: Fix reading of extended tv_sec (bug 23732) 2013-11-07 16:03 ` Jan Kara @ 2013-11-07 22:54 ` David Turner 2013-11-07 23:14 ` Jan Kara 0 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-07 22:54 UTC (permalink / raw) To: Jan Kara; +Cc: linux-ext4, linux-kernel, Andreas Dilger, Theodore Ts'o On Thu, 2013-11-07 at 17:03 +0100, Jan Kara wrote: > So I'm somewhat wondering: Previously we decoded tv_nsec regardless of > tv_sec size. After your patch we do it only if sizeof(time->tv_sec) > 4. Is > this an intended change? Why is it OK? This is an error. Here is a corrected version of the patch. -- In ext4, the bottom two bits of {a,c,m}time_extra are used to extend the {a,c,m}time fields, deferring the year 2038 problem to the year 2446. The representation (which this patch does not alter) is a bit hackish, in that the most-significant bit is no longer (alone) sufficient to indicate the sign. That's because we're representing an asymmetric range, with seven times as many positive values as negative. When decoding these extended fields, for times whose bottom 32 bits would represent a negative number, sign extension causes the 64-bit extended timestamp to be negative as well, which is not what's intended. This patch corrects that issue, so that the only negative {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed timestamps). Signed-off-by: David Turner <novalis@novalis.org> Reported-by: Mark Harris <mh8928@yahoo.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 --- fs/ext4/ext4.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index af815ea..3c2d0b3 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -722,10 +722,15 @@ static inline __le32 ext4_encode_extra_time(struct timespec *time) static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) { - if (sizeof(time->tv_sec) > 4) - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) - << 32; - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; + if (sizeof(time->tv_sec) > 4) { + u64 extra_bits = (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK); + if (time->tv_sec > 0 || extra_bits != EXT4_EPOCH_MASK) { + time->tv_sec &= 0xFFFFFFFF; + time->tv_sec |= extra_bits << 32; + } + } + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> + EXT4_EPOCH_BITS; } #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2] ext4: Fix reading of extended tv_sec (bug 23732) 2013-11-07 22:54 ` [PATCH v2] " David Turner @ 2013-11-07 23:14 ` Jan Kara 2013-11-07 23:26 ` [PATCH v3] " David Turner 0 siblings, 1 reply; 39+ messages in thread From: Jan Kara @ 2013-11-07 23:14 UTC (permalink / raw) To: David Turner Cc: Jan Kara, linux-ext4, linux-kernel, Andreas Dilger, Theodore Ts'o On Thu 07-11-13 17:54:24, David Turner wrote: > On Thu, 2013-11-07 at 17:03 +0100, Jan Kara wrote: > > So I'm somewhat wondering: Previously we decoded tv_nsec regardless of > > tv_sec size. After your patch we do it only if sizeof(time->tv_sec) > 4. Is > > this an intended change? Why is it OK? > > This is an error. Here is a corrected version of the patch. > > > -- > > In ext4, the bottom two bits of {a,c,m}time_extra are used to extend > the {a,c,m}time fields, deferring the year 2038 problem to the year > 2446. The representation (which this patch does not alter) is a bit > hackish, in that the most-significant bit is no longer (alone) > sufficient to indicate the sign. That's because we're representing an > asymmetric range, with seven times as many positive values as > negative. > > When decoding these extended fields, for times whose bottom 32 bits > would represent a negative number, sign extension causes the 64-bit > extended timestamp to be negative as well, which is not what's > intended. This patch corrects that issue, so that the only negative > {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed > timestamps). > > Signed-off-by: David Turner <novalis@novalis.org> > Reported-by: Mark Harris <mh8928@yahoo.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 > --- > fs/ext4/ext4.h | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index af815ea..3c2d0b3 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -722,10 +722,15 @@ static inline __le32 ext4_encode_extra_time(struct timespec *time) > > static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) > { > - if (sizeof(time->tv_sec) > 4) > - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > - << 32; > - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; > + if (sizeof(time->tv_sec) > 4) { > + u64 extra_bits = (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK); ^^^^ Still unnecessary type cast here (but that's a cosmetic issue). Otherwise the patch looks good. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > + if (time->tv_sec > 0 || extra_bits != EXT4_EPOCH_MASK) { > + time->tv_sec &= 0xFFFFFFFF; > + time->tv_sec |= extra_bits << 32; > + } > + } > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> > + EXT4_EPOCH_BITS; > } > > #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ > -- > 1.8.1.2 > > > -- Jan Kara <jack@suse.cz> SUSE Labs, CR ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v3] ext4: Fix reading of extended tv_sec (bug 23732) 2013-11-07 23:14 ` Jan Kara @ 2013-11-07 23:26 ` David Turner 2013-11-08 5:17 ` Theodore Ts'o 2013-11-08 21:37 ` Andreas Dilger 0 siblings, 2 replies; 39+ messages in thread From: David Turner @ 2013-11-07 23:26 UTC (permalink / raw) To: Jan Kara; +Cc: linux-ext4, linux-kernel, Andreas Dilger, Theodore Ts'o On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: > Still unnecessary type cast here (but that's a cosmetic issue). ... > Otherwise the patch looks good. You can add: > Reviewed-by: Jan Kara <jack@suse.cz> Thanks. A version with this correction and the reviewed-by follows. -- In ext4, the bottom two bits of {a,c,m}time_extra are used to extend the {a,c,m}time fields, deferring the year 2038 problem to the year 2446. The representation (which this patch does not alter) is a bit hackish, in that the most-significant bit is no longer (alone) sufficient to indicate the sign. That's because we're representing an asymmetric range, with seven times as many positive values as negative. When decoding these extended fields, for times whose bottom 32 bits would represent a negative number, sign extension causes the 64-bit extended timestamp to be negative as well, which is not what's intended. This patch corrects that issue, so that the only negative {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed timestamps). Signed-off-by: David Turner <novalis@novalis.org> Reported-by: Mark Harris <mh8928@yahoo.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 Reviewed-by: Jan Kara <jack@suse.cz> --- fs/ext4/ext4.h | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index af815ea..3c2d0b3 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -722,10 +722,15 @@ static inline __le32 ext4_encode_extra_time(struct timespec *time) static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) { - if (sizeof(time->tv_sec) > 4) - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) - << 32; - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; + if (sizeof(time->tv_sec) > 4) { + u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; + if (time->tv_sec > 0 || extra_bits != EXT4_EPOCH_MASK) { + time->tv_sec &= 0xFFFFFFFF; + time->tv_sec |= extra_bits << 32; + } + } + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> + EXT4_EPOCH_BITS; } #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v3] ext4: Fix reading of extended tv_sec (bug 23732) 2013-11-07 23:26 ` [PATCH v3] " David Turner @ 2013-11-08 5:17 ` Theodore Ts'o 2013-11-08 21:37 ` Andreas Dilger 1 sibling, 0 replies; 39+ messages in thread From: Theodore Ts'o @ 2013-11-08 5:17 UTC (permalink / raw) To: David Turner; +Cc: Jan Kara, linux-ext4, linux-kernel, Andreas Dilger On Thu, Nov 07, 2013 at 06:26:47PM -0500, David Turner wrote: > In ext4, the bottom two bits of {a,c,m}time_extra are used to extend > the {a,c,m}time fields, deferring the year 2038 problem to the year > 2446. The representation (which this patch does not alter) is a bit > hackish, in that the most-significant bit is no longer (alone) > sufficient to indicate the sign. That's because we're representing an > asymmetric range, with seven times as many positive values as > negative. > > When decoding these extended fields, for times whose bottom 32 bits > would represent a negative number, sign extension causes the 64-bit > extended timestamp to be negative as well, which is not what's > intended. This patch corrects that issue, so that the only negative > {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed > timestamps). > > Signed-off-by: David Turner <novalis@novalis.org> > Reported-by: Mark Harris <mh8928@yahoo.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 > Reviewed-by: Jan Kara <jack@suse.cz> Thanks, applied. - Ted ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v3] ext4: Fix reading of extended tv_sec (bug 23732) 2013-11-07 23:26 ` [PATCH v3] " David Turner 2013-11-08 5:17 ` Theodore Ts'o @ 2013-11-08 21:37 ` Andreas Dilger 2013-11-09 7:19 ` David Turner 1 sibling, 1 reply; 39+ messages in thread From: Andreas Dilger @ 2013-11-08 21:37 UTC (permalink / raw) To: David Turner Cc: Jan Kara, Ext4 Developers List, Linux Kernel Mailing List, Theodore Ts'o On Nov 7, 2013, at 4:26 PM, David Turner <novalis@novalis.org> wrote: > On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: >> Still unnecessary type cast here (but that's a cosmetic issue). > ... >> Otherwise the patch looks good. You can add: >> Reviewed-by: Jan Kara <jack@suse.cz> > > Thanks. A version with this correction and the reviewed-by follows. Thanks for working on this. It was (seriously) in my list of things to get done, but I figured I still had a few years to work on it... My (unfinished) version of this patch had a nice comment at ext4_encode_time() that explained the encoding that is being used very clearly: /* * We need is an encoding that preserves the times for extra epoch "00": * * extra msb of adjust for signed * epoch 32-bit 32-bit tv_sec to * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 */ It seems that your version of the patch seems to use a different encoding. Not that this is a problem, per-se, since my patch wasn’t in use anywhere, but it would be nice to have a similarly clear explanation of what the mapping is so that it can be clearly understood. My ext4_{encode,decode}_extra_time() used add/subtract instead of mask/OR ops, which changed the on-disk format for times beyond 2038, but those were already broken, and presumably not in use by anyone yet. However, it seemed to me this was easier to produce the correct results. Have you tested your patch with a variety of timestamps to verify its correctness? It looks to me like you have mapped the 1901-1969 range onto 0x3 for the epoch bits, instead of the (IMHO) natural 0x0 bits. The critical time ranges are listed above. Cheers, Andreas > -- > In ext4, the bottom two bits of {a,c,m}time_extra are used to extend > the {a,c,m}time fields, deferring the year 2038 problem to the year > 2446. The representation (which this patch does not alter) is a bit > hackish, in that the most-significant bit is no longer (alone) > sufficient to indicate the sign. That's because we're representing an > asymmetric range, with seven times as many positive values as > negative. > > When decoding these extended fields, for times whose bottom 32 bits > would represent a negative number, sign extension causes the 64-bit > extended timestamp to be negative as well, which is not what's > intended. This patch corrects that issue, so that the only negative > {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed > timestamps). > > Signed-off-by: David Turner <novalis@novalis.org> > Reported-by: Mark Harris <mh8928@yahoo.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 > Reviewed-by: Jan Kara <jack@suse.cz> > --- > fs/ext4/ext4.h | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index af815ea..3c2d0b3 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -722,10 +722,15 @@ static inline __le32 ext4_encode_extra_time(struct timespec *time) > > static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) > { > - if (sizeof(time->tv_sec) > 4) > - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > - << 32; > - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; > + if (sizeof(time->tv_sec) > 4) { > + u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; > + if (time->tv_sec > 0 || extra_bits != EXT4_EPOCH_MASK) { > + time->tv_sec &= 0xFFFFFFFF; > + time->tv_sec |= extra_bits << 32; > + } > + } > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> > + EXT4_EPOCH_BITS; > } > > #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ > -- > 1.8.1.2 > > > Cheers, Andreas ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-08 21:37 ` Andreas Dilger @ 2013-11-09 7:19 ` David Turner 0 siblings, 0 replies; 39+ messages in thread From: David Turner @ 2013-11-09 7:19 UTC (permalink / raw) To: Andreas Dilger Cc: Jan Kara, Ext4 Developers List, Linux Kernel Mailing List, Theodore Ts'o On Fri, 2013-11-08 at 14:37 -0700, Andreas Dilger wrote: > On Nov 7, 2013, at 4:26 PM, David Turner <novalis@novalis.org> wrote: > > On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: > >> Still unnecessary type cast here (but that's a cosmetic issue). > > ... > >> Otherwise the patch looks good. You can add: > >> Reviewed-by: Jan Kara <jack@suse.cz> > > > > Thanks. A version with this correction and the reviewed-by follows. > > Thanks for working on this. It was (seriously) in my list of things to > get done, but I figured I still had a few years to work on it... > > My (unfinished) version of this patch had a nice comment at ext4_encode_time() > that explained the encoding that is being used very clearly: > > /* > * We need is an encoding that preserves the times for extra epoch "00": > * > * extra msb of adjust for signed > * epoch 32-bit 32-bit tv_sec to > * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range > * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 > * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 > * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 > * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 > * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 > * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 > * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 > * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 > */ > > It seems that your version of the patch seems to use a different encoding. Not > that this is a problem, per-se, since my patch wasn’t in use anywhere, but it > would be nice to have a similarly clear explanation of what the mapping is so > that it can be clearly understood. I have included a patch with an explanation (the patch is against tytso/dev -- I hope that's the correct place). > My ext4_{encode,decode}_extra_time() used add/subtract instead of mask/OR ops, > which changed the on-disk format for times beyond 2038, but those were already > broken, and presumably not in use by anyone yet. They were actually correct according to my patch's encoding (that is, my patch used the existing encoding). The existing encoding was written correctly, but read wrongly. As you say, this should not matter, since nobody should be writing these timestamps, but I assumed that the existing encoding had happened for a reason, and wanted to make the minimal change. If you believe it is important, I would be happy to change it. > However, it seemed to me this > was easier to produce the correct results. Have you tested your patch with > a variety of timestamps to verify its correctness? I tested by using touch -d to create one file in each year between 1902 and 2446. Then I unmounted and remounted the FS, and did ls -l and manually verified that each file's date matched its name. > It looks to me like you > have mapped the 1901-1969 range onto 0x3 for the epoch bits, instead > of the (IMHO) natural 0x0 bits. The critical time ranges are listed > above. I think the idea of this is that it is the bottom 34 bits of the 64-bit signed time. However, it occurs to me that this relies on a two's complement machine. Even though the C standard does not guarantee this, I believe the kernel requires it, so that's probably OK. Patch follows: -- Add a comment explaining the encoding of ext4's extra {a,c,m}time bits. Signed-off-by: David Turner <novalis@novalis.org> --- fs/ext4/ext4.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 121da383..ab69f14 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -713,6 +713,24 @@ struct move_extent { sizeof((ext4_inode)->field)) \ <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ +/* + * We use the bottom 34 bits of the signed 64-bit time value, with + * the top two of these bits in the bottom of extra. This leads + * to a slightly odd encoding, which works like this: + * + * extra msb of + * epoch 32-bit + * bits time decoded 64-bit tv_sec valid time range + * 0 0 0 0x000000000..0x07fffffff 1970-01-01..2038-01-19 + * 0 0 1 0x080000000..0x0ffffffff 2038-01-19..2106-02-07 + * 0 1 0 0x100000000..0x17fffffff 2106-02-07..2174-02-25 + * 0 1 1 0x180000000..0x1ffffffff 2174-02-25..2242-03-16 + * 1 0 0 0x200000000..0x27fffffff 2242-03-16..2310-04-04 + * 1 0 1 0x280000000..0x2ffffffff 2310-04-04..2378-04-22 + * 1 1 0 0x300000000..0x37fffffff 2378-04-22..2446-05-10 + + * 1 1 1 -0x80000000..-0x00000001 1901-12-13..1969-12-31 + */ static inline __le32 ext4_encode_extra_time(struct timespec *time) { -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH] ext4: explain encoding of 34-bit a,c,mtime values @ 2013-11-09 7:19 ` David Turner 0 siblings, 0 replies; 39+ messages in thread From: David Turner @ 2013-11-09 7:19 UTC (permalink / raw) To: Andreas Dilger Cc: Jan Kara, Ext4 Developers List, Linux Kernel Mailing List, Theodore Ts'o On Fri, 2013-11-08 at 14:37 -0700, Andreas Dilger wrote: > On Nov 7, 2013, at 4:26 PM, David Turner <novalis@novalis.org> wrote: > > On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: > >> Still unnecessary type cast here (but that's a cosmetic issue). > > ... > >> Otherwise the patch looks good. You can add: > >> Reviewed-by: Jan Kara <jack@suse.cz> > > > > Thanks. A version with this correction and the reviewed-by follows. > > Thanks for working on this. It was (seriously) in my list of things to > get done, but I figured I still had a few years to work on it... > > My (unfinished) version of this patch had a nice comment at ext4_encode_time() > that explained the encoding that is being used very clearly: > > /* > * We need is an encoding that preserves the times for extra epoch "00": > * > * extra msb of adjust for signed > * epoch 32-bit 32-bit tv_sec to > * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range > * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 > * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 > * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 > * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 > * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 > * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 > * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 > * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 > */ > > It seems that your version of the patch seems to use a different encoding. Not > that this is a problem, per-se, since my patch wasn’t in use anywhere, but it > would be nice to have a similarly clear explanation of what the mapping is so > that it can be clearly understood. I have included a patch with an explanation (the patch is against tytso/dev -- I hope that's the correct place). > My ext4_{encode,decode}_extra_time() used add/subtract instead of mask/OR ops, > which changed the on-disk format for times beyond 2038, but those were already > broken, and presumably not in use by anyone yet. They were actually correct according to my patch's encoding (that is, my patch used the existing encoding). The existing encoding was written correctly, but read wrongly. As you say, this should not matter, since nobody should be writing these timestamps, but I assumed that the existing encoding had happened for a reason, and wanted to make the minimal change. If you believe it is important, I would be happy to change it. > However, it seemed to me this > was easier to produce the correct results. Have you tested your patch with > a variety of timestamps to verify its correctness? I tested by using touch -d to create one file in each year between 1902 and 2446. Then I unmounted and remounted the FS, and did ls -l and manually verified that each file's date matched its name. > It looks to me like you > have mapped the 1901-1969 range onto 0x3 for the epoch bits, instead > of the (IMHO) natural 0x0 bits. The critical time ranges are listed > above. I think the idea of this is that it is the bottom 34 bits of the 64-bit signed time. However, it occurs to me that this relies on a two's complement machine. Even though the C standard does not guarantee this, I believe the kernel requires it, so that's probably OK. Patch follows: -- Add a comment explaining the encoding of ext4's extra {a,c,m}time bits. Signed-off-by: David Turner <novalis@novalis.org> --- fs/ext4/ext4.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 121da383..ab69f14 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -713,6 +713,24 @@ struct move_extent { sizeof((ext4_inode)->field)) \ <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ +/* + * We use the bottom 34 bits of the signed 64-bit time value, with + * the top two of these bits in the bottom of extra. This leads + * to a slightly odd encoding, which works like this: + * + * extra msb of + * epoch 32-bit + * bits time decoded 64-bit tv_sec valid time range + * 0 0 0 0x000000000..0x07fffffff 1970-01-01..2038-01-19 + * 0 0 1 0x080000000..0x0ffffffff 2038-01-19..2106-02-07 + * 0 1 0 0x100000000..0x17fffffff 2106-02-07..2174-02-25 + * 0 1 1 0x180000000..0x1ffffffff 2174-02-25..2242-03-16 + * 1 0 0 0x200000000..0x27fffffff 2242-03-16..2310-04-04 + * 1 0 1 0x280000000..0x2ffffffff 2310-04-04..2378-04-22 + * 1 1 0 0x300000000..0x37fffffff 2378-04-22..2446-05-10 + + * 1 1 1 -0x80000000..-0x00000001 1901-12-13..1969-12-31 + */ static inline __le32 ext4_encode_extra_time(struct timespec *time) { -- 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-09 7:19 ` David Turner @ 2013-11-09 23:51 ` Mark Harris -1 siblings, 0 replies; 39+ messages in thread From: Mark Harris @ 2013-11-09 23:51 UTC (permalink / raw) To: David Turner Cc: Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List, Theodore Ts'o On 8 November 2013 23:19, David Turner <novalis@novalis.org> wrote: > > On Fri, 2013-11-08 at 14:37 -0700, Andreas Dilger wrote: > > On Nov 7, 2013, at 4:26 PM, David Turner <novalis@novalis.org> wrote: > > > On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: > > >> Still unnecessary type cast here (but that's a cosmetic issue). > > > ... > > >> Otherwise the patch looks good. You can add: > > >> Reviewed-by: Jan Kara <jack@suse.cz> > > > > > > Thanks. A version with this correction and the reviewed-by follows. > > > > Thanks for working on this. It was (seriously) in my list of things to > > get done, but I figured I still had a few years to work on it... > > > > My (unfinished) version of this patch had a nice comment at ext4_encode_time() > > that explained the encoding that is being used very clearly: > > > > /* > > * We need is an encoding that preserves the times for extra epoch "00": > > * > > * extra msb of adjust for signed > > * epoch 32-bit 32-bit tv_sec to > > * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range > > * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 > > * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 > > * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 > > * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 > > * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 > > * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 > > * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 > > * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 > > */ > > > > It seems that your version of the patch seems to use a different encoding. Not > > that this is a problem, per-se, since my patch wasn’t in use anywhere, but it > > would be nice to have a similarly clear explanation of what the mapping is so > > that it can be clearly understood. > > I have included a patch with an explanation (the patch is against > tytso/dev -- I hope that's the correct place). > > > My ext4_{encode,decode}_extra_time() used add/subtract instead of mask/OR ops, > > which changed the on-disk format for times beyond 2038, but those were already > > broken, and presumably not in use by anyone yet. > > They were actually correct according to my patch's encoding (that is, my > patch used the existing encoding). The existing encoding was written > correctly, but read wrongly. As you say, this should not matter, since > nobody should be writing these timestamps, but I assumed that the > existing encoding had happened for a reason, and wanted to make the > minimal change. > > If you believe it is important, I would be happy to change it. The problem with the existing encoding is that pre-1970 dates are encoded with extra bits 1,1 in 64-bit kernels with ext4, but on 32-bit kernels and inodes that were originally written as ext3 the extra bits will be 0,0. Currently, both are decoded as pre-1970 dates. With your patch, only the 1,1 format used by 64-bit ext4 will decode as a pre-1970 date. Dates previously written by ext3 or a 32-bit kernel will no longer be decoded as expected. Also the patch does not update ext4_encode_extra_time to use this format for pre-1970 dates in 32-bit mode. Possible solutions were discussed here, but no decision was made: http://thread.gmane.org/gmane.comp.file-systems.ext4/26087/focus=26406 > > > > However, it seemed to me this > > was easier to produce the correct results. Have you tested your patch with > > a variety of timestamps to verify its correctness? > > I tested by using touch -d to create one file in each year between 1902 > and 2446. Then I unmounted and remounted the FS, and did ls -l and > manually verified that each file's date matched its name. > > > It looks to me like you > > have mapped the 1901-1969 range onto 0x3 for the epoch bits, instead > > of the (IMHO) natural 0x0 bits. The critical time ranges are listed > > above. > > I think the idea of this is that it is the bottom 34 bits of the 64-bit > signed time. However, it occurs to me that this relies on a two's > complement machine. Even though the C standard does not guarantee this, > I believe the kernel requires it, so that's probably OK. - Mark ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values @ 2013-11-09 23:51 ` Mark Harris 0 siblings, 0 replies; 39+ messages in thread From: Mark Harris @ 2013-11-09 23:51 UTC (permalink / raw) To: David Turner Cc: Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List, Theodore Ts'o On 8 November 2013 23:19, David Turner <novalis@novalis.org> wrote: > > On Fri, 2013-11-08 at 14:37 -0700, Andreas Dilger wrote: > > On Nov 7, 2013, at 4:26 PM, David Turner <novalis@novalis.org> wrote: > > > On Fri, 2013-11-08 at 00:14 +0100, Jan Kara wrote: > > >> Still unnecessary type cast here (but that's a cosmetic issue). > > > ... > > >> Otherwise the patch looks good. You can add: > > >> Reviewed-by: Jan Kara <jack@suse.cz> > > > > > > Thanks. A version with this correction and the reviewed-by follows. > > > > Thanks for working on this. It was (seriously) in my list of things to > > get done, but I figured I still had a few years to work on it... > > > > My (unfinished) version of this patch had a nice comment at ext4_encode_time() > > that explained the encoding that is being used very clearly: > > > > /* > > * We need is an encoding that preserves the times for extra epoch "00": > > * > > * extra msb of adjust for signed > > * epoch 32-bit 32-bit tv_sec to > > * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range > > * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 > > * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 > > * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 > > * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 > > * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 > > * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 > > * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 > > * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 > > */ > > > > It seems that your version of the patch seems to use a different encoding. Not > > that this is a problem, per-se, since my patch wasn’t in use anywhere, but it > > would be nice to have a similarly clear explanation of what the mapping is so > > that it can be clearly understood. > > I have included a patch with an explanation (the patch is against > tytso/dev -- I hope that's the correct place). > > > My ext4_{encode,decode}_extra_time() used add/subtract instead of mask/OR ops, > > which changed the on-disk format for times beyond 2038, but those were already > > broken, and presumably not in use by anyone yet. > > They were actually correct according to my patch's encoding (that is, my > patch used the existing encoding). The existing encoding was written > correctly, but read wrongly. As you say, this should not matter, since > nobody should be writing these timestamps, but I assumed that the > existing encoding had happened for a reason, and wanted to make the > minimal change. > > If you believe it is important, I would be happy to change it. The problem with the existing encoding is that pre-1970 dates are encoded with extra bits 1,1 in 64-bit kernels with ext4, but on 32-bit kernels and inodes that were originally written as ext3 the extra bits will be 0,0. Currently, both are decoded as pre-1970 dates. With your patch, only the 1,1 format used by 64-bit ext4 will decode as a pre-1970 date. Dates previously written by ext3 or a 32-bit kernel will no longer be decoded as expected. Also the patch does not update ext4_encode_extra_time to use this format for pre-1970 dates in 32-bit mode. Possible solutions were discussed here, but no decision was made: http://thread.gmane.org/gmane.comp.file-systems.ext4/26087/focus=26406 > > > > However, it seemed to me this > > was easier to produce the correct results. Have you tested your patch with > > a variety of timestamps to verify its correctness? > > I tested by using touch -d to create one file in each year between 1902 > and 2446. Then I unmounted and remounted the FS, and did ls -l and > manually verified that each file's date matched its name. > > > It looks to me like you > > have mapped the 1901-1969 range onto 0x3 for the epoch bits, instead > > of the (IMHO) natural 0x0 bits. The critical time ranges are listed > > above. > > I think the idea of this is that it is the bottom 34 bits of the 64-bit > signed time. However, it occurs to me that this relies on a two's > complement machine. Even though the C standard does not guarantee this, > I believe the kernel requires it, so that's probably OK. - Mark -- 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-09 23:51 ` Mark Harris (?) @ 2013-11-10 7:56 ` David Turner 2013-11-12 0:30 ` Theodore Ts'o -1 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-10 7:56 UTC (permalink / raw) To: Mark Harris Cc: Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List, Theodore Ts'o On Sat, 2013-11-09 at 15:51 -0800, Mark Harris wrote: > > The problem with the existing encoding is that pre-1970 dates are > encoded with extra bits 1,1 in 64-bit kernels with ext4, but on 32-bit > kernels and inodes that were originally written as ext3 the extra bits > will be 0,0. Currently, both are decoded as pre-1970 dates. > > With your patch, only the 1,1 format used by 64-bit ext4 will decode > as a pre-1970 date. Dates previously written by ext3 or a 32-bit > kernel will no longer be decoded as expected. Also the patch does > not update ext4_encode_extra_time to use this format for pre-1970 > dates in 32-bit mode. You're right -- I missed the complexity here. > Possible solutions were discussed here, but no decision was made: > http://thread.gmane.org/gmane.comp.file-systems.ext4/26087/focus=26406 To summarize, the previous discussion offered four possible solutions, of which two were thought good: b. Use Andreas's encoding, which is incompatible with pre-1970 files written on 64-bit systems. c. Use an encoding which is compatible with all pre-1970 files, but incompatible with 64-bit post-2038 files, and which encodes a smaller range of time and is more complicated. ------- I don't care about currently-existing post-2038 files, because I believe that nobody has a valid reason to have such files. However, I do believe that pre-1970 files are probably important to someone. Despite this, I prefer option (b), because I think the simplicity is valuable, and because I hate to give up date ranges (even ones that I think we'll "never" need). Option (b) is not actually lossy, because we could correct pre-1970 files with e2fsck; under Andreas's encoding, their dates would be in the far future (and thus cannot be legitimate). Would a patch that does (b) be accepted? I would accompany it with a patch to e2fsck (which I assume would also go to the ext4 developers mailing list?). ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-10 7:56 ` David Turner @ 2013-11-12 0:30 ` Theodore Ts'o 2013-11-12 21:35 ` Andreas Dilger ` (2 more replies) 0 siblings, 3 replies; 39+ messages in thread From: Theodore Ts'o @ 2013-11-12 0:30 UTC (permalink / raw) To: David Turner Cc: Mark Harris, Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > b. Use Andreas's encoding, which is incompatible with pre-1970 files > written on 64-bit systems. > > I don't care about currently-existing post-2038 files, because I believe > that nobody has a valid reason to have such files. However, I do > believe that pre-1970 files are probably important to someone. > > Despite this, I prefer option (b), because I think the simplicity is > valuable, and because I hate to give up date ranges (even ones that I > think we'll "never" need). Option (b) is not actually lossy, because we > could correct pre-1970 files with e2fsck; under Andreas's encoding, > their dates would be in the far future (and thus cannot be legitimate). > > Would a patch that does (b) be accepted? I would accompany it with a > patch to e2fsck (which I assume would also go to the ext4 developers > mailing list?). I agree, I think this is the best way to go. I'm going to drop your earlier patch, and wait for an updated patch from you. It may miss this merge window, but as Andreas has pointed out, we still have a few years to get this right. :-) Thanks!! - Ted ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-12 0:30 ` Theodore Ts'o @ 2013-11-12 21:35 ` Andreas Dilger 2013-11-13 7:00 ` [PATCH v4 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner 2013-11-13 7:00 ` [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 2013-11-12 23:03 ` [PATCH] ext4: explain encoding of 34-bit a,c,mtime values Darrick J. Wong 2014-01-22 6:22 ` Darrick J. Wong 2 siblings, 2 replies; 39+ messages in thread From: Andreas Dilger @ 2013-11-12 21:35 UTC (permalink / raw) To: Theodore Ts'o Cc: David Turner, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 2004 bytes --] On Nov 11, 2013, at 5:30 PM, Theodore Ts'o <tytso@mit.edu> wrote: > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: >> b. Use Andreas's encoding, which is incompatible with pre-1970 files >> written on 64-bit systems. >> >> I don't care about currently-existing post-2038 files, because I believe >> that nobody has a valid reason to have such files. However, I do >> believe that pre-1970 files are probably important to someone. >> >> Despite this, I prefer option (b), because I think the simplicity is >> valuable, and because I hate to give up date ranges (even ones that I >> think we'll "never" need). Option (b) is not actually lossy, because we >> could correct pre-1970 files with e2fsck; under Andreas's encoding, >> their dates would be in the far future (and thus cannot be legitimate). >> >> Would a patch that does (b) be accepted? I would accompany it with a >> patch to e2fsck (which I assume would also go to the ext4 developers >> mailing list?). > > I agree, I think this is the best way to go. I'm going to drop your > earlier patch, and wait for an updated patch from you. It may miss > this merge window, but as Andreas has pointed out, we still have a few > years to get this right. :-) Since this change would immediately break files encoded with pre-1970 dates on ext4 filesystems, should there be a transition period where both pre-1970 dates (with extra epoch bits == 0x3 in the current encoding) and post-2378 (with extra epoch bits == 0x3 in the new encoding) are decoded as being pre-1970? That could be conditional until some release in the future (e.g. >= Linux 4.20, at least 5 years away) to give folks a chance to run the new e2fsck to fix up those files. Are there really any ext4 filesystems that have files with valid dates so old? I don’t want to break anyone’s data, but if this extra complexity is completely pointless then I’m also fine with this minor risk of breakage. Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP using GPGMail --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v4 1/2] ext4: Fix handling of extended tv_sec (bug 23732) 2013-11-12 21:35 ` Andreas Dilger @ 2013-11-13 7:00 ` David Turner 2013-11-13 8:19 ` Darrick J. Wong 2013-11-13 7:00 ` [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 1 sibling, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-13 7:00 UTC (permalink / raw) To: Andreas Dilger Cc: Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List In ext4, the bottom two bits of {a,c,m}time_extra are used to extend the {a,c,m}time fields, deferring the year 2038 problem to the year 2446. When decoding these extended fields, for times whose bottom 32 bits would represent a negative number, sign extension causes the 64-bit extended timestamp to be negative as well, which is not what's intended. This patch corrects that issue, so that the only negative {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed timestamps). Some older kernels might have written pre-1970 dates with 1,1 in the extra bits. This patch treats those incorrectly-encoded dates as pre-1970, instead of post-2311, until kernel 4.20 is released. Hopefully by then e2fsck will have fixed up the bad data. Signed-off-by: David Turner <novalis@novalis.org> Reported-by: Mark Harris <mh8928@yahoo.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 --- fs/ext4/ext4.h | 61 +++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 18aa56b..7d5e019 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -26,6 +26,7 @@ #include <linux/seqlock.h> #include <linux/mutex.h> #include <linux/timer.h> +#include <linux/version.h> #include <linux/wait.h> #include <linux/blockgroup_lock.h> #include <linux/percpu_counter.h> @@ -713,38 +714,54 @@ struct move_extent { sizeof((ext4_inode)->field)) \ <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ + /* - * We use the bottom 34 bits of the signed 64-bit time value, with - * the top two of these bits in the bottom of extra. This leads - * to a slightly odd encoding, which works like this: + * We need is an encoding that preserves the times for extra epoch "00": * - * extra msb of - * epoch 32-bit - * bits time decoded 64-bit tv_sec valid time range - * 0 0 0 0x000000000..0x07fffffff 1970-01-01..2038-01-19 - * 0 0 1 0x080000000..0x0ffffffff 2038-01-19..2106-02-07 - * 0 1 0 0x100000000..0x17fffffff 2106-02-07..2174-02-25 - * 0 1 1 0x180000000..0x1ffffffff 2174-02-25..2242-03-16 - * 1 0 0 0x200000000..0x27fffffff 2242-03-16..2310-04-04 - * 1 0 1 0x280000000..0x2ffffffff 2310-04-04..2378-04-22 - * 1 1 0 0x300000000..0x37fffffff 2378-04-22..2446-05-10 - - * 1 1 1 -0x80000000..-0x00000001 1901-12-13..1969-12-31 + * extra msb of adjust for signed + * epoch 32-bit 32-bit tv_sec to + * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range + * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 + * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 + * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 + * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 + * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 + * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 + * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 + * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 + * + * Note that previous versions of the kernel on 64-bit systems would + * incorrectly use extra epoch bits 1,1 for dates between 1901 and + * 1970. e2fsck will correct this, assuming that it is run on the + * affected filesystem before 2311. */ static inline __le32 ext4_encode_extra_time(struct timespec *time) { - return cpu_to_le32((sizeof(time->tv_sec) > 4 ? - (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) | - ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK)); + u32 extra = sizeof(time->tv_sec) > 4 ? + ((time->tv_sec - (s32)time->tv_sec) >> 32) & EXT4_EPOCH_MASK : 0; + return cpu_to_le32(extra | (time->tv_nsec << EXT4_EPOCH_BITS)); } static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) { - if (sizeof(time->tv_sec) > 4) - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) - << 32; - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; + if (unlikely(sizeof(time->tv_sec) > 4 && + (extra & cpu_to_le32(EXT4_EPOCH_MASK)))) { +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,20,0) + /* Handle legacy encoding of pre-1970 dates with epoch + * bits 1,1. We assume that by kernel version 4.20, + * everyone will have run fsck over the affected + * filesystems to correct the problem. + */ + u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; + if (extra_bits == 3) + extra_bits = 0; + time->tv_sec += extra_bits << 32; +#else + time->tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; +#endif + } + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; } #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v4 1/2] ext4: Fix handling of extended tv_sec (bug 23732) 2013-11-13 7:00 ` [PATCH v4 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner @ 2013-11-13 8:19 ` Darrick J. Wong 0 siblings, 0 replies; 39+ messages in thread From: Darrick J. Wong @ 2013-11-13 8:19 UTC (permalink / raw) To: David Turner Cc: Andreas Dilger, Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List On Wed, Nov 13, 2013 at 02:00:15AM -0500, David Turner wrote: > In ext4, the bottom two bits of {a,c,m}time_extra are used to extend > the {a,c,m}time fields, deferring the year 2038 problem to the year > 2446. > > When decoding these extended fields, for times whose bottom 32 bits > would represent a negative number, sign extension causes the 64-bit > extended timestamp to be negative as well, which is not what's > intended. This patch corrects that issue, so that the only negative > {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed > timestamps). > > Some older kernels might have written pre-1970 dates with 1,1 in the > extra bits. This patch treats those incorrectly-encoded dates as > pre-1970, instead of post-2311, until kernel 4.20 is released. > Hopefully by then e2fsck will have fixed up the bad data. > > Signed-off-by: David Turner <novalis@novalis.org> > Reported-by: Mark Harris <mh8928@yahoo.com> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 > --- > fs/ext4/ext4.h | 61 +++++++++++++++++++++++++++++++++++++--------------------- > 1 file changed, 39 insertions(+), 22 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 18aa56b..7d5e019 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -26,6 +26,7 @@ > #include <linux/seqlock.h> > #include <linux/mutex.h> > #include <linux/timer.h> > +#include <linux/version.h> > #include <linux/wait.h> > #include <linux/blockgroup_lock.h> > #include <linux/percpu_counter.h> > @@ -713,38 +714,54 @@ struct move_extent { > sizeof((ext4_inode)->field)) \ > <= (EXT4_GOOD_OLD_INODE_SIZE + \ > (einode)->i_extra_isize)) \ > + > /* > - * We use the bottom 34 bits of the signed 64-bit time value, with > - * the top two of these bits in the bottom of extra. This leads > - * to a slightly odd encoding, which works like this: > + * We need is an encoding that preserves the times for extra epoch "00": > * > - * extra msb of > - * epoch 32-bit > - * bits time decoded 64-bit tv_sec valid time range > - * 0 0 0 0x000000000..0x07fffffff 1970-01-01..2038-01-19 > - * 0 0 1 0x080000000..0x0ffffffff 2038-01-19..2106-02-07 > - * 0 1 0 0x100000000..0x17fffffff 2106-02-07..2174-02-25 > - * 0 1 1 0x180000000..0x1ffffffff 2174-02-25..2242-03-16 > - * 1 0 0 0x200000000..0x27fffffff 2242-03-16..2310-04-04 > - * 1 0 1 0x280000000..0x2ffffffff 2310-04-04..2378-04-22 > - * 1 1 0 0x300000000..0x37fffffff 2378-04-22..2446-05-10 > - > - * 1 1 1 -0x80000000..-0x00000001 1901-12-13..1969-12-31 > + * extra msb of adjust for signed > + * epoch 32-bit 32-bit tv_sec to > + * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range > + * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 > + * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 > + * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 > + * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 > + * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 > + * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 > + * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 > + * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 > + * > + * Note that previous versions of the kernel on 64-bit systems would > + * incorrectly use extra epoch bits 1,1 for dates between 1901 and > + * 1970. e2fsck will correct this, assuming that it is run on the > + * affected filesystem before 2311. Given the table, should this be "before 2310-04-04"? --D > */ > > static inline __le32 ext4_encode_extra_time(struct timespec *time) > { > - return cpu_to_le32((sizeof(time->tv_sec) > 4 ? > - (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) | > - ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK)); > + u32 extra = sizeof(time->tv_sec) > 4 ? > + ((time->tv_sec - (s32)time->tv_sec) >> 32) & EXT4_EPOCH_MASK : 0; > + return cpu_to_le32(extra | (time->tv_nsec << EXT4_EPOCH_BITS)); > } > > static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) > { > - if (sizeof(time->tv_sec) > 4) > - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) > - << 32; > - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; > + if (unlikely(sizeof(time->tv_sec) > 4 && > + (extra & cpu_to_le32(EXT4_EPOCH_MASK)))) { > +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,20,0) > + /* Handle legacy encoding of pre-1970 dates with epoch > + * bits 1,1. We assume that by kernel version 4.20, > + * everyone will have run fsck over the affected > + * filesystems to correct the problem. > + */ > + u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; > + if (extra_bits == 3) > + extra_bits = 0; > + time->tv_sec += extra_bits << 32; > +#else > + time->tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; > +#endif > + } > + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; > } > > #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ > -- > 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-12 21:35 ` Andreas Dilger 2013-11-13 7:00 ` [PATCH v4 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner @ 2013-11-13 7:00 ` David Turner 2013-11-13 7:56 ` Andreas Dilger 1 sibling, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-13 7:00 UTC (permalink / raw) To: Andreas Dilger Cc: Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List 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) { + return (xtime & (1 << 31)) != 0 && + (extra & EXT4_EPOCH_MASK) == EXT4_EPOCH_MASK; +} + +#define LARGE_INODE_EXTRA(inode, xtime) \ + 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. + */ +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 6 * (1UL << 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,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)) { + + 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 */ + { 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"), + PROMPT_FIX, 0 }, /* Pass 1b errors */ 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 + /* * Pass 1b errors */ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-13 7:00 ` [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner @ 2013-11-13 7:56 ` Andreas Dilger 2013-11-14 8:38 ` [PATCH v5 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner 2013-11-14 8:44 ` [PATCH v5 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 0 siblings, 2 replies; 39+ messages in thread From: Andreas Dilger @ 2013-11-13 7:56 UTC (permalink / raw) To: David Turner Cc: Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v5 1/2] ext4: Fix handling of extended tv_sec (bug 23732) 2013-11-13 7:56 ` Andreas Dilger @ 2013-11-14 8:38 ` David Turner 2013-11-14 8:44 ` [PATCH v5 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 1 sibling, 0 replies; 39+ messages in thread From: David Turner @ 2013-11-14 8:38 UTC (permalink / raw) To: Andreas Dilger Cc: Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List Thanks for your patient and detailed comments. -- In ext4, the bottom two bits of {a,c,m}time_extra are used to extend the {a,c,m}time fields, deferring the year 2038 problem to the year 2446. When decoding these extended fields, for times whose bottom 32 bits would represent a negative number, sign extension causes the 64-bit extended timestamp to be negative as well, which is not what's intended. This patch corrects that issue, so that the only negative {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed timestamps). Some older kernels might have written pre-1970 dates with 1,1 in the extra bits. This patch treats those incorrectly-encoded dates as pre-1970, instead of post-2311, until kernel 4.20 is released. Hopefully by then e2fsck will have fixed up the bad data. Signed-off-by: David Turner <novalis@novalis.org> Reported-by: Mark Harris <mh8928@yahoo.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 --- fs/ext4/ext4.h | 61 +++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index 18aa56b..ac54dac 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -26,6 +26,7 @@ #include <linux/seqlock.h> #include <linux/mutex.h> #include <linux/timer.h> +#include <linux/version.h> #include <linux/wait.h> #include <linux/blockgroup_lock.h> #include <linux/percpu_counter.h> @@ -713,38 +714,54 @@ struct move_extent { sizeof((ext4_inode)->field)) \ <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ + /* - * We use the bottom 34 bits of the signed 64-bit time value, with - * the top two of these bits in the bottom of extra. This leads - * to a slightly odd encoding, which works like this: + * We need is an encoding that preserves the times for extra epoch "00": * - * extra msb of - * epoch 32-bit - * bits time decoded 64-bit tv_sec valid time range - * 0 0 0 0x000000000..0x07fffffff 1970-01-01..2038-01-19 - * 0 0 1 0x080000000..0x0ffffffff 2038-01-19..2106-02-07 - * 0 1 0 0x100000000..0x17fffffff 2106-02-07..2174-02-25 - * 0 1 1 0x180000000..0x1ffffffff 2174-02-25..2242-03-16 - * 1 0 0 0x200000000..0x27fffffff 2242-03-16..2310-04-04 - * 1 0 1 0x280000000..0x2ffffffff 2310-04-04..2378-04-22 - * 1 1 0 0x300000000..0x37fffffff 2378-04-22..2446-05-10 - - * 1 1 1 -0x80000000..-0x00000001 1901-12-13..1969-12-31 + * extra msb of adjust for signed + * epoch 32-bit 32-bit tv_sec to + * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range + * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 + * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 + * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 + * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 + * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 + * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 + * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 + * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 + * + * Note that previous versions of the kernel on 64-bit systems would + * incorrectly use extra epoch bits 1,1 for dates between 1901 and + * 1970. e2fsck will correct this, assuming that it is run on the + * affected filesystem before 2242. */ static inline __le32 ext4_encode_extra_time(struct timespec *time) { - return cpu_to_le32((sizeof(time->tv_sec) > 4 ? - (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) | - ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK)); + u32 extra = sizeof(time->tv_sec) > 4 ? + ((time->tv_sec - (s32)time->tv_sec) >> 32) & EXT4_EPOCH_MASK : 0; + return cpu_to_le32(extra | (time->tv_nsec << EXT4_EPOCH_BITS)); } static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) { - if (sizeof(time->tv_sec) > 4) - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) - << 32; - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; + if (unlikely(sizeof(time->tv_sec) > 4 && + (extra & cpu_to_le32(EXT4_EPOCH_MASK)))) { +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,20,0) + /* Handle legacy encoding of pre-1970 dates with epoch + * bits 1,1. We assume that by kernel version 4.20, + * everyone will have run fsck over the affected + * filesystems to correct the problem. + */ + u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; + if (extra_bits == 3) + extra_bits = 0; + time->tv_sec += extra_bits << 32; +#else + time->tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; +#endif + } + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; } #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v5 2/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-13 7:56 ` Andreas Dilger 2013-11-14 8:38 ` [PATCH v5 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner @ 2013-11-14 8:44 ` David Turner 2013-11-14 10:15 ` Mark Harris 1 sibling, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-14 8:44 UTC (permalink / raw) To: Andreas Dilger Cc: Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List (apologies if this is a dup -- my mail client crashed and I don't see this one in the lkml archives) -- 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). Signed-off-by: David Turner <novalis@novalis.org> --- e2fsck/pass1.c | 38 ++++++++++++++++++++++++++++++++++++++ e2fsck/problem.c | 4 ++++ e2fsck/problem.h | 4 ++++ 3 files changed, 46 insertions(+) diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c index ab23e42..b5ae885 100644 --- a/e2fsck/pass1.c +++ b/e2fsck/pass1.c @@ -348,6 +348,24 @@ fix: EXT2_INODE_SIZE(sb), "pass1"); } +#define EXT4_EPOCH_BITS 2 +#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1) + +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, and mtimes greater than 2242 are actually pre-1970 dates + * mis-encoded. + */ +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 5 * (1ULL << 32) + static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx) { struct ext2_super_block *sb = ctx->fs->super; @@ -388,6 +406,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 && + CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, atime) || + CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, ctime) || + CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, mtime)) { + + 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..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 */ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v5 2/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-14 8:44 ` [PATCH v5 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner @ 2013-11-14 10:15 ` Mark Harris 2013-11-14 21:06 ` [PATCH v6] " David Turner 0 siblings, 1 reply; 39+ messages in thread From: Mark Harris @ 2013-11-14 10:15 UTC (permalink / raw) To: David Turner Cc: Andreas Dilger, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List > +/* When today's date is earlier than 2242, we assume that atimes, > + * ctimes, and mtimes greater than 2242 are actually pre-1970 dates ...mtimes with years in the range 2310..2378 are... > + * mis-encoded. > + */ > +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 5 * (1ULL << 32) Wouldn't 2242 be 0x200000000ULL, i.e. 2 * (1ULL << 32)? > + > + /* > + * 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 && Needs parentheses ( > + CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, atime) || > + CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, ctime) || > + CHECK_INODE_EXTRA_NEGATIVE_EPOCH(inode, mtime)) { ) > + > + 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; With e.g. 2039 atime and 1968 mtime, shouldn't only i_mtime_extra be altered? > + e2fsck_write_inode_full(ctx, pctx->ino, pctx->inode, > + EXT2_INODE_SIZE(sb), "pass1"); > + } - Mark ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v6] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-14 10:15 ` Mark Harris @ 2013-11-14 21:06 ` David Turner 2013-11-29 21:54 ` David Turner 0 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-14 21:06 UTC (permalink / raw) To: Mark Harris Cc: Andreas Dilger, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List Not sure what the official subject line format is for revising only one of N patches, so I'm trying this one. Let me now if it is wrong. On Thu, 2013-11-14 at 02:15 -0800, Mark Harris wrote: > > + * mis-encoded. > > + */ > > +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 5 * (1ULL << 32) > > Wouldn't 2242 be 0x200000000ULL, i.e. 2 * (1ULL << 32)? Actually, it would be 2 * (1LL << 32), because we later use it in a comparison with a signed value. -- 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). Signed-off-by: David Turner <novalis@novalis.org> --- e2fsck/pass1.c | 41 +++++++++++++++++++++++++++++++++++++++++ e2fsck/problem.c | 4 ++++ e2fsck/problem.h | 4 ++++ 3 files changed, 49 insertions(+) diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c index ab23e42..e19855f 100644 --- a/e2fsck/pass1.c +++ b/e2fsck/pass1.c @@ -348,6 +348,24 @@ fix: EXT2_INODE_SIZE(sb), "pass1"); } +#define EXT4_EPOCH_BITS 2 +#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1) + +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, 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 +406,29 @@ 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, 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, 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, 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 */ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v6] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-14 21:06 ` [PATCH v6] " David Turner @ 2013-11-29 21:54 ` David Turner 2013-11-29 22:11 ` Andreas Dilger 0 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2013-11-29 21:54 UTC (permalink / raw) To: Mark Harris Cc: Andreas Dilger, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List Is this version good, or should I make some more improvements? On Thu, 2013-11-14 at 16:06 -0500, David Turner wrote: > Not sure what the official subject line format is for revising only > one of N patches, so I'm trying this one. Let me now if it is wrong. > > On Thu, 2013-11-14 at 02:15 -0800, Mark Harris wrote: > > > + * mis-encoded. > > > + */ > > > +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 5 * (1ULL << 32) > > > > Wouldn't 2242 be 0x200000000ULL, i.e. 2 * (1ULL << 32)? > > Actually, it would be 2 * (1LL << 32), because we later use it in a > comparison with a signed value. > > -- > 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). > > Signed-off-by: David Turner <novalis@novalis.org> > --- > e2fsck/pass1.c | 41 +++++++++++++++++++++++++++++++++++++++++ > e2fsck/problem.c | 4 ++++ > e2fsck/problem.h | 4 ++++ > 3 files changed, 49 insertions(+) > > diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c > index ab23e42..e19855f 100644 > --- a/e2fsck/pass1.c > +++ b/e2fsck/pass1.c > @@ -348,6 +348,24 @@ fix: > EXT2_INODE_SIZE(sb), "pass1"); > } > > +#define EXT4_EPOCH_BITS 2 > +#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1) > + > +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, 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 +406,29 @@ 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, 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, 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, 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 > */ ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v6] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-29 21:54 ` David Turner @ 2013-11-29 22:11 ` Andreas Dilger 2013-12-07 20:02 ` [PATCH v7 1/2] " David Turner 2013-12-07 20:02 ` [PATCH v7 2/2] debugfs: Decode {a,c,cr,m}time_extra fields in stat David Turner 0 siblings, 2 replies; 39+ messages in thread From: Andreas Dilger @ 2013-11-29 22:11 UTC (permalink / raw) To: David Turner Cc: Mark Harris, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 5644 bytes --] On Nov 29, 2013, at 2:54 PM, David Turner <novalis@novalis.org> wrote: > Is this version good, or should I make some more improvements? The patch looks good to me (you could add a Reviewed-by: line for me if you want. What you need to do now is to add a new test case or two to verify that this is working correctly. One way is to format a small binary image that has timestamps of various "time eras". Another way is to have the test script format a new image each time and then use "debugfs -w -f" and the "sif" ("set_inode_field") command to set the timestamps each time. The only potential problem with "sif" is that debugfs might internally "fix" the timestamps in the future and invalidate the tests, but probably not. The benefit of using debugfs+sif is that it is more clear what the test is actually doing, and is easier to enhance in the future. In both cases, we want to verify that e.g. e2fsck fixes old timestamps and that "debugfs -R 'stat testfile'" decodes the times correctly (atime, mtime, ctime, and crtime). You'd need to set TZ in the test script so that the time zone in which the test is run does not affect the results. There is also the E2FSCK_TIME environment variable that could be used to check e2fsck running beyond 2242. Cheers, Andreas > On Thu, 2013-11-14 at 16:06 -0500, David Turner wrote: >> Not sure what the official subject line format is for revising only >> one of N patches, so I'm trying this one. Let me now if it is wrong. >> >> On Thu, 2013-11-14 at 02:15 -0800, Mark Harris wrote: >>>> + * mis-encoded. >>>> + */ >>>> +#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 5 * (1ULL << 32) >>> >>> Wouldn't 2242 be 0x200000000ULL, i.e. 2 * (1ULL << 32)? >> >> Actually, it would be 2 * (1LL << 32), because we later use it in a >> comparison with a signed value. >> >> -- >> 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). >> >> Signed-off-by: David Turner <novalis@novalis.org> >> --- >> e2fsck/pass1.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> e2fsck/problem.c | 4 ++++ >> e2fsck/problem.h | 4 ++++ >> 3 files changed, 49 insertions(+) >> >> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c >> index ab23e42..e19855f 100644 >> --- a/e2fsck/pass1.c >> +++ b/e2fsck/pass1.c >> @@ -348,6 +348,24 @@ fix: >> EXT2_INODE_SIZE(sb), "pass1"); >> } >> >> +#define EXT4_EPOCH_BITS 2 >> +#define EXT4_EPOCH_MASK ((1 << EXT4_EPOCH_BITS) - 1) >> + >> +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, 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 +406,29 @@ 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, 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, 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, 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 >> */ > > Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP using GPGMail --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v7 1/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-11-29 22:11 ` Andreas Dilger @ 2013-12-07 20:02 ` David Turner 2013-12-07 22:33 ` Andreas Dilger 2013-12-08 0:53 ` Theodore Ts'o 2013-12-07 20:02 ` [PATCH v7 2/2] debugfs: Decode {a,c,cr,m}time_extra fields in stat David Turner 1 sibling, 2 replies; 39+ messages in thread From: David Turner @ 2013-12-07 20:02 UTC (permalink / raw) To: Andreas Dilger Cc: Mark Harris, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v7 1/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-12-07 20:02 ` [PATCH v7 1/2] " David Turner @ 2013-12-07 22:33 ` Andreas Dilger 2013-12-08 0:53 ` Theodore Ts'o 1 sibling, 0 replies; 39+ messages in thread From: Andreas Dilger @ 2013-12-07 22:33 UTC (permalink / raw) To: David Turner Cc: Mark Harris, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List 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 > > > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v7 1/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-12-07 20:02 ` [PATCH v7 1/2] " David Turner 2013-12-07 22:33 ` Andreas Dilger @ 2013-12-08 0:53 ` Theodore Ts'o 2013-12-08 2:58 ` David Turner 1 sibling, 1 reply; 39+ messages in thread From: Theodore Ts'o @ 2013-12-08 0:53 UTC (permalink / raw) To: David Turner Cc: Andreas Dilger, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v7 1/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-12-08 0:53 ` Theodore Ts'o @ 2013-12-08 2:58 ` David Turner 2013-12-08 3:21 ` Theodore Ts'o 0 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2013-12-08 2:58 UTC (permalink / raw) To: Theodore Ts'o Cc: Andreas Dilger, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List 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? ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v7 1/2] e2fsck: Correct ext4 dates generated by old kernels. 2013-12-08 2:58 ` David Turner @ 2013-12-08 3:21 ` Theodore Ts'o 0 siblings, 0 replies; 39+ messages in thread From: Theodore Ts'o @ 2013-12-08 3:21 UTC (permalink / raw) To: David Turner Cc: Andreas Dilger, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v7 2/2] debugfs: Decode {a,c,cr,m}time_extra fields in stat 2013-11-29 22:11 ` Andreas Dilger 2013-12-07 20:02 ` [PATCH v7 1/2] " David Turner @ 2013-12-07 20:02 ` David Turner 1 sibling, 0 replies; 39+ messages in thread From: David Turner @ 2013-12-07 20:02 UTC (permalink / raw) To: Andreas Dilger Cc: Mark Harris, Theodore Ts'o, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List Decodes post-2038 dates correctly on on machines with a 64-bit time_t. When decoding dates from xtime+xtime_extra fields, we assume that these dates are in the correct format (i.e. pre-1970 dates have xtime_extra low bits == 0 instead of 3). So uncorrected pre-1970 dates will be displayed as post-2310 dates. This is because we don't want our filesystem debugging tools to silently correct data errors. Signed-off-by: David Turner <novalis@novalis.org> --- debugfs/debugfs.c | 22 ++++++++++++++++++---- debugfs/debugfs.h | 2 +- debugfs/util.c | 7 +++---- 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c index 8c32eff..aa2f972 100644 --- a/debugfs/debugfs.c +++ b/debugfs/debugfs.c @@ -37,6 +37,8 @@ extern char *optarg; #include "../version.h" #include "jfs_user.h" +#include "extra_epoch.h" + #ifndef BUFSIZ #define BUFSIZ 8192 #endif @@ -718,6 +720,14 @@ static void dump_extents(FILE *f, const char *prefix, ext2_ino_t ino, fprintf(f, "\n"); } +char* inode_time_to_string(__s32 xtime, __u32 xtime_extra) { + time_t time = (time_t) xtime; + if (sizeof(time_t) > 4) { + time += (xtime_extra & EXT4_EPOCH_MASK) << 32; + } + return time_to_string(time); +} + void internal_dump_inode(FILE *out, const char *prefix, ext2_ino_t inode_num, struct ext2_inode *inode, int do_dump_blocks) @@ -791,16 +801,20 @@ void internal_dump_inode(FILE *out, const char *prefix, if (is_large_inode && large_inode->i_extra_isize >= 24) { fprintf(out, "%s ctime: 0x%08x:%08x -- %s", prefix, inode->i_ctime, large_inode->i_ctime_extra, - time_to_string(inode->i_ctime)); + inode_time_to_string(inode->i_ctime, + large_inode->i_ctime_extra)); fprintf(out, "%s atime: 0x%08x:%08x -- %s", prefix, inode->i_atime, large_inode->i_atime_extra, - time_to_string(inode->i_atime)); + inode_time_to_string(inode->i_atime, + large_inode->i_atime_extra)); fprintf(out, "%s mtime: 0x%08x:%08x -- %s", prefix, inode->i_mtime, large_inode->i_mtime_extra, - time_to_string(inode->i_mtime)); + inode_time_to_string(inode->i_mtime, + large_inode->i_mtime_extra)); fprintf(out, "%scrtime: 0x%08x:%08x -- %s", prefix, large_inode->i_crtime, large_inode->i_crtime_extra, - time_to_string(large_inode->i_crtime)); + inode_time_to_string(large_inode->i_crtime, + large_inode->i_crtime_extra)); } else { fprintf(out, "%sctime: 0x%08x -- %s", prefix, inode->i_ctime, time_to_string(inode->i_ctime)); diff --git a/debugfs/debugfs.h b/debugfs/debugfs.h index 45175cf..a02f29c 100644 --- a/debugfs/debugfs.h +++ b/debugfs/debugfs.h @@ -33,7 +33,7 @@ extern int check_fs_not_open(char *name); extern int check_fs_read_write(char *name); extern int check_fs_bitmaps(char *name); extern ext2_ino_t string_to_inode(char *str); -extern char *time_to_string(__u32); +extern char *time_to_string(time_t); extern time_t string_to_time(const char *); extern unsigned long parse_ulong(const char *str, const char *cmd, const char *descr, int *err); diff --git a/debugfs/util.c b/debugfs/util.c index cf3a6c6..a60aec7 100644 --- a/debugfs/util.c +++ b/debugfs/util.c @@ -187,13 +187,12 @@ int check_fs_bitmaps(char *name) } /* - * This function takes a __u32 time value and converts it to a string, - * using ctime + * This function takes a time_t time value and converts it to a string, + * using asctime */ -char *time_to_string(__u32 cl) +char *time_to_string(time_t t) { static int do_gmt = -1; - time_t t = (time_t) cl; const char *tz; if (do_gmt == -1) { -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-12 0:30 ` Theodore Ts'o 2013-11-12 21:35 ` Andreas Dilger @ 2013-11-12 23:03 ` Darrick J. Wong 2013-11-13 2:36 ` David Turner 2014-01-22 6:22 ` Darrick J. Wong 2 siblings, 1 reply; 39+ messages in thread From: Darrick J. Wong @ 2013-11-12 23:03 UTC (permalink / raw) To: Theodore Ts'o, David Turner, Mark Harris, Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > written on 64-bit systems. > > > > I don't care about currently-existing post-2038 files, because I believe > > that nobody has a valid reason to have such files. However, I do > > believe that pre-1970 files are probably important to someone. > > > > Despite this, I prefer option (b), because I think the simplicity is > > valuable, and because I hate to give up date ranges (even ones that I > > think we'll "never" need). Option (b) is not actually lossy, because we > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > their dates would be in the far future (and thus cannot be legitimate). > > > > Would a patch that does (b) be accepted? I would accompany it with a > > patch to e2fsck (which I assume would also go to the ext4 developers > > mailing list?). > > I agree, I think this is the best way to go. I'm going to drop your > earlier patch, and wait for an updated patch from you. It may miss > this merge window, but as Andreas has pointed out, we still have a few > years to get this right. :-) Just to be clear... we're going with Andreas' fix, wherein time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; becomes: time->tv_sec += (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; "or" becomes "plus"? So I can update fuse2fs. Also, can someone proofread [1] and make sure it's correct? --D [1] https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout#Inode_Timestamps > > Thanks!! > > - 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-12 23:03 ` [PATCH] ext4: explain encoding of 34-bit a,c,mtime values Darrick J. Wong @ 2013-11-13 2:36 ` David Turner 0 siblings, 0 replies; 39+ messages in thread From: David Turner @ 2013-11-13 2:36 UTC (permalink / raw) To: Darrick J. Wong Cc: Theodore Ts'o, Mark Harris, Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List On Tue, 2013-11-12 at 15:03 -0800, Darrick J. Wong wrote: > On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > > written on 64-bit systems. > > > > > > I don't care about currently-existing post-2038 files, because I believe > > > that nobody has a valid reason to have such files. However, I do > > > believe that pre-1970 files are probably important to someone. > > > > > > Despite this, I prefer option (b), because I think the simplicity is > > > valuable, and because I hate to give up date ranges (even ones that I > > > think we'll "never" need). Option (b) is not actually lossy, because we > > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > > their dates would be in the far future (and thus cannot be legitimate). > > > > > > Would a patch that does (b) be accepted? I would accompany it with a > > > patch to e2fsck (which I assume would also go to the ext4 developers > > > mailing list?). > > > > I agree, I think this is the best way to go. I'm going to drop your > > earlier patch, and wait for an updated patch from you. It may miss > > this merge window, but as Andreas has pointed out, we still have a few > > years to get this right. :-) > > Just to be clear... we're going with Andreas' fix, wherein > > time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; > > becomes: > > time->tv_sec += (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; > > "or" becomes "plus"? So I can update fuse2fs. Yes, but with a kernel-version-dependent change, which is something like this: #if LINUX_VERSION_CODE >= KERNEL_VERSION(4,20,0) time->tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; #else u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; if (extra_bits == 3) extra_bits = 0; time->tv_sec += extra_bits << 32; #endif > Also, can someone proofread [1] and make sure it's correct? It's not quite right. I've requested an account so that I can correct it. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2013-11-12 0:30 ` Theodore Ts'o 2013-11-12 21:35 ` Andreas Dilger 2013-11-12 23:03 ` [PATCH] ext4: explain encoding of 34-bit a,c,mtime values Darrick J. Wong @ 2014-01-22 6:22 ` Darrick J. Wong 2014-02-11 5:12 ` David Turner 2 siblings, 1 reply; 39+ messages in thread From: Darrick J. Wong @ 2014-01-22 6:22 UTC (permalink / raw) To: Theodore Ts'o, David Turner, Mark Harris, Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > written on 64-bit systems. > > > > I don't care about currently-existing post-2038 files, because I believe > > that nobody has a valid reason to have such files. However, I do > > believe that pre-1970 files are probably important to someone. > > > > Despite this, I prefer option (b), because I think the simplicity is > > valuable, and because I hate to give up date ranges (even ones that I > > think we'll "never" need). Option (b) is not actually lossy, because we > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > their dates would be in the far future (and thus cannot be legitimate). > > > > Would a patch that does (b) be accepted? I would accompany it with a > > patch to e2fsck (which I assume would also go to the ext4 developers > > mailing list?). > > I agree, I think this is the best way to go. I'm going to drop your > earlier patch, and wait for an updated patch from you. It may miss > this merge window, but as Andreas has pointed out, we still have a few > years to get this right. :-) Just out of curiosity, did this (updated patch) ever happen? --D > > Thanks!! > > - 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2014-01-22 6:22 ` Darrick J. Wong @ 2014-02-11 5:12 ` David Turner 2014-02-11 7:07 ` Andreas Dilger 0 siblings, 1 reply; 39+ messages in thread From: David Turner @ 2014-02-11 5:12 UTC (permalink / raw) To: Darrick J. Wong Cc: Theodore Ts'o, Mark Harris, Andreas Dilger, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List On Tue, 2014-01-21 at 22:22 -0800, Darrick J. Wong wrote: > On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: > > On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: > > > b. Use Andreas's encoding, which is incompatible with pre-1970 files > > > written on 64-bit systems. > > > > > > I don't care about currently-existing post-2038 files, because I believe > > > that nobody has a valid reason to have such files. However, I do > > > believe that pre-1970 files are probably important to someone. > > > > > > Despite this, I prefer option (b), because I think the simplicity is > > > valuable, and because I hate to give up date ranges (even ones that I > > > think we'll "never" need). Option (b) is not actually lossy, because we > > > could correct pre-1970 files with e2fsck; under Andreas's encoding, > > > their dates would be in the far future (and thus cannot be legitimate). > > > > > > Would a patch that does (b) be accepted? I would accompany it with a > > > patch to e2fsck (which I assume would also go to the ext4 developers > > > mailing list?). > > > > I agree, I think this is the best way to go. I'm going to drop your > > earlier patch, and wait for an updated patch from you. It may miss > > this merge window, but as Andreas has pointed out, we still have a few > > years to get this right. :-) > > Just out of curiosity, did this (updated patch) ever happen? I think I sent a usable patch that Ted merged part of into e2fscktools; the kernel portion was dropped for some reason. While I was waiting to hear back on the kernel portion, I started looking into the dtime stuff, but then I got distracted by a new job. Assuming that I won't have time to deal with dtime (since it seems to be much more complicated), is the right way forward for me to rebase the non-dtime portion of my patch against the latest kernel, and resend it? If so, will it get merged? (Assume here that I do the same with the e2fsck stuff) ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] ext4: explain encoding of 34-bit a,c,mtime values 2014-02-11 5:12 ` David Turner @ 2014-02-11 7:07 ` Andreas Dilger 2014-02-14 3:47 ` [PATCH v8 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner 2014-02-14 3:47 ` [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 0 siblings, 2 replies; 39+ messages in thread From: Andreas Dilger @ 2014-02-11 7:07 UTC (permalink / raw) To: David Turner Cc: Darrick J. Wong, Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List [-- Attachment #1: Type: text/plain, Size: 2712 bytes --] On Feb 10, 2014, at 10:12 PM, David Turner <novalis@novalis.org> wrote: > On Tue, 2014-01-21 at 22:22 -0800, Darrick J. Wong wrote: >> On Mon, Nov 11, 2013 at 07:30:18PM -0500, Theodore Ts'o wrote: >>> On Sun, Nov 10, 2013 at 02:56:54AM -0500, David Turner wrote: >>>> b. Use Andreas's encoding, which is incompatible with pre-1970 files >>>> written on 64-bit systems. >>>> >>>> I don't care about currently-existing post-2038 files, because I believe >>>> that nobody has a valid reason to have such files. However, I do >>>> believe that pre-1970 files are probably important to someone. >>>> >>>> Despite this, I prefer option (b), because I think the simplicity is >>>> valuable, and because I hate to give up date ranges (even ones that I >>>> think we'll "never" need). Option (b) is not actually lossy, because we could correct pre-1970 files with e2fsck; under Andreas's encoding, >>>> their dates would be in the far future (and thus cannot be legitimate). >>>> >>>> Would a patch that does (b) be accepted? I would accompany it with a >>>> patch to e2fsck (which I assume would also go to the ext4 developers >>>> mailing list?). >>> >>> I agree, I think this is the best way to go. I'm going to drop your >>> earlier patch, and wait for an updated patch from you. It may miss >>> this merge window, but as Andreas has pointed out, we still have a few >>> years to get this right. :-) >> >> Just out of curiosity, did this (updated patch) ever happen? > > I think I sent a usable patch that Ted merged part of into e2fscktools; > the kernel portion was dropped for some reason. > > While I was waiting to hear back on the kernel portion, I started > looking into the dtime stuff, but then I got distracted by a new job. > > Assuming that I won't have time to deal with dtime (since it seems to be > much more complicated), is the right way forward for me to rebase the > non-dtime portion of my patch against the latest kernel, and resend it? > If so, will it get merged? (Assume here that I do the same with the > e2fsck stuff) Yes, please submit an updated patch for the kernel. Ted will hopefully merge it this time. I don't know that we really care about dtime so much, since it is mostly just treated as "zero == not deleted" and "non-zero == deleted" by e2fsck, and maybe useful for manual debugging. We might consider that it uses a sliding window in the future, since I'm sure we won't care about files deleted more than 70 years ago, and if we did the fact that they appear to be files deleted 70 years in the future won't matter so much. In any case, that can be looked at in a separate patch. Cheers, Andreas [-- Attachment #2: Message signed with OpenPGP using GPGMail --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v8 1/2] ext4: Fix handling of extended tv_sec (bug 23732) 2014-02-11 7:07 ` Andreas Dilger @ 2014-02-14 3:47 ` David Turner 2014-02-14 3:47 ` [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 1 sibling, 0 replies; 39+ messages in thread From: David Turner @ 2014-02-14 3:47 UTC (permalink / raw) To: Andreas Dilger Cc: Darrick J. Wong, Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List against tytso/dev -- In ext4, the bottom two bits of {a,c,m}time_extra are used to extend the {a,c,m}time fields, deferring the year 2038 problem to the year 2446. When decoding these extended fields, for times whose bottom 32 bits would represent a negative number, sign extension causes the 64-bit extended timestamp to be negative as well, which is not what's intended. This patch corrects that issue, so that the only negative {a,c,m}times are those between 1901 and 1970 (as per 32-bit signed timestamps). Some older kernels might have written pre-1970 dates with 1,1 in the extra bits. This patch treats those incorrectly-encoded dates as pre-1970, instead of post-2311, until kernel 4.20 is released. Hopefully by then e2fsck will have fixed up the bad data. Also add a comment explaining the encoding of ext4's extra {a,c,m}time bits. Signed-off-by: David Turner <novalis@novalis.org> Reported-by: Mark Harris <mh8928@yahoo.com> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=23732 --- fs/ext4/ext4.h | 49 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h index ece5556..f9c6d2f 100644 --- a/fs/ext4/ext4.h +++ b/fs/ext4/ext4.h @@ -26,6 +26,7 @@ #include <linux/seqlock.h> #include <linux/mutex.h> #include <linux/timer.h> +#include <linux/version.h> #include <linux/wait.h> #include <linux/blockgroup_lock.h> #include <linux/percpu_counter.h> @@ -724,19 +725,53 @@ struct move_extent { <= (EXT4_GOOD_OLD_INODE_SIZE + \ (einode)->i_extra_isize)) \ +/* + * We need is an encoding that preserves the times for extra epoch "00": + * + * extra msb of adjust for signed + * epoch 32-bit 32-bit tv_sec to + * bits time decoded 64-bit tv_sec 64-bit tv_sec valid time range + * 0 0 1 -0x80000000..-0x00000001 0x000000000 1901-12-13..1969-12-31 + * 0 0 0 0x000000000..0x07fffffff 0x000000000 1970-01-01..2038-01-19 + * 0 1 1 0x080000000..0x0ffffffff 0x100000000 2038-01-19..2106-02-07 + * 0 1 0 0x100000000..0x17fffffff 0x100000000 2106-02-07..2174-02-25 + * 1 0 1 0x180000000..0x1ffffffff 0x200000000 2174-02-25..2242-03-16 + * 1 0 0 0x200000000..0x27fffffff 0x200000000 2242-03-16..2310-04-04 + * 1 1 1 0x280000000..0x2ffffffff 0x300000000 2310-04-04..2378-04-22 + * 1 1 0 0x300000000..0x37fffffff 0x300000000 2378-04-22..2446-05-10 + * + * Note that previous versions of the kernel on 64-bit systems would + * incorrectly use extra epoch bits 1,1 for dates between 1901 and + * 1970. e2fsck will correct this, assuming that it is run on the + * affected filesystem before 2242. + */ + static inline __le32 ext4_encode_extra_time(struct timespec *time) { - return cpu_to_le32((sizeof(time->tv_sec) > 4 ? - (time->tv_sec >> 32) & EXT4_EPOCH_MASK : 0) | - ((time->tv_nsec << EXT4_EPOCH_BITS) & EXT4_NSEC_MASK)); + u32 extra = sizeof(time->tv_sec) > 4 ? + ((time->tv_sec - (s32)time->tv_sec) >> 32) & EXT4_EPOCH_MASK : 0; + return cpu_to_le32(extra | (time->tv_nsec << EXT4_EPOCH_BITS)); } static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra) { - if (sizeof(time->tv_sec) > 4) - time->tv_sec |= (__u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) - << 32; - time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; + if (unlikely(sizeof(time->tv_sec) > 4 && + (extra & cpu_to_le32(EXT4_EPOCH_MASK)))) { +#if LINUX_VERSION_CODE < KERNEL_VERSION(4,20,0) + /* Handle legacy encoding of pre-1970 dates with epoch + * bits 1,1. We assume that by kernel version 4.20, + * everyone will have run fsck over the affected + * filesystems to correct the problem. + */ + u64 extra_bits = le32_to_cpu(extra) & EXT4_EPOCH_MASK; + if (extra_bits == 3) + extra_bits = 0; + time->tv_sec += extra_bits << 32; +#else + time->tv_sec += (u64)(le32_to_cpu(extra) & EXT4_EPOCH_MASK) << 32; +#endif + } + time->tv_nsec = (le32_to_cpu(extra) & EXT4_NSEC_MASK) >> EXT4_EPOCH_BITS; } #define EXT4_INODE_SET_XTIME(xtime, inode, raw_inode) \ -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels 2014-02-11 7:07 ` Andreas Dilger 2014-02-14 3:47 ` [PATCH v8 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner @ 2014-02-14 3:47 ` David Turner 2014-02-14 5:40 ` Andreas Dilger 1 sibling, 1 reply; 39+ messages in thread From: David Turner @ 2014-02-14 3:47 UTC (permalink / raw) To: Andreas Dilger Cc: Darrick J. Wong, Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List, Linux Kernel Mailing List against e2fsprogs/next -- 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels 2014-02-14 3:47 ` [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner @ 2014-02-14 5:40 ` Andreas Dilger 2014-02-14 22:11 ` Darrick J. Wong 0 siblings, 1 reply; 39+ messages in thread From: Andreas Dilger @ 2014-02-14 5:40 UTC (permalink / raw) To: David Turner Cc: Darrick J. Wong, Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List Thanks for the patch, and especially the good test case. You (or Ted) can add: Reviewed-by: Andreas Dilger <adilger@dilger.ca> for both of these patches. Cheers, Andreas > On Feb 13, 2014, at 20:47, David Turner <novalis@novalis.org> wrote: > > against e2fsprogs/next > -- > 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 > > > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels 2014-02-14 5:40 ` Andreas Dilger @ 2014-02-14 22:11 ` Darrick J. Wong 0 siblings, 0 replies; 39+ messages in thread From: Darrick J. Wong @ 2014-02-14 22:11 UTC (permalink / raw) To: Andreas Dilger Cc: David Turner, Theodore Ts'o, Mark Harris, Jan Kara, Ext4 Developers List Looks good to me too, so you can also add Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D On Thu, Feb 13, 2014 at 10:40:06PM -0700, Andreas Dilger wrote: > Thanks for the patch, and especially the good test case. You (or Ted) can add: > > Reviewed-by: Andreas Dilger <adilger@dilger.ca> > > for both of these patches. > > Cheers, Andreas > > > On Feb 13, 2014, at 20:47, David Turner <novalis@novalis.org> wrote: > > > > against e2fsprogs/next > > -- > > 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2014-02-14 22:11 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-11-07 7:16 [PATCH] ext4: Fix reading of extended tv_sec (bug 23732) David Turner 2013-11-07 16:03 ` Jan Kara 2013-11-07 22:54 ` [PATCH v2] " David Turner 2013-11-07 23:14 ` Jan Kara 2013-11-07 23:26 ` [PATCH v3] " David Turner 2013-11-08 5:17 ` Theodore Ts'o 2013-11-08 21:37 ` Andreas Dilger 2013-11-09 7:19 ` [PATCH] ext4: explain encoding of 34-bit a,c,mtime values David Turner 2013-11-09 7:19 ` David Turner 2013-11-09 23:51 ` Mark Harris 2013-11-09 23:51 ` Mark Harris 2013-11-10 7:56 ` David Turner 2013-11-12 0:30 ` Theodore Ts'o 2013-11-12 21:35 ` Andreas Dilger 2013-11-13 7:00 ` [PATCH v4 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner 2013-11-13 8:19 ` Darrick J. Wong 2013-11-13 7:00 ` [PATCH v4 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 2013-11-13 7:56 ` Andreas Dilger 2013-11-14 8:38 ` [PATCH v5 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner 2013-11-14 8:44 ` [PATCH v5 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 2013-11-14 10:15 ` Mark Harris 2013-11-14 21:06 ` [PATCH v6] " David Turner 2013-11-29 21:54 ` David Turner 2013-11-29 22:11 ` Andreas Dilger 2013-12-07 20:02 ` [PATCH v7 1/2] " David Turner 2013-12-07 22:33 ` Andreas Dilger 2013-12-08 0:53 ` Theodore Ts'o 2013-12-08 2:58 ` David Turner 2013-12-08 3:21 ` Theodore Ts'o 2013-12-07 20:02 ` [PATCH v7 2/2] debugfs: Decode {a,c,cr,m}time_extra fields in stat David Turner 2013-11-12 23:03 ` [PATCH] ext4: explain encoding of 34-bit a,c,mtime values Darrick J. Wong 2013-11-13 2:36 ` David Turner 2014-01-22 6:22 ` Darrick J. Wong 2014-02-11 5:12 ` David Turner 2014-02-11 7:07 ` Andreas Dilger 2014-02-14 3:47 ` [PATCH v8 1/2] ext4: Fix handling of extended tv_sec (bug 23732) David Turner 2014-02-14 3:47 ` [PATCH v8 2/2] e2fsck: Correct ext4 dates generated by old kernels David Turner 2014-02-14 5:40 ` Andreas Dilger 2014-02-14 22:11 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.