[PATCH 0/3] arm64: Allow the compat vdso to be disabled at runtime

[PATCH 0/3] arm64: Allow the compat vdso to be disabled at runtime

[Marc Zyngier]

The relatively recent introduction of the compat vdso on arm64 has
overlooked its interactions with some of the interesting errata
workarounds, such as ARM64_ERRATUM_1418040 (and its older 1188873
incarnation).

This erratum requires the 64bit kernel to trap 32bit accesses to the
virtual counter and emulate it. When the workaround was introduced,
the compat vdso simply wasn't a thing. Now that the patches have
landed in mainline, we trap the CVTVCT accesses from the vdso.

This can end-up in a nasty loop in the vdso, where the sequence number
changes on each trap, never stabilising, and leaving userspace in a
bit of a funny state (which is why we disable the vdso in most similar
cases). This erratum mentionned above is a bit special in the sense
that in only requires to trap AArch32 accesses, and 64bit tasks can be
left alone. Consequently, the vdso is never disabled and AArch32 tasks
are affected.

Obviously, we really want to retain the 64bit vdso in this case. To
that effect, this series offers a way to disable the 32bit view of the
vdso without impacting its 64bit counterpart, by providing a
"no-compat" vdso clock_mode, and plugging this feature into the
1418040 detection code.

I haven't put any Cc stable for now, but I believe we need to do
something for older kernels.

Marc Zyngier (3):
  arm64: Introduce a way to disable the 32bit vdso
  arm64: arch_timer: Allow an workaround descriptor to provide
    vdso_clock_mode
  arm64: timers: Disable the compat vdso for cores affected by
    ARM64_WORKAROUND_1418040

 arch/arm64/include/asm/arch_timer.h               |  3 +++
 arch/arm64/include/asm/vdso/clocksource.h         |  7 +++++--
 arch/arm64/include/asm/vdso/compat_gettimeofday.h |  8 +++++++-
 drivers/clocksource/arm_arch_timer.c              | 11 +++++++++++
 4 files changed, 26 insertions(+), 3 deletions(-)

-- 
2.27.0

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

[Marc Zyngier]

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>
---
 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
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_VDSO_GETTIMEOFDAY_H */
-- 
2.27.0

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

[PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode

[Marc Zyngier]

As we are about to disable the vdso for compat tasks in some circumstances,
let's allow a workaround descriptor to provide the vdso_clock_mode that
matches the platform.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/arch_timer.h  | 3 +++
 drivers/clocksource/arm_arch_timer.c | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 7ae54d7d333a..fb8dfcbf9c01 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -18,6 +18,8 @@
 #include <linux/smp.h>
 #include <linux/types.h>
 
+#include <vdso/clocksource.h>
+
 #include <clocksource/arm_arch_timer.h>
 
 #if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
@@ -58,6 +60,7 @@ struct arch_timer_erratum_workaround {
 	u64 (*read_cntvct_el0)(void);
 	int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
 	int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
+	enum vdso_clock_mode vdso_clock_mode;
 };
 
 DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index ecf7b7db2d05..f828835c568f 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
 	if (wa->read_cntvct_el0) {
 		clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
 		vdso_default = VDSO_CLOCKMODE_NONE;
+	} else {
+		clocksource_counter.vdso_clock_mode = wa->vdso_clock_mode;
+		vdso_default = wa->vdso_clock_mode;
 	}
 }
 
-- 
2.27.0

Re: [PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode

[Mark Rutland]

On Wed, Jul 01, 2020 at 05:18:23PM +0100, Marc Zyngier wrote:
> As we are about to disable the vdso for compat tasks in some circumstances,
> let's allow a workaround descriptor to provide the vdso_clock_mode that
> matches the platform.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/arch_timer.h  | 3 +++
>  drivers/clocksource/arm_arch_timer.c | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 7ae54d7d333a..fb8dfcbf9c01 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -18,6 +18,8 @@
>  #include <linux/smp.h>
>  #include <linux/types.h>
>  
> +#include <vdso/clocksource.h>
> +
>  #include <clocksource/arm_arch_timer.h>
>  
>  #if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
> @@ -58,6 +60,7 @@ struct arch_timer_erratum_workaround {
>  	u64 (*read_cntvct_el0)(void);
>  	int (*set_next_event_phys)(unsigned long, struct clock_event_device *);
>  	int (*set_next_event_virt)(unsigned long, struct clock_event_device *);
> +	enum vdso_clock_mode vdso_clock_mode;
>  };
>  
>  DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index ecf7b7db2d05..f828835c568f 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct arch_timer_erratum_workaround *wa
>  	if (wa->read_cntvct_el0) {
>  		clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
>  		vdso_default = VDSO_CLOCKMODE_NONE;
> +	} else {
> +		clocksource_counter.vdso_clock_mode = wa->vdso_clock_mode;
> +		vdso_default = wa->vdso_clock_mode;
>  	}

I fear that we're liable to forget to set vdso_clock_mode on new errata
that don't need a read_cntvct_el0 hook, and if so we'll happen to set
these to 0 (i.e. VDSO_CLOCKMODE_NONE).

For now could we instead have a boolan disable_compat_vdso, with this
being:

| if (wa->read_cntvct_el0) {
| 	clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
| 	vdso_default = VDSO_CLOCKMODE_NONE;
| } else if (wa->disable_compat_vdso) {
| 	clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
| 	vdso_default = wa->vdso_clock_mode = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
| }

Do we need to handle the comination of a workaround seeting NONE and one
setting ARCHTIMER_NOCOMPAT?

Mark.

Re: [PATCH 2/3] arm64: arch_timer: Allow an workaround descriptor to provide vdso_clock_mode

[Marc Zyngier]

On 2020-07-02 11:28, Mark Rutland wrote:
> On Wed, Jul 01, 2020 at 05:18:23PM +0100, Marc Zyngier wrote:
>> As we are about to disable the vdso for compat tasks in some 
>> circumstances,
>> let's allow a workaround descriptor to provide the vdso_clock_mode 
>> that
>> matches the platform.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/arch_timer.h  | 3 +++
>>  drivers/clocksource/arm_arch_timer.c | 3 +++
>>  2 files changed, 6 insertions(+)
>> 
>> diff --git a/arch/arm64/include/asm/arch_timer.h 
>> b/arch/arm64/include/asm/arch_timer.h
>> index 7ae54d7d333a..fb8dfcbf9c01 100644
>> --- a/arch/arm64/include/asm/arch_timer.h
>> +++ b/arch/arm64/include/asm/arch_timer.h
>> @@ -18,6 +18,8 @@
>>  #include <linux/smp.h>
>>  #include <linux/types.h>
>> 
>> +#include <vdso/clocksource.h>
>> +
>>  #include <clocksource/arm_arch_timer.h>
>> 
>>  #if IS_ENABLED(CONFIG_ARM_ARCH_TIMER_OOL_WORKAROUND)
>> @@ -58,6 +60,7 @@ struct arch_timer_erratum_workaround {
>>  	u64 (*read_cntvct_el0)(void);
>>  	int (*set_next_event_phys)(unsigned long, struct clock_event_device 
>> *);
>>  	int (*set_next_event_virt)(unsigned long, struct clock_event_device 
>> *);
>> +	enum vdso_clock_mode vdso_clock_mode;
>>  };
>> 
>>  DECLARE_PER_CPU(const struct arch_timer_erratum_workaround *,
>> diff --git a/drivers/clocksource/arm_arch_timer.c 
>> b/drivers/clocksource/arm_arch_timer.c
>> index ecf7b7db2d05..f828835c568f 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -566,6 +566,9 @@ void arch_timer_enable_workaround(const struct 
>> arch_timer_erratum_workaround *wa
>>  	if (wa->read_cntvct_el0) {
>>  		clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
>>  		vdso_default = VDSO_CLOCKMODE_NONE;
>> +	} else {
>> +		clocksource_counter.vdso_clock_mode = wa->vdso_clock_mode;
>> +		vdso_default = wa->vdso_clock_mode;
>>  	}
> 
> I fear that we're liable to forget to set vdso_clock_mode on new errata
> that don't need a read_cntvct_el0 hook, and if so we'll happen to set
> these to 0 (i.e. VDSO_CLOCKMODE_NONE).
> 
> For now could we instead have a boolan disable_compat_vdso, with this
> being:
> 
> | if (wa->read_cntvct_el0) {
> | 	clocksource_counter.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
> | 	vdso_default = VDSO_CLOCKMODE_NONE;
> | } else if (wa->disable_compat_vdso) {
> | 	clocksource_counter.vdso_clock_mode = 
> VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
> | 	vdso_default = wa->vdso_clock_mode = 
> VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT;
> | }

Sure, that'd work too.

> Do we need to handle the comination of a workaround seeting NONE and 
> one
> setting ARCHTIMER_NOCOMPAT?

Not yet. Probably coming any time now. Which is why we may need to
look into per-CPU vdso data pages (Broonie has a series that could
be of some use, apparently).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 3/3] arm64: timers: Disable the compat vdso for cores affected by ARM64_WORKAROUND_1418040

[Marc Zyngier]

ARM64_WORKAROUND_1418040 requires that AArch32 EL0 accesses to
the virtual counter register are trapped and emulated by the kernel.
This makes the vdso pretty pointless, and in some cases livelock
prone.

Provide a workaround entry that limits the vdso to 64bit tasks.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/clocksource/arm_arch_timer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index f828835c568f..9c61252e7d99 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -480,6 +480,14 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
 		.set_next_event_virt = erratum_set_next_event_tval_virt,
 	},
 #endif
+#ifdef CONFIG_ARM64_ERRATUM_1418040
+	{
+		.match_type = ate_match_local_cap_id,
+		.id = (void *)ARM64_WORKAROUND_1418040,
+		.desc = "ARM erratum 1418040",
+		.vdso_clock_mode = VDSO_CLOCKMODE_ARCHTIMER_NOCOMPAT,
+	},
+#endif
 };
 
 typedef bool (*ate_match_fn_t)(const struct arch_timer_erratum_workaround *,
-- 
2.27.0