> On Aug 18, 2017, at 9:38 AM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote: > > On Fri, Aug 18, 2017 at 2:31 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote: >> On Fri, Aug 18, 2017 at 3:23 AM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote: >>> >>>> 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. > > There is some confusion here. > I was only referring to the current implementation: > > static int recently_deleted(struct super_block *sb, ext4_group_t group, int ino) > { > . > . > . > unsigned long dtime, now; > int offset, ret = 0, recentcy = RECENTCY_MIN; > . > . > . > offset = (ino % inodes_per_block) * EXT4_INODE_SIZE(sb); > raw_inode = (struct ext4_inode *) (bh->b_data + offset); > dtime = le32_to_cpu(raw_inode->i_dtime); > now = get_seconds(); > if (buffer_dirty(bh)) > recentcy += RECENTCY_DIRTY; > > if (dtime && (dtime < now) && (now < dtime + recentcy)) > ret = 1; > . > . > . > } > > In the above implementation, I do not see any problem on a 64 bit machine. > The only problem is that dtime on disk representation is signed 32 bits only. > If that were not a problem then this would be fine from time prespective. The 32-bit dtime is the root of the problem. There is no plan to extend the dtime field on disk, because it is used so little (mostly as a boolean value, and for forensics). >>> 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. > > Dtime on disk representation again breaks this for certain values in > 2038 even though everything is unsigned. > > I was just saying that whatever we do here depends on how dtime on > disk is interpreted. > > Agree that ktime_get_real_seconds() should be used here. But, the way > we handle new values would rely on this new interpretation of dtime. > Also, using time64_t variables on stack only matters after this. Once > the types are corrected, maybe the comparison expression need not > change at all (after new dtime interpretation is in place). There will not be a new dtime format on disk, but since the calculation here only depends on relative times (within a few minutes), then it would be fine to use only 32-bit timestamps, and truncate off the high bits from get_seconds()/ktime_get_real_seconds(). Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
