Re: Y2038 bug in ext4 recently_deleted() function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Aug 18, 2017 at 3:23 AM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
>> Strange, I never even knew recently_deleted() existed, even though it was
>> added to the tree 4 years ago yesterday.  It looks like this is only used
>> with the no-journal code, which I don't really interact with.
>>
>> One thing I did notice when looking at it is that there is a Y2038 bug in
>> recently_deleted(), as it is comparing 32-bit i_dtime directly with 64-bit
>> get_seconds().
>
> I don't think dtime has widened on the disk layout for ext4 according
> to https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout. So I am
> not sure how fixing the internal implementation would be useful until
> we do that. Is there a plan for that?
>
> As far as get_seconds() is concerned, get_seconds() returns unsigned
> long which is 64 bits on a 64 bit arch and 32 bit on a 32 bit arch.
> Since dtime variable is declared as unsigned long in this function,
> same holds for the size of this variable.
>
> There is no y2038 problem on a 64 bit machine.

I think what Andreas was saying is that it's actually the opposite:
on a 32-bit machine, the code will work correctly for 32-bit unsigned
long values as long as 'dtime' and 'now' are in the same epoch,
e.g. both are before 2106 or both are after.

On 64-bit systems it's always wrong after 2106.

> So moving to the case of a 32 bit machine:
>
> get_seconds() can return values until year 2106. And, recentcy at max
> can only be 35. Analyzing the current line:
>
> if (dtime && (dtime < now) && (now < dtime + recentcy))
>
> The above equation should work fine at least until 35 seconds before
> y2038 deadline.

Since it's all unsigned arithmetic, it should be fine until 2106.
However, we should get rid of get_seconds() long before then
and use ktime_get_real_seconds() instead, as most other users
of get_seconds() are (more) broken.

Looking at the two suggested approaches:

>>        u32 now, dtime;
>>
>>        /* assume dtime is within the past 30 years, see time_after() */
>>        now = get_seconds();
>>        if (dtime && (dtime - now < 0) && (dtime + recentcy - now < 0))
>>                ret = 1;

* As 'dtime' and 'now' are both unsigned, subtracting them will also result
  in an unsigned value that is never less than zero, so it won't work.
  Adding a cast to 's32' would fix that the same way that time_after() does.

* please use ktime_get_real_seconds() instead of get_seconds(), so we
   don't have to replace it later.

* The comment should say '68 years', not 30.

> or use i_ctime_extra to implicitly extend i_dtime beyond 2038, something like:
>
>        /* assume dtime epoch same as ctime, see EXT4_INODE_GET_XTIME() */
>        dtime = le32_to_cpu(raw_inode->i_dtime);
>        if (EXT4_INODE_SIZE(sb) > EXT4_GOOD_OLD_INODE_SIZE &&
>            offsetof(typeof(*raw_inode), i_ctime_extra) + 4 <=
>            EXT4_GOOD_OLD_INODE_SIZE + le32_to_cpu(raw_inode->i_extra_isize))
>                dtime += (long)(le32_to_cpu(raw_inode->i_ctime_extra) &
>                                EXT4_EPOCH_MASK) << 32;

* This is slightly incorrect when we are close to the epoch boundary, as i_ctime
  and i_dtime might end up being in different epochs. I would not go there.

* If we were to pick this approach, a cast to 'long' is obviously wrong on
  32-bit systems, better use 'u64' or 'time64_t'.

     Arnd



[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux