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

[Mark Rutland]

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@xxxxxxxxxx>

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@xxxxxxx>

> ---
>  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.