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

[Aurelien Jarno <aurelien@aurel32.net>]

On 2022-11-01 09:40, Adhemerval Zanella Netto via Libc-alpha wrote:
> 
> 
> 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.

Sounds good. I have tested that solution and confirm it works fine. I
have just sent a proper patch for that.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net