Date:	Fri, 24 Jun 2011 16:46:44 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Mark Harris <mhlk@....us>
Cc:	Akira Fujita <a-fujita@...jp.nec.com>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: [BUG] ext4 timestamps corruption

On 2011-06-23, at 4:37 PM, Mark Harris wrote:
> 2011/6/23 Akira Fujita <a-fujita@...jp.nec.com>:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=23732
>> 
>> ext4: Fix ext4 timestamps corruption
>> 
>> Officially, ext4 can handle its timestamps until 2514
>> with 32bit entries plus EPOCH_BIT (2bits).
>> But when timestamps values use 32+ bit
>> (e.g. 2038-01-19 9:14:08 0x0000000080000000),
>> we can get corrupted values.
>> Because sign bit is overwritten by transferring value
>> between kernel space and user space.
>> 
>> To fix this issue, 32th bit of extra time fields in ext4_inode structure
>> (e.g. i_ctime_extra) are used as the sign for 64bit user space.
>> Because these are used only 20bits for nano-second and bottom of 2bits
>> are for EXT4_EPOCH_BITS shift.
>> With this patch, ext4 supports timestamps Y1901-2514.
> 
> Thanks for looking into this bug.  However tv_nsec is in the
> range 0..999999999 and requires 30 bits.  That is why tv_sec was
> only extended by 2 bits.  So there are no additional spare bits
> in the "extra" field.
> 
> 34-bit seconds can accommodate a maximum of 544.4 years, e.g.
> 1970..2514 or 1901..2446.  Although an early version of the patch
> for 34-bit tv_sec in ext4 worked with years 1970..2514, prior to
> being committed the patch was changed to support pre-1970
> timestamps (introducing the sign extension issue in the decoding):
>  http://marc.info/?l=linux-ext4&m=118208541320999

Sigh, it seems so unlikely that anyone even has a valid timestamp
on a file with a date before 1970, it makes me wonder if this extra
effort is even worthwhile...

> The existing encoding simply encodes bits 31..0 of tv_sec in the
> regular time field and bits 33..32 in the extra field (along with
> the 30-bit tv_nsec).  The issue is in the decoding, which I think
> can be addressed by changing only the body of the "if" in in the
> ext4_decode_extra_time function, to something like this:
> 
>        time->tv_sec += ((__u32)time->tv_sec +
>                ((__u64)le32_to_cpu(extra) << 32) +
>                0x80000000LL) & 0x300000000LL;
> 
> This is untested, and might look nicer with some macros, but
> it should decode the 34 bits into a timestamp in the range
> -0x80000000 (1901-12-13) to 0x37fffffff (2446-05-10), while
> retaining compatibility with the existing encoding.
> 
>   2    msb of                         adjustment needed to convert
> extra  32-bit                         sign-extended 32-bit tv_sec
>  bits   time   decoded 64-bit tv_sec   to decoded 64-bit tv_sec
>  1 1     1    -0x80000000..-1           0
>  0 0     0    0x000000000..0x07fffffff  0
>  0 0     1    0x080000000..0x0ffffffff  0x100000000
>  0 1     0    0x100000000..0x17fffffff  0x100000000
>  0 1     1    0x180000000..0x1ffffffff  0x200000000
>  1 0     0    0x200000000..0x27fffffff  0x200000000
>  1 0     1    0x280000000..0x2ffffffff  0x300000000
>  1 1     0    0x300000000..0x37fffffff  0x300000000

The problem with this encoding is that it requires existing 32-bit
timestamps before 1970 to have encoded "11" in the extra epoch bits,
which is not the case.  Current pre-1970 timestamps would be encoded
with "00" there, which would (according to your table) bump them past
2038 incorrectly.

What we need is an encoding that preserves the times for extra epoch "00":

  2    msb of                         adjustment needed to convert
extra  32-bit                         sign-extended 32-bit tv_sec
 bits   time   decoded 64-bit tv_sec   to decoded 64-bit tv_sec
 0 0     1    -0x80000000..-1           0
 0 0     0    0x000000000..0x07fffffff  0
 0 1     1    0x080000000..0x0ffffffff  0x100000000
 0 1     0    0x100000000..0x17fffffff  0x100000000
 1 0     1    0x180000000..0x1ffffffff  0x200000000
 1 0     0    0x200000000..0x27fffffff  0x200000000
 1 1     1    0x280000000..0x2ffffffff  0x300000000
 1 1     0    0x300000000..0x37fffffff  0x300000000

So, looking at the above desired encoding, it looks like the error in
the existing code is that it is doing a boolean operation on decode
instead of a mathematical one, and it was incorrectly trying to extend
the time to (1ULL<<34).  The below should fix this:

static inline void ext4_decode_extra_time(struct timespec *time, __le32 extra)
{
	if (unlikely(sizeof(time->tv_sec) > 4 &&
		     (extra & cpu_to_le32(EXT4_EPOCH_MASK)))
		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;
}

Cheers, Andreas





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

Powered by blists - more mailing lists