Re: [PATCH 15/16] linux: Add {f}stat{at} y2038 support

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 5669 bytes --]

Hi Adhemerval,

> On 13/10/2020 11:18, Adhemerval Zanella wrote:
> > 
> > 
> > On 13/10/2020 10:58, Lukasz Majewski wrote:  
> >> Hi Adhemerval,
> >>  
> >>> On 07/10/2020 09:52, Adhemerval Zanella wrote:  
> >>>>
> >>>>
> >>>> On 06/10/2020 06:48, Lukasz Majewski wrote:    
> >>>>> Hi Adhemerval,
> >>>>>    
> >>>>>> A new struct __stat{64}_t64 type is added with the required
> >>>>>> __timespec64 time definition.  Both non-LFS and LFS support
> >>>>>> were done with an extra __NR_statx call plus a conversion to
> >>>>>> the new __stat{64}_t64 type.  The statx call is done only for
> >>>>>> architectures with support for 32-bit time_t ABI.
> >>>>>>
> >>>>>> Internally some extra routines to copy from/to struct stat{64}
> >>>>>> to struct __stat{64} used on multiple implementations (stat,
> >>>>>> fstat, lstat, and fstatat) are added on a extra file
> >>>>>> (stat_t64_cp.c).  Aslo some extra routines to copy from statx
> >>>>>> to __stat{64} is added on statx_cp.c.
> >>>>>>
> >>>>>> Checked with a build for all affected ABIs. I also checked on
> >>>>>> x86_64, i686, powerpc, powerpc64le, sparcv9, sparc64, s390, and
> >>>>>> s390x.    
> >>>>>
> >>>>> When do you plan to pull this patch set to -master?
> >>>>> Those patches have been available for review on the mailing list
> >>>>> for more than two months now.    
> >>>>
> >>>> Hi Lukasz, thanks to remind me. I will rebase against master and
> >>>> run some regressions tests against some platforms and push it.
> >>>>     
> >>>
> >>> One required change with the rebase is adapt the riscv32 ABI to
> >>> exclude the __{f,l}xstat{at} symbol and replace with proper
> >>> {f,l}stat ones. It is possible because the new ABI was added on
> >>> current development branch, however one minor inconvenient is the
> >>> toolchain need to be rebuild with a updated glibc branch to avoid
> >>> linking failures with libstd++ (which uses __{f,l}xstat{at}).
> >>>  
> >>
> >> I'm not sure if this is related, but on my ARMv7 (32 bit) sandbox
> >> there is an issue with fstat accesses to files.
> >>
> >> When I try to run a program build against newest glibc (installed
> >> in /opt/lib) I do see issues with {f}stat on other libraries (e.g.
> >> /opt/lib/librt.so). To be more specific I do experience the
> >> EOVERFLOW error:
> >>
> >> error while loading shared libraries: librt.so.1: cannot stat
> >> shared object: Error 75
> >>
> >> The "base" glibc is 2.28 (installed in /lib). The glibc under test
> >> is the newest master installed in /opt/lib.
> >>
> >> I'm now investigating this issue.  
> > 
> > I am not sure what it might be based on these information, could you
> > provide a strace so we can pinpoint what might the issue?  
> > 
> > The arm-linux-gnueabihf testing I did was on a aarch64 kernel
> > (4.12.13). Besides the make check without regression, I could run
> > system binaries with ./testrun.sh.
> > 
> > I will check on a different kernel/system with a 32-bit kernel.  
> 
> Ok, this change in fact triggered a very subtle issue at dl-load.c
> that I saw in both arm-linux-gnueabihf system with a 32-bit kernel
> and on mips-linux-gnu.
> 
> The issue is at:
> 
> elf/dl-load.c
> 
> 1982       if (here_any && (err = errno) != ENOENT && err != EACCES)
> 1983         /* The file exists and is readable, but something went
> wrong.  */ 1984         return -1;
> 
> And it is just triggered on system where {f,l}stat{at}{64} issues
> __NR_statx and it fails with ENOSYS but later success with the system
> stat* syscall.
> 
> This code here checks the errno value without checking whether the
> previous function call that might change err actually has failed
> (in this specific case the stat64 at line 1931). And this due how we 
> currently implement the y2038 support with INLINE_SYSCALL_CALL 
> (a function that succeeds is allowed to change errno and it
> simplifies the resulting y2038 support a bit).
> 
> In fact this check does not really make sense, since either 'fd' will
> be different than '0' (meaning it has being opened) or the 'stat64'
> at line 1931 failed and 'here_any' won't be set (the stat64 at line
> 1951 already explicit sets errno in failure case).  
> 
> Also, git history does not give much information on why it was added
> at fist place.  So I think we just need to remove this extra check,
> you can check if the following patch helps (I am running some
> regression tests before sensing it upstream):
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index f3201e7c14..39ae43c6ce 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1878,7 +1878,6 @@ open_path (const char *name, size_t namelen,
> int mode, size_t cnt;
>        char *edp;
>        int here_any = 0;
> -      int err;
>  
>        /* If we are debugging the search for libraries print the path
>          now if it hasn't happened now.  */
> @@ -1979,9 +1978,6 @@ open_path (const char *name, size_t namelen,
> int mode, return -1;
>             }
>         }
> -      if (here_any && (err = errno) != ENOENT && err != EACCES)
> -       /* The file exists and is readable, but something went wrong.
>  */
> -       return -1;
>  
>        /* Remember whether we found anything.  */
>        any |= here_any;
> 
> 

I've tested this patch and it doesn't fix my issue.

> 
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]