Messages in this thread | | | From | Arnd Bergmann <> | Date | Mon, 30 Dec 2019 13:27:39 +0100 | Subject | Re: [RFC PATCH v2 01/10] lib: vdso: ensure all arches have 32bit fallback |
| |
On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote: > > In order to simplify next step which moves fallback call at arch > level, ensure all arches have a 32bit fallback instead of handling > the lack of 32bit fallback in the common code based > on VDSO_HAS_32BIT_FALLBACK > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
I like the idea of removing VDSO_HAS_32BIT_FALLBACK and ensuring that all 32-bit architectures implement them, but we really should *not* have any implementation calling the 64-bit syscalls.
> +static __always_inline > +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts) > +{ > + struct __kernel_timespec ts; > + int ret = clock_gettime_fallback(clock, &ts); > + > + if (likely(!ret)) { > + _ts->tv_sec = ts.tv_sec; > + _ts->tv_nsec = ts.tv_nsec; > + } > + return ret; > +} > + > +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.
> diff --git a/arch/arm64/include/asm/vdso/gettimeofday.h b/arch/arm64/include/asm/vdso/gettimeofday.h > index b08f476b72b4..c41c86a07423 100644 > --- a/arch/arm64/include/asm/vdso/gettimeofday.h > +++ b/arch/arm64/include/asm/vdso/gettimeofday.h > @@ -66,6 +66,32 @@ int clock_getres_fallback(clockid_t _clkid, struct __kernel_timespec *_ts) > return ret; > } > > +static __always_inline > +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts) > +{ > + struct __kernel_timespec ts; > + int ret = clock_gettime_fallback(clock, &ts); > + > + if (likely(!ret)) { > + _ts->tv_sec = ts.tv_sec; > + _ts->tv_nsec = ts.tv_nsec; > + } > + return ret; > +}
As Andy said, this makes no sense at all, nothing should ever call this on a 64-bit architecture.
> diff --git a/arch/mips/include/asm/vdso/gettimeofday.h b/arch/mips/include/asm/vdso/gettimeofday.h > index b08825531e9f..60608e930a5c 100644 > --- a/arch/mips/include/asm/vdso/gettimeofday.h > +++ b/arch/mips/include/asm/vdso/gettimeofday.h > @@ -109,8 +109,6 @@ static __always_inline int clock_getres_fallback( > > #if _MIPS_SIM != _MIPS_SIM_ABI64 > > -#define VDSO_HAS_32BIT_FALLBACK 1 > - > static __always_inline long clock_gettime32_fallback( > clockid_t _clkid, > struct old_timespec32 *_ts) > @@ -150,6 +148,32 @@ static __always_inline int clock_getres32_fallback( > > return error ? -ret : ret; > } > +#else > +static __always_inline > +long clock_gettime32_fallback(clockid_t _clkid, struct old_timespec32 *_ts) > +{ > + struct __kernel_timespec ts; > + int ret = clock_gettime_fallback(clock, &ts); > + > + if (likely(!ret)) { > + _ts->tv_sec = ts.tv_sec; > + _ts->tv_nsec = ts.tv_nsec; > + } > + return ret; > +} > +
Same here.
> --- a/lib/vdso/gettimeofday.c > +++ b/lib/vdso/gettimeofday.c > @@ -125,13 +125,8 @@ __cvdso_clock_gettime32(clockid_t clock, struct old_timespec32 *res) > > ret = __cvdso_clock_gettime_common(clock, &ts); > > -#ifdef VDSO_HAS_32BIT_FALLBACK > if (unlikely(ret)) > return clock_gettime32_fallback(clock, res); > -#else > - if (unlikely(ret)) > - ret = clock_gettime_fallback(clock, &ts); > -#endif > > if (likely(!ret)) { > res->tv_sec = ts.tv_sec;
Removing the #ifdef and the fallback seems fine. I think this is actually required for correctness on arm32 as well. Maybe enclose the entire function in
#ifdef VDSO_HAS_CLOCK_GETTIME32
to only define it when it is called?
> @@ -238,13 +233,8 @@ __cvdso_clock_getres_time32(clockid_t clock, struct old_timespec32 *res) > > ret = __cvdso_clock_getres_common(clock, &ts); > > -#ifdef VDSO_HAS_32BIT_FALLBACK > if (unlikely(ret)) > return clock_getres32_fallback(clock, res); > -#else > - if (unlikely(ret)) > - ret = clock_getres_fallback(clock, &ts); > -#endif
The same applies to all the getres stuff of course.
Arnd
| |