On Mon, Dec 23, 2019 at 3:31 PM Christophe Leroy
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
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, );
> +
> + 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, );
> +
> + 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, );
> +
> + 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_FALLBACK1
> -
> 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, );
> +
> + 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, );
>
> -#ifdef VDSO_HAS_32BIT_FALLBACK
> if (unlikely(ret))
> return clock_gettime32_fallback(clock, res);
> -#else
> - if (unlikely(ret))
> - ret = clock_gettime_fallback(clock, );
> -#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, );
>
> -#ifdef VDSO_HAS_32BIT_FALLBACK
> if (unlikely(ret))
> return
