Re: [RFC PATCH v2 01/10] lib: vdso: ensure all arches have 32bit fallback

[Christophe Leroy]

Le 02/01/2020 Ã 12:29, Arnd Bergmann a ÃcritÂ:
> On Mon, Dec 30, 2019 at 1:27 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy <christophe.leroy@xxxxxx> wrote:
> > > +static __always_inline
> > > +long clock_getres32_fallback(clockid_t _clkid, struct old_timespec32 *_ts)
> > > +{
> > > +       struct __kernel_timespec ts;
> > > +       int ret = clock_getres_fallback(clock, &ts);
> > > +
> > > +       if (likely(!ret && _ts)) {
> > > +               _ts->tv_sec = ts.tv_sec;
> > > +               _ts->tv_nsec = ts.tv_nsec;
> > > +       }
> > > +       return ret;
> > > +}
> >
> > Please change these to call __NR_clock_gettime and __NR_clock_getres_time
> > instead of __NR_clock_gettime64/__NR_clock_getres_time64 for multiple reasons.
> >
> > - When doing migration between containers, the vdso may get copied into
> >    an application running on a kernel that does not support the time64
> >    variants, and then the fallback fails.
> >
> > - When CONFIG_COMPAT_32BIT_TIME is disabled, the time32 syscalls
> >    return -ENOSYS, and the vdso version should have the exact same behavior
> >    to avoid surprises. In particular an application that checks clock_gettime()
> >    to see if the time32 are in part of the kernel would get an incorrect result
> >    here.
> >
> > arch/arm64/include/asm/vdso/compat_gettimeofday.h already does this,
> > I think you can just copy the implementation or find a way to share it.
>
> There was a related discussion on this after a vdso regression on mips,
> and I suggested to drop the time32 functions completely from the
> vdso when CONFIG_COMPAT_32BIT_TIME is disabled, such as
>
> diff --git a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> index 00c025ba4a92..605f259fa24c 100644
> --- a/arch/powerpc/kernel/vdso32/vdso32.lds.S
> +++ b/arch/powerpc/kernel/vdso32/vdso32.lds.S
> @@ -145,10 +145,12 @@ VERSION
>
>                  __kernel_get_syscall_map;
>   #ifndef CONFIG_PPC_BOOK3S_601
> +#ifdef CONFIG_COMPAT_32BIT_TIME
>                  __kernel_gettimeofday;
>                  __kernel_clock_gettime;
>                  __kernel_clock_getres;
>                  __kernel_time;
> +#endif
>                  __kernel_get_tbfreq;
>   #endif
>                  __kernel_sync_dicache;
>
> Any opinions on this? If everyone agrees with that approach, I can
> send a cross-architecture patch to do this everywhere. It's probably
> best though if Christophe adds that to his series as it touches a lot
> of the same files and I would prefer to avoid conflicting changes.

I guess it would be wise.

I don't think my series to switch powerpc to C VDSO will get ready 
anytime soon, because (in addition to the performance impact) I'm having 
hard time with the 32 bits VDSO for PPC64. Many of the powerpc header 
files used by lib/vdso/gettimeofday.c are not ready for generating 32 
bits code for PPC64. Main problem is that at many places, #ifdef 
CONFIG_PPC64 is used instead of #ifdef __powerpc64__. There are also 
some CONFIG options like CONFIG_GENERIC_ATOMIC64 that are selected only 
when CONFIG_PPC32 is set, but which are required for building the 32 
bits VDSO. For that I don't even know how to deal with it.

So, feel free to send your patch, and if my series comes early enough to 
conflict, I'll manage it.

Christophe