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
