Re: [PATCH] linux: Fix fstatat with !XSTAT_IS_XSTAT64 and __TIMESIZE=64 (BZ #29730)

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 01/11/22 08:57, Aurelien Jarno wrote:
> On 2022-11-01 11:55, Aurelien Jarno wrote:
>> On 2022-10-31 23:20, Aurelien Jarno wrote:
>>> On 2022-10-31 14:25, Adhemerval Zanella Netto via Libc-alpha wrote:
>>>>
>>>>
>>>> On 31/10/22 13:03, Adhemerval Zanella Netto wrote:
>>>>>
>>>>>
>>>>> On 28/10/22 17:54, Aurelien Jarno wrote:
>>>>>> Commit 6e8a0aac2f883 ("time: Fix overflow itimer tests on 32-bit
>>>>>> systems") changed in_time_t_range to assume a 32-bit time_t. Therefore
>>>>>> stop calling it from __fstatat for systems with 64-bit time_t.
>>>>>>
>>>>>> Resolves: BZ #29730
>>>>>
>>>>> Although this change is not incorrect, I think the fix I just sent [1] is 
>>>>> slight better because it just alias non-LFS stat to LFS stat for mips 
>>>>> (which is also a small runtime and code optimization), while avoiding 
>>>>> touching generic code (so we need to check if trigger another issue on 
>>>>> different architecture).
>>>>>
>>>>> [1] https://sourceware.org/pipermail/libc-alpha/2022-October/143089.html
>>>>
>>>> So I was wrong that we can assume stat and stat64 are equal on mips.  Maybe
>>>> instead of adding the fix on stat we can make it more generic and move it
>>>> to is_time_t_range (so it can potentially fix usage not only on stat code)
>>>> as my comment on https://sourceware.org/bugzilla/show_bug.cgi?id=29730 ?
>>>
>>> Yes, we can move it to is_time_t_range, but then it gets trickier to get
>>> the condition. Your comment in BZ #29730 uses TIME64_NON_DEFAULT, but it
>>> is not defined for OpenRISC, so it means it will cast to time_t. However
>>> this has been explicitly changed that way in commit 6e8a0aac2f883 to
>>> unbreak OpenRISC.
>>>
>>> We probably want a condition that is "kernel ABI uses 32-bit time_t". Do
>>> we have something like that already defined?
>>
>> I guess this can be checked with:
>> (__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))
>>
>> And then we can rename the function to is_ktime_t_range to better
>> reflect its behaviour.
> 
> Looking at it more in details, I am not sure it's a good idea, as this
> function is also used for non-syscall cases, like in mktime or timegm.
> While that works because there is a __TIMESIZE != 64 guard, this still
> seems fragile.

The "(__WORDSIZE == 32 && (!defined __SYSCALL_WORDSIZE || __SYSCALL_WORDSIZE == 32))"
is a Linuxism and I agree it should not be used in generic code.

> 
> In that regard, I think the approach from YunQiang to explicitly call
> either one or the other function might be better.
> 
> An hybrid solution would be to define both in_time_t_range and
> in_ktime_t_range, the later being the one called in_int32_t_range in
> YunQiang's patch.
> 
> What do you think?
> 

I am more inclined to either go with your initial patch that handles it on
fstatat or go with a mips specific fstatat.c to make clear about MIPS ABI
idiosyncrasies (which is something I tried to avoid, but for this specific
case seems to make sense):

* sysdeps/unix/sysv/linux/mips/mips64/n64/fstatat.c:

/* Different than other ABIS, mips64 has different layouts for non-LFS
   and LFS struct stat.  */
int
__fstatat (int fd, const char *file, struct stat *buf, int flag)
{
  struct __stat64_t64 st64;
  int r = __fstatat64_time64 (fd, file, &st64, flag);
  if (r == 0)
    {
      /* Clear internal pad and reserved fields.  */
      memset (buf, 0, sizeof (*buf));

      buf->st_dev = st64.st_dev;
      buf->st_ino = st64.st_ino;
      buf->st_mode = st64.st_mode;
      buf->st_nlink = st64.st_nlink;
      buf->st_uid = st64.st_uid;
      buf->st_gid = st64.st_gid;
      buf->st_rdev = st64.st_rdev;
      buf->st_size = st64.st_size;
      buf->st_blksize = st64.st_blksize;
      buf->st_blocks  = st64.st_blocks;
      buf->st_atim = st64.st_atim;
      buf->st_mtim = st64.st_mtim;
      buf->st_ctim = st64.st_ctim;
    }
  return r;
}
 
We can then treat the YunQiang patch as a code refactor instead.