diff mbox series

[v9,2/4] arm64: mte: change ASYNC and SYNC TCF settings into bitfields

Message ID 20210702194110.2045282-3-pcc@google.com (mailing list archive)
State New, archived
Headers show
Series arm64: mte: allow async MTE to be upgraded to sync on a per-CPU basis | expand

Commit Message

Peter Collingbourne July 2, 2021, 7:41 p.m. UTC 
Allow the user program to specify both ASYNC and SYNC TCF modes by
repurposing the existing constants as bitfields. This will allow the
kernel to select one of the modes on behalf of the user program. With
this patch the kernel will always select async mode, but a subsequent
patch will make this configurable.

Link: https://linux-review.googlesource.com/id/Icc5923c85a8ea284588cc399ae74fd19ec291230
Signed-off-by: Peter Collingbourne <pcc@google.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
---
v9:
- make mte_update_sctlr_user static

 arch/arm64/include/asm/processor.h |  3 ++
 arch/arm64/kernel/mte.c            | 70 ++++++++++++------------------
 include/uapi/linux/prctl.h         | 11 ++---
 3 files changed, 37 insertions(+), 47 deletions(-)

Comments

Will Deacon July 7, 2021, 11:10 a.m. UTC | #1 
On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:
> Allow the user program to specify both ASYNC and SYNC TCF modes by
> repurposing the existing constants as bitfields. This will allow the
> kernel to select one of the modes on behalf of the user program. With
> this patch the kernel will always select async mode, but a subsequent
> patch will make this configurable.
> 
> Link: https://linux-review.googlesource.com/id/Icc5923c85a8ea284588cc399ae74fd19ec291230
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
> v9:
> - make mte_update_sctlr_user static
> 
>  arch/arm64/include/asm/processor.h |  3 ++
>  arch/arm64/kernel/mte.c            | 70 ++++++++++++------------------
>  include/uapi/linux/prctl.h         | 11 ++---
>  3 files changed, 37 insertions(+), 47 deletions(-)

...

>  long set_mte_ctrl(struct task_struct *task, unsigned long arg)
>  {
> -	u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
>  	u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
>  			SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
>  
>  	if (!system_supports_mte())
>  		return 0;
>  
> -	switch (arg & PR_MTE_TCF_MASK) {
> -	case PR_MTE_TCF_NONE:
> -		sctlr |= SCTLR_EL1_TCF0_NONE;
> -		break;
> -	case PR_MTE_TCF_SYNC:
> -		sctlr |= SCTLR_EL1_TCF0_SYNC;
> -		break;
> -	case PR_MTE_TCF_ASYNC:
> -		sctlr |= SCTLR_EL1_TCF0_ASYNC;
> -		break;
> -	default:
> -		return -EINVAL;
> -	}
> +	if (arg & PR_MTE_TCF_ASYNC)
> +		mte_ctrl |= MTE_CTRL_TCF_ASYNC;
> +	if (arg & PR_MTE_TCF_SYNC)
> +		mte_ctrl |= MTE_CTRL_TCF_SYNC;
>  
> -	if (task != current) {
> -		task->thread.sctlr_user = sctlr;
> -		task->thread.mte_ctrl = mte_ctrl;
> -	} else {
> -		set_task_sctlr_el1(sctlr);
> -		set_gcr_el1_excl(mte_ctrl);
> +	task->thread.mte_ctrl = mte_ctrl;
> +	if (task == current) {
> +		mte_update_sctlr_user(task);

In conjunction with the next patch, what happens if we migrate at this
point? I worry that we can install a stale sctlr_user value.

> +		set_task_sctlr_el1(task->thread.sctlr_user);

Will
Peter Collingbourne July 12, 2021, 7:04 p.m. UTC | #2 
On Wed, Jul 7, 2021 at 4:11 AM Will Deacon <will@kernel.org> wrote:
>
> On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:
> > Allow the user program to specify both ASYNC and SYNC TCF modes by
> > repurposing the existing constants as bitfields. This will allow the
> > kernel to select one of the modes on behalf of the user program. With
> > this patch the kernel will always select async mode, but a subsequent
> > patch will make this configurable.
> >
> > Link: https://linux-review.googlesource.com/id/Icc5923c85a8ea284588cc399ae74fd19ec291230
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> > v9:
> > - make mte_update_sctlr_user static
> >
> >  arch/arm64/include/asm/processor.h |  3 ++
> >  arch/arm64/kernel/mte.c            | 70 ++++++++++++------------------
> >  include/uapi/linux/prctl.h         | 11 ++---
> >  3 files changed, 37 insertions(+), 47 deletions(-)
>
> ...
>
> >  long set_mte_ctrl(struct task_struct *task, unsigned long arg)
> >  {
> > -     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
> >       u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
> >                       SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
> >
> >       if (!system_supports_mte())
> >               return 0;
> >
> > -     switch (arg & PR_MTE_TCF_MASK) {
> > -     case PR_MTE_TCF_NONE:
> > -             sctlr |= SCTLR_EL1_TCF0_NONE;
> > -             break;
> > -     case PR_MTE_TCF_SYNC:
> > -             sctlr |= SCTLR_EL1_TCF0_SYNC;
> > -             break;
> > -     case PR_MTE_TCF_ASYNC:
> > -             sctlr |= SCTLR_EL1_TCF0_ASYNC;
> > -             break;
> > -     default:
> > -             return -EINVAL;
> > -     }
> > +     if (arg & PR_MTE_TCF_ASYNC)
> > +             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
> > +     if (arg & PR_MTE_TCF_SYNC)
> > +             mte_ctrl |= MTE_CTRL_TCF_SYNC;
> >
> > -     if (task != current) {
> > -             task->thread.sctlr_user = sctlr;
> > -             task->thread.mte_ctrl = mte_ctrl;
> > -     } else {
> > -             set_task_sctlr_el1(sctlr);
> > -             set_gcr_el1_excl(mte_ctrl);
> > +     task->thread.mte_ctrl = mte_ctrl;
> > +     if (task == current) {
> > +             mte_update_sctlr_user(task);
>
> In conjunction with the next patch, what happens if we migrate at this
> point? I worry that we can install a stale sctlr_user value.
>
> > +             set_task_sctlr_el1(task->thread.sctlr_user);

In this case, we will call mte_update_sctlr_user when scheduled onto
the new CPU as a result of the change to mte_thread_switch, and both
the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
for the current CPU.

Peter
Will Deacon July 13, 2021, 5:27 p.m. UTC | #3 
On Mon, Jul 12, 2021 at 12:04:39PM -0700, Peter Collingbourne wrote:
> On Wed, Jul 7, 2021 at 4:11 AM Will Deacon <will@kernel.org> wrote:
> > On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:
> > >  long set_mte_ctrl(struct task_struct *task, unsigned long arg)
> > >  {
> > > -     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
> > >       u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
> > >                       SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
> > >
> > >       if (!system_supports_mte())
> > >               return 0;
> > >
> > > -     switch (arg & PR_MTE_TCF_MASK) {
> > > -     case PR_MTE_TCF_NONE:
> > > -             sctlr |= SCTLR_EL1_TCF0_NONE;
> > > -             break;
> > > -     case PR_MTE_TCF_SYNC:
> > > -             sctlr |= SCTLR_EL1_TCF0_SYNC;
> > > -             break;
> > > -     case PR_MTE_TCF_ASYNC:
> > > -             sctlr |= SCTLR_EL1_TCF0_ASYNC;
> > > -             break;
> > > -     default:
> > > -             return -EINVAL;
> > > -     }
> > > +     if (arg & PR_MTE_TCF_ASYNC)
> > > +             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
> > > +     if (arg & PR_MTE_TCF_SYNC)
> > > +             mte_ctrl |= MTE_CTRL_TCF_SYNC;
> > >
> > > -     if (task != current) {
> > > -             task->thread.sctlr_user = sctlr;
> > > -             task->thread.mte_ctrl = mte_ctrl;
> > > -     } else {
> > > -             set_task_sctlr_el1(sctlr);
> > > -             set_gcr_el1_excl(mte_ctrl);
> > > +     task->thread.mte_ctrl = mte_ctrl;
> > > +     if (task == current) {
> > > +             mte_update_sctlr_user(task);
> >
> > In conjunction with the next patch, what happens if we migrate at this
> > point? I worry that we can install a stale sctlr_user value.
> >
> > > +             set_task_sctlr_el1(task->thread.sctlr_user);
> 
> In this case, we will call mte_update_sctlr_user when scheduled onto
> the new CPU as a result of the change to mte_thread_switch, and both
> the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
> for the current CPU.

Doesn't that rely on task->thread.sctlr_user being explicitly read on the
new CPU? For example, the following rough sequence is what I'm worried
about:


CPU x (prefer ASYNC)
set_mte_ctrl(ASYNC | SYNC)
	current->thread.mte_ctrl = ASYNC | SYNC;
	mte_update_sctlr_user
		current->thread.sctlr_user = ASYNC;
	Register Xn = current->thread.sctlr_user; // ASYNC
	<migration to CPU y>

CPU y (prefer SYNC)
mte_thread_switch
	mte_update_sctlr_user
		next->thread.sctlr_user = SYNC;
	update_sctlr_el1
		SCTLR_EL1 = SYNC;

	<resume next back in set_mte_ctrl>
	set_task_sctlr_el1(Xn); // ASYNC
	current->thread.sctlr_user = Xn; // ASYNC  XXX: also superfluous?
	SCTLR_EL1 = ASYNC;


Does that make sense?

I'm thinking set_mte_ctrl() should be using update_sctlr_el1() and disabling
preemption around the whole thing, which would make it a lot closer to the
context-switch path.

Will
Peter Collingbourne July 13, 2021, 10:52 p.m. UTC | #4 
On Tue, Jul 13, 2021 at 10:27 AM Will Deacon <will@kernel.org> wrote:
>
> On Mon, Jul 12, 2021 at 12:04:39PM -0700, Peter Collingbourne wrote:
> > On Wed, Jul 7, 2021 at 4:11 AM Will Deacon <will@kernel.org> wrote:
> > > On Fri, Jul 02, 2021 at 12:41:08PM -0700, Peter Collingbourne wrote:
> > > >  long set_mte_ctrl(struct task_struct *task, unsigned long arg)
> > > >  {
> > > > -     u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
> > > >       u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
> > > >                       SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
> > > >
> > > >       if (!system_supports_mte())
> > > >               return 0;
> > > >
> > > > -     switch (arg & PR_MTE_TCF_MASK) {
> > > > -     case PR_MTE_TCF_NONE:
> > > > -             sctlr |= SCTLR_EL1_TCF0_NONE;
> > > > -             break;
> > > > -     case PR_MTE_TCF_SYNC:
> > > > -             sctlr |= SCTLR_EL1_TCF0_SYNC;
> > > > -             break;
> > > > -     case PR_MTE_TCF_ASYNC:
> > > > -             sctlr |= SCTLR_EL1_TCF0_ASYNC;
> > > > -             break;
> > > > -     default:
> > > > -             return -EINVAL;
> > > > -     }
> > > > +     if (arg & PR_MTE_TCF_ASYNC)
> > > > +             mte_ctrl |= MTE_CTRL_TCF_ASYNC;
> > > > +     if (arg & PR_MTE_TCF_SYNC)
> > > > +             mte_ctrl |= MTE_CTRL_TCF_SYNC;
> > > >
> > > > -     if (task != current) {
> > > > -             task->thread.sctlr_user = sctlr;
> > > > -             task->thread.mte_ctrl = mte_ctrl;
> > > > -     } else {
> > > > -             set_task_sctlr_el1(sctlr);
> > > > -             set_gcr_el1_excl(mte_ctrl);
> > > > +     task->thread.mte_ctrl = mte_ctrl;
> > > > +     if (task == current) {
> > > > +             mte_update_sctlr_user(task);
> > >
> > > In conjunction with the next patch, what happens if we migrate at this
> > > point? I worry that we can install a stale sctlr_user value.
> > >
> > > > +             set_task_sctlr_el1(task->thread.sctlr_user);
> >
> > In this case, we will call mte_update_sctlr_user when scheduled onto
> > the new CPU as a result of the change to mte_thread_switch, and both
> > the scheduler and prctl will set SCTLR_EL1 to the new (correct) value
> > for the current CPU.
>
> Doesn't that rely on task->thread.sctlr_user being explicitly read on the
> new CPU? For example, the following rough sequence is what I'm worried
> about:
>
>
> CPU x (prefer ASYNC)
> set_mte_ctrl(ASYNC | SYNC)
>         current->thread.mte_ctrl = ASYNC | SYNC;
>         mte_update_sctlr_user
>                 current->thread.sctlr_user = ASYNC;
>         Register Xn = current->thread.sctlr_user; // ASYNC
>         <migration to CPU y>
>
> CPU y (prefer SYNC)
> mte_thread_switch
>         mte_update_sctlr_user
>                 next->thread.sctlr_user = SYNC;
>         update_sctlr_el1
>                 SCTLR_EL1 = SYNC;
>
>         <resume next back in set_mte_ctrl>
>         set_task_sctlr_el1(Xn); // ASYNC
>         current->thread.sctlr_user = Xn; // ASYNC  XXX: also superfluous?
>         SCTLR_EL1 = ASYNC;
>
>
> Does that make sense?
>
> I'm thinking set_mte_ctrl() should be using update_sctlr_el1() and disabling
> preemption around the whole thing, which would make it a lot closer to the
> context-switch path.

Okay, I see what you mean. I also noticed that
prctl(PR_PAC_SET_ENABLED_KEYS) would now have the same problem. In v10
I've addressed this issue by inserting a patch after this one that
disables preemption in both prctl implementations.

Peter
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
index 6322fb1714d5..80ceb9cbdd60 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -19,6 +19,9 @@ 
 #define MTE_CTRL_GCR_USER_EXCL_SHIFT	0
 #define MTE_CTRL_GCR_USER_EXCL_MASK	0xffff
 
+#define MTE_CTRL_TCF_SYNC		(1UL << 16)
+#define MTE_CTRL_TCF_ASYNC		(1UL << 17)
+
 #ifndef __ASSEMBLY__
 
 #include <linux/build_bug.h>
diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index d3884d09513d..53d89915029d 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -197,14 +197,19 @@  static void update_gcr_el1_excl(u64 excl)
 	sysreg_clear_set_s(SYS_GCR_EL1, SYS_GCR_EL1_EXCL_MASK, excl);
 }
 
-static void set_gcr_el1_excl(u64 excl)
+static void mte_update_sctlr_user(struct task_struct *task)
 {
-	current->thread.mte_ctrl = excl;
+	unsigned long sctlr = task->thread.sctlr_user;
+	unsigned long pref = MTE_CTRL_TCF_ASYNC;
+	unsigned long mte_ctrl = task->thread.mte_ctrl;
+	unsigned long resolved_mte_tcf = (mte_ctrl & pref) ? pref : mte_ctrl;
 
-	/*
-	 * SYS_GCR_EL1 will be set to current->thread.gcr_user_excl value
-	 * by mte_set_user_gcr() in kernel_exit,
-	 */
+	sctlr &= ~SCTLR_EL1_TCF0_MASK;
+	if (resolved_mte_tcf & MTE_CTRL_TCF_ASYNC)
+		sctlr |= SCTLR_EL1_TCF0_ASYNC;
+	else if (resolved_mte_tcf & MTE_CTRL_TCF_SYNC)
+		sctlr |= SCTLR_EL1_TCF0_SYNC;
+	task->thread.sctlr_user = sctlr;
 }
 
 void mte_thread_init_user(void)
@@ -216,15 +221,16 @@  void mte_thread_init_user(void)
 	dsb(ish);
 	write_sysreg_s(0, SYS_TFSRE0_EL1);
 	clear_thread_flag(TIF_MTE_ASYNC_FAULT);
-	/* disable tag checking */
-	set_task_sctlr_el1((current->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK) |
-			   SCTLR_EL1_TCF0_NONE);
-	/* reset tag generation mask */
-	set_gcr_el1_excl(SYS_GCR_EL1_EXCL_MASK);
+	/* disable tag checking and reset tag generation mask */
+	current->thread.mte_ctrl = MTE_CTRL_GCR_USER_EXCL_MASK;
+	mte_update_sctlr_user(current);
+	set_task_sctlr_el1(current->thread.sctlr_user);
 }
 
 void mte_thread_switch(struct task_struct *next)
 {
+	mte_update_sctlr_user(next);
+
 	/*
 	 * Check if an async tag exception occurred at EL1.
 	 *
@@ -262,33 +268,21 @@  void mte_suspend_exit(void)
 
 long set_mte_ctrl(struct task_struct *task, unsigned long arg)
 {
-	u64 sctlr = task->thread.sctlr_user & ~SCTLR_EL1_TCF0_MASK;
 	u64 mte_ctrl = (~((arg & PR_MTE_TAG_MASK) >> PR_MTE_TAG_SHIFT) &
 			SYS_GCR_EL1_EXCL_MASK) << MTE_CTRL_GCR_USER_EXCL_SHIFT;
 
 	if (!system_supports_mte())
 		return 0;
 
-	switch (arg & PR_MTE_TCF_MASK) {
-	case PR_MTE_TCF_NONE:
-		sctlr |= SCTLR_EL1_TCF0_NONE;
-		break;
-	case PR_MTE_TCF_SYNC:
-		sctlr |= SCTLR_EL1_TCF0_SYNC;
-		break;
-	case PR_MTE_TCF_ASYNC:
-		sctlr |= SCTLR_EL1_TCF0_ASYNC;
-		break;
-	default:
-		return -EINVAL;
-	}
+	if (arg & PR_MTE_TCF_ASYNC)
+		mte_ctrl |= MTE_CTRL_TCF_ASYNC;
+	if (arg & PR_MTE_TCF_SYNC)
+		mte_ctrl |= MTE_CTRL_TCF_SYNC;
 
-	if (task != current) {
-		task->thread.sctlr_user = sctlr;
-		task->thread.mte_ctrl = mte_ctrl;
-	} else {
-		set_task_sctlr_el1(sctlr);
-		set_gcr_el1_excl(mte_ctrl);
+	task->thread.mte_ctrl = mte_ctrl;
+	if (task == current) {
+		mte_update_sctlr_user(task);
+		set_task_sctlr_el1(task->thread.sctlr_user);
 	}
 
 	return 0;
@@ -305,18 +299,10 @@  long get_mte_ctrl(struct task_struct *task)
 		return 0;
 
 	ret = incl << PR_MTE_TAG_SHIFT;
-
-	switch (task->thread.sctlr_user & SCTLR_EL1_TCF0_MASK) {
-	case SCTLR_EL1_TCF0_NONE:
-		ret |= PR_MTE_TCF_NONE;
-		break;
-	case SCTLR_EL1_TCF0_SYNC:
-		ret |= PR_MTE_TCF_SYNC;
-		break;
-	case SCTLR_EL1_TCF0_ASYNC:
+	if (mte_ctrl & MTE_CTRL_TCF_ASYNC)
 		ret |= PR_MTE_TCF_ASYNC;
-		break;
-	}
+	if (mte_ctrl & MTE_CTRL_TCF_SYNC)
+		ret |= PR_MTE_TCF_SYNC;
 
 	return ret;
 }
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index 18a9f59dc067..d3a5afb4c1ae 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -234,14 +234,15 @@  struct prctl_mm_map {
 #define PR_GET_TAGGED_ADDR_CTRL		56
 # define PR_TAGGED_ADDR_ENABLE		(1UL << 0)
 /* MTE tag check fault modes */
-# define PR_MTE_TCF_SHIFT		1
-# define PR_MTE_TCF_NONE		(0UL << PR_MTE_TCF_SHIFT)
-# define PR_MTE_TCF_SYNC		(1UL << PR_MTE_TCF_SHIFT)
-# define PR_MTE_TCF_ASYNC		(2UL << PR_MTE_TCF_SHIFT)
-# define PR_MTE_TCF_MASK		(3UL << PR_MTE_TCF_SHIFT)
+# define PR_MTE_TCF_NONE		0
+# define PR_MTE_TCF_SYNC		(1UL << 1)
+# define PR_MTE_TCF_ASYNC		(1UL << 2)
+# define PR_MTE_TCF_MASK		(PR_MTE_TCF_SYNC | PR_MTE_TCF_ASYNC)
 /* MTE tag inclusion mask */
 # define PR_MTE_TAG_SHIFT		3
 # define PR_MTE_TAG_MASK		(0xffffUL << PR_MTE_TAG_SHIFT)
+/* Unused; kept only for source compatibility */
+# define PR_MTE_TCF_SHIFT		1
 
 /* Control reclaim behavior when allocating memory */
 #define PR_SET_IO_FLUSHER		57