Message-ID: <CADQUmxXSRkvNFJX2+1Wefd5BWFeBB9pThpiucgyn=TBbvFutVw@mail.gmail.com>
Date:	Fri, 8 Jul 2011 10:06:28 -0700
From:	Mark Harris <mhlk@....us>
To:	Akira Fujita <a-fujita@...jp.nec.com>
Cc:	Andreas Dilger <adilger@...ger.ca>,
	ext4 development <linux-ext4@...r.kernel.org>
Subject: Re: [BUG] ext4 timestamps corruption

Akira Fujita wrote:
> Hi, Andreas and Mark,
> Thank you for looking at this issue.
>
> (2011/06/27 18:04), Andreas Dilger wrote:
>> On 2011-06-24, at 11:12 PM, Mark Harris wrote:
>>> On Fri, Jun 24, 2011 at 15:46, Andreas Dilger wrote:
>>>> 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.
>>>
>>> I was under the impression that the encoding code stored bits
>>> 33&  32 of tv_sec there, which would be 1,1 for a negative value
>>> like -1.  Certainly the decoding would be simpler if the extra
>>> value was only non-zero for large timestamps.
>>
>> One problem with a symmetrical encoding is that it wastes half of the
>> dynamic range for values that nobody will ever use.  Even values before
>> 1970 seem so unlikely that I question whether we should support them
>> at all.
>>
>>> On closer inspection of ext4_encode_extra_time, it looks like for
>>> tv_sec = -1, a 64-bit kernel will store 1,1 in the extra bits and
>>> a 32-bit kernel will store 0,0 in the extra bits.  That is a problem
>>> if both of these need to be decoded as -1 on a 64-bit system.
>>
>> That is definitely a problem.
>>
>>>> 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;
>>>> }
>>>
>>> That is not compatible with the existing ext4_encode_extra_time.
>>> For example, 2038-01-31 (0x80101500) is encoded with extra bits
>>> equal to bits 33&  32, i.e. 0,0.  But this code would decode it
>>> as 1901-12-25 (i.e. it would leave the sign-extended 32-bit value
>>> unchanged).
>>
>> Part of the problem is that the encoding/decoding of timestamps beyond
>> 2038 is already broken today, so I don't think anyone has been using
>> them so far.  This gives us some leeway for fixing this problem I think.
>>
>>> Possible solutions:
>>>
>>> (a) Define the current 64-bit encoding as the correct encoding since
>>>      the 2 extra bits are not even decoded on 32-bit kernels, so its
>>>      encoding doesn't matter much.  However, if anyone with existing
>>>      pre-1970 timestamps written using a 32-bit kernel wants to use
>>>      their ext4 filesystem with a 64-bit kernel, the pre-1970
>>>      timestamps would be wrong unless they re-write them with a
>>>      fixed kernel.
>>>
>>>      Change ext4_decode_extra_time "if" body to something like:
>>>         time->tv_sec += ((__u32)time->tv_sec +
>>>                 ((__u64)le32_to_cpu(extra)<<  32) +
>>>                 0x80000000LL)&  0x300000000LL;
>>>
>>>      Change ext4_encode_extra_time ": 0" to something like:
>>>         time->tv_sec<  0 ? EXT4_EPOCH_MASK : 0
>>
>> The real-world problem isn't with 32-bit systems, where it doesn't
>> really matter at all how time is encoded, nor with files on 64-bit systems
>> with timestamps 26 years in the future, but rather 256-byte inodes that
>> were previously written with ext3 that will break if they are mounted
>> with ext4 on a 64-bit system.
>>
>>> (b) Change the encoding of the extra bits to be those in your new
>>>      table.  This is compatible with the 32-bit kernel encoding
>>>      (which does not decode these bits) but incompatible with the
>>>      64-bit kernel encoding.  Existing pre-1970 timestamps written
>>>      with a 64-bit kernel would be decoded as dates far in the future.
>>>
>>>      Requires your change to ext4_decode_extra_time.
>>>      Also requires a change to ext4_encode_extra_time, changing
>>>      (time->tv_sec>>  32) to something like:
>>>         ((time->tv_sec - (signed int)time->tv_sec)>>  32)
>>
>> I think this is a reasonable solution, but I dislike that it breaks
>> pre-1970 timestamps on 64-bit systems.
>
> I agree with this solution.
> I guess that no one has pre-1970 timestamps on ext4, actually.
>
> Mark, are you working on this right now?
> If you have a patch to fix this issue, please send it to the list.

I think that all of the code changes needed to implement this
solution (b) are in ext4_decode_extra_time and ext4_encode_extra_time
and are included above, if you want to submit them in patch format
as a new version of your patch.  The issue with this solution is
that it breaks existing 1901..1969 timestamps written with a 64-bit
kernel to ext4 with 256-byte inodes.  (It breaks existing year 2038+
timestamps as well, but those are already broken.)  It sounds like
Andreas favors either (b) or (c) but would like to hear from Ted.

 - Mark

>
> Regards,
> Akira Fujita
>
--
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