Re: [PATCH 1/3] arm64: Introduce a way to disable the 32bit vdso

Mark Rutland
2020-07-02T10:18:27Z

On Wed, Jul 01, 2020 at 05:18:22PM +0100, Marc Zyngier wrote:
> We have a class of errata (grouped under the ARM64_WORKAROUND_1418040
> banner) that force the trapping of counter access from 32bit EL0.
>
> We would normally disable the whole vdso for such defect, except that
> it would disable it for 64bit userspace as well, which is a shame.
>
> Instead, add a new vdso_clock_mode, which signals that the vdso
> isn't usable for compat tasks. This gets checked in the new
> vdso_clocksource_ok() helper, now provided for the 32bit vdso.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>

As-is this looks sound to me. It's not entirely clear to me how this
will compose with VDSO_CLOCKMODE_TIMENS, but given this series is fixing
an existing brokenness I think this has higher priority.

Minor comment below, but regardless:

Acked-by: Mark Rutland <mark.rutland@arm.com>

> ---
> arch/arm64/include/asm/vdso/clocksource.h | 7 +++++--
> arch/arm64/include/asm/vdso/compat_gettimeofday.h | 8 +++++++-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/vdso/clocksource.h b/arch/arm64/include/asm/vdso/clocksource.h
> index df6ea65c1dec..b054d9febfb5 100644
> --- a/arch/arm64/include/asm/vdso/clocksource.h
> +++ b/arch/arm64/include/asm/vdso/clocksource.h
> @@ -2,7 +2,10 @@
> #ifndef __ASM_VDSOCLOCKSOURCE_H
> #define __ASM_VDSOCLOCKSOURCE_H
>
> -#define VDSO_ARCH_CLOCKMODES \
> - VDSO_CLOCKMODE_ARCHTIMER
> +#define VDSO_ARCH_CLOCKMODES \
> + /* vdso clocksource for both 32 and 64bit tasks */ \
> + VDSO_CLOCKMODE_ARCHTIMER, \
> + /* vdso clocksource for 64bit tasks only */ \
> + VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT
>
> #endif
> diff --git a/arch/arm64/include/asm/vdso/compat_gettimeofday.h b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
> index b6907ae78e53..9a625e8947ff 100644
> --- a/arch/arm64/include/asm/vdso/compat_gettimeofday.h
> +++ b/arch/arm64/include/asm/vdso/compat_gettimeofday.h
> @@ -111,7 +111,7 @@ static __always_inline u64 __arch_get_hw_counter(s32 clock_mode)
> * update. Return something. Core will do another round and then
> * see the mode change and fallback to the syscall.
> */
> - if (clock_mode == VDSO_CLOCKMODE_NONE)
> + if (clock_mode != VDSO_CLOCKMODE_ARCHTIMER)
> return 0;
>
> /*
> @@ -152,6 +152,12 @@ static __always_inline const struct vdso_data *__arch_get_vdso_data(void)
> return ret;
> }
>
> +static inline bool vdso_clocksource_ok(const struct vdso_data *vd)
> +{
> + return vd->clock_mode == VDSO_CLOCKMODE_ARCHTIMER;
> +}
> +#define vdso_clocksource_ok vdso_clocksource_ok

Existing issue, but it's a shame this doesn't take the clock mode
directly, then we wouldn't have to duplicate the conditions (as above,
inverted).

Mark.