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

From: Arnd Bergmann
Date: Mon Dec 30 2019 - 07:28:04 EST


On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy
<christophe.leroy@xxxxxx> 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@xxxxxx>

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