[RFC PATCH 00/10] thread_info: use helpers to snapshot thread flags

[RFC PATCH 00/10] thread_info: use helpers to snapshot thread flags

[Mark Rutland]

As thread_info::flags scan be manipulated by remote threads, it is necessary to
use atomics or READ_ONCE() to ensure that code manipulates a consistent
snapshot, but we open-code plain accesses to thread_info::flags across the kernel tree.

Generally we get away with this, but tools like KCSAN legitimately warn that
there is a data-race, and this is potentially fragile with compiler
optimizations, LTO, etc.

These patches introduce new helpers to snahpshot the thread flags, with the
intent being that these should replace all plain accesses.

I'm sending this as an RFC as I'm certain I've missed places that need to be
moved over to the helpers, and I want to check that this has the right shape
before digging deeper.

Thanks,
Mark.

Mark Rutland (10):
  thread_info: add helpers to snapshot thread flags
  entry: snapshot thread flags
  sched: snapshot thread flags
  alpha: snapshot thread flags
  arm: snapshot thread flags
  arm64: read thread flags
  microblaze: snapshot thread flags
  openrisc: snapshot thread flags
  powerpc: snapshot thread flags
  x86: snapshot thread flags

 arch/alpha/kernel/signal.c          |  2 +-
 arch/arm/kernel/signal.c            |  2 +-
 arch/arm/mm/alignment.c             |  2 +-
 arch/arm64/kernel/ptrace.c          |  4 ++--
 arch/arm64/kernel/signal.c          |  2 +-
 arch/arm64/kernel/syscall.c         |  4 ++--
 arch/microblaze/kernel/signal.c     |  2 +-
 arch/openrisc/kernel/signal.c       |  2 +-
 arch/powerpc/kernel/interrupt.c     | 16 ++++++++--------
 arch/powerpc/kernel/ptrace/ptrace.c |  3 +--
 arch/x86/kernel/process.c           |  8 ++++----
 arch/x86/kernel/process.h           |  6 +++---
 arch/x86/mm/tlb.c                   |  2 +-
 include/linux/entry-kvm.h           |  2 +-
 include/linux/thread_info.h         | 10 ++++++++++
 kernel/entry/common.c               |  4 ++--
 kernel/entry/kvm.c                  |  4 ++--
 kernel/sched/core.c                 |  2 +-
 18 files changed, 43 insertions(+), 34 deletions(-)

-- 
2.11.0

[RFC PATCH 01/10] thread_info: add helpers to snapshot thread flags

[Mark Rutland]

We have common helpers to manipulate individual thread flags, but where
code wants to check several flags at once, it must open code reading
current_thread_info()->flags and operating on a snapshot.

As some flags can be set remotely it's necessary to use READ_ONCE() to
get a consistent snapshot even when IRQs are disabled, but some code
forgets to do this. Generally this is unlike to cause a problem in
practice, but it is somewhat unsound, and KCSAN will legitimately warn
that there is a data race.

To make it easier to do the right thing, and to highlight that
concurrent modification is possible, let's add a new helpers to snapshot
the flags, which should be used in preference to plain reads.
Subsequent patches will move existing code to use the new helpers.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Marco Elver <elver@google.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
---
 include/linux/thread_info.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 157762db9d4b..f3769842046d 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -117,6 +117,11 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 	return test_bit(flag, (unsigned long *)&ti->flags);
 }
 
+static inline unsigned long read_ti_thread_flags(struct thread_info *ti)
+{
+	return READ_ONCE(ti->flags);
+}
+
 #define set_thread_flag(flag) \
 	set_ti_thread_flag(current_thread_info(), flag)
 #define clear_thread_flag(flag) \
@@ -129,6 +134,11 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 	test_and_clear_ti_thread_flag(current_thread_info(), flag)
 #define test_thread_flag(flag) \
 	test_ti_thread_flag(current_thread_info(), flag)
+#define read_thread_flags() \
+	read_ti_thread_flags(current_thread_info())
+
+#define read_task_thread_flags(t) \
+	read_ti_thread_flags(task_thread_info(t))
 
 #ifdef CONFIG_GENERIC_ENTRY
 #define set_syscall_work(fl) \
-- 
2.11.0

Re: [RFC PATCH 01/10] thread_info: add helpers to snapshot thread flags

[Marco Elver]

On Wed, 9 Jun 2021 at 14:20, Mark Rutland <mark.rutland@arm.com> wrote:
>
> We have common helpers to manipulate individual thread flags, but where
> code wants to check several flags at once, it must open code reading
> current_thread_info()->flags and operating on a snapshot.
>
> As some flags can be set remotely it's necessary to use READ_ONCE() to
> get a consistent snapshot even when IRQs are disabled, but some code
> forgets to do this. Generally this is unlike to cause a problem in
> practice, but it is somewhat unsound, and KCSAN will legitimately warn
> that there is a data race.
>
> To make it easier to do the right thing, and to highlight that
> concurrent modification is possible, let's add a new helpers to snapshot
> the flags, which should be used in preference to plain reads.
> Subsequent patches will move existing code to use the new helpers.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Marco Elver <elver@google.com>
> Cc: Paul E. McKenney <paulmck@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>

Acked-by: Marco Elver <elver@google.com>


> ---
>  include/linux/thread_info.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 157762db9d4b..f3769842046d 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -117,6 +117,11 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
>         return test_bit(flag, (unsigned long *)&ti->flags);
>  }
>
> +static inline unsigned long read_ti_thread_flags(struct thread_info *ti)
> +{
> +       return READ_ONCE(ti->flags);
> +}
> +

Are some of the callers 'noinstr'? I haven't seen it in this series
yet, but if yes, then not inlining (which some compilers may do with
heavier instrumentation) might cause issues and this could be
__always_inline.

Thanks,
-- Marco

Re: [RFC PATCH 01/10] thread_info: add helpers to snapshot thread flags

[Mark Rutland]

On Thu, Jun 10, 2021 at 11:01:34AM +0200, Marco Elver wrote:
> On Wed, 9 Jun 2021 at 14:20, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > We have common helpers to manipulate individual thread flags, but where
> > code wants to check several flags at once, it must open code reading
> > current_thread_info()->flags and operating on a snapshot.
> >
> > As some flags can be set remotely it's necessary to use READ_ONCE() to
> > get a consistent snapshot even when IRQs are disabled, but some code
> > forgets to do this. Generally this is unlike to cause a problem in
> > practice, but it is somewhat unsound, and KCSAN will legitimately warn
> > that there is a data race.
> >
> > To make it easier to do the right thing, and to highlight that
> > concurrent modification is possible, let's add a new helpers to snapshot
> > the flags, which should be used in preference to plain reads.
> > Subsequent patches will move existing code to use the new helpers.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Boqun Feng <boqun.feng@gmail.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Paul E. McKenney <paulmck@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Will Deacon <will@kernel.org>
> 
> Acked-by: Marco Elver <elver@google.com>

Thanks!

> > ---
> >  include/linux/thread_info.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> > index 157762db9d4b..f3769842046d 100644
> > --- a/include/linux/thread_info.h
> > +++ b/include/linux/thread_info.h
> > @@ -117,6 +117,11 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
> >         return test_bit(flag, (unsigned long *)&ti->flags);
> >  }
> >
> > +static inline unsigned long read_ti_thread_flags(struct thread_info *ti)
> > +{
> > +       return READ_ONCE(ti->flags);
> > +}
> > +
> 
> Are some of the callers 'noinstr'? I haven't seen it in this series
> yet, but if yes, then not inlining (which some compilers may do with
> heavier instrumentation) might cause issues and this could be
> __always_inline.

That's a very good point; I agree it should be __always_inline, and I'll
fix that up for the next spin.

Thanks,
Mark.

Re: [RFC PATCH 01/10] thread_info: add helpers to snapshot thread flags

[Thomas Gleixner]

On Wed, Jun 09 2021 at 13:19, Mark Rutland wrote:
> We have common helpers to manipulate individual thread flags, but where
> code wants to check several flags at once, it must open code reading
> current_thread_info()->flags and operating on a snapshot.

Who's we?

> As some flags can be set remotely it's necessary to use READ_ONCE() to
> get a consistent snapshot even when IRQs are disabled, but some code
> forgets to do this. Generally this is unlike to cause a problem in
> practice, but it is somewhat unsound, and KCSAN will legitimately warn
> that there is a data race.
>
> To make it easier to do the right thing, and to highlight that
> concurrent modification is possible, let's add a new helpers to
> snapshot

let's add? Why not simply "add"?

> +static inline unsigned long read_ti_thread_flags(struct thread_info *ti)

__always_inline() as Marco pointed out already

Other than those nitpicks:

 Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [RFC PATCH 01/10] thread_info: add helpers to snapshot thread flags

[Mark Rutland]

On Sun, Jun 20, 2021 at 12:28:04AM +0200, Thomas Gleixner wrote:
> On Wed, Jun 09 2021 at 13:19, Mark Rutland wrote:
> > We have common helpers to manipulate individual thread flags, but where
> > code wants to check several flags at once, it must open code reading
> > current_thread_info()->flags and operating on a snapshot.
> 
> Who's we?

Locally I've changed that to:

| In <linux/thread_info.h> there are helpers to [...]

Please shout if there's a better way of wording that!

> > As some flags can be set remotely it's necessary to use READ_ONCE() to
> > get a consistent snapshot even when IRQs are disabled, but some code
> > forgets to do this. Generally this is unlike to cause a problem in
> > practice, but it is somewhat unsound, and KCSAN will legitimately warn
> > that there is a data race.
> >
> > To make it easier to do the right thing, and to highlight that
> > concurrent modification is possible, let's add a new helpers to
> > snapshot
> 
> let's add? Why not simply "add"?

Done.

> > +static inline unsigned long read_ti_thread_flags(struct thread_info *ti)
> 
> __always_inline() as Marco pointed out already
> 
> Other than those nitpicks:
> 
>  Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks!

Mark.

[RFC PATCH 02/10] entry: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so in the common entry code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/entry-kvm.h | 2 +-
 kernel/entry/common.c     | 4 ++--
 kernel/entry/kvm.c        | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/entry-kvm.h b/include/linux/entry-kvm.h
index 8b2b1d68b954..bc487dffb803 100644
--- a/include/linux/entry-kvm.h
+++ b/include/linux/entry-kvm.h
@@ -70,7 +70,7 @@ static inline void xfer_to_guest_mode_prepare(void)
  */
 static inline bool __xfer_to_guest_mode_work_pending(void)
 {
-	unsigned long ti_work = READ_ONCE(current_thread_info()->flags);
+	unsigned long ti_work = read_thread_flags();
 
 	return !!(ti_work & XFER_TO_GUEST_MODE_WORK);
 }
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index a0b3b04fb596..3147a1f2ed74 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -188,7 +188,7 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 		/* Check if any of the above work has queued a deferred wakeup */
 		rcu_nocb_flush_deferred_wakeup();
 
-		ti_work = READ_ONCE(current_thread_info()->flags);
+		ti_work = read_thread_flags();
 	}
 
 	/* Return the latest work state for arch_exit_to_user_mode() */
@@ -197,7 +197,7 @@ static unsigned long exit_to_user_mode_loop(struct pt_regs *regs,
 
 static void exit_to_user_mode_prepare(struct pt_regs *regs)
 {
-	unsigned long ti_work = READ_ONCE(current_thread_info()->flags);
+	unsigned long ti_work = read_thread_flags();
 
 	lockdep_assert_irqs_disabled();
 
diff --git a/kernel/entry/kvm.c b/kernel/entry/kvm.c
index 49972ee99aff..96d476e06c77 100644
--- a/kernel/entry/kvm.c
+++ b/kernel/entry/kvm.c
@@ -26,7 +26,7 @@ static int xfer_to_guest_mode_work(struct kvm_vcpu *vcpu, unsigned long ti_work)
 		if (ret)
 			return ret;
 
-		ti_work = READ_ONCE(current_thread_info()->flags);
+		ti_work = read_thread_flags();
 	} while (ti_work & XFER_TO_GUEST_MODE_WORK || need_resched());
 	return 0;
 }
@@ -43,7 +43,7 @@ int xfer_to_guest_mode_handle_work(struct kvm_vcpu *vcpu)
 	 * disabled in the inner loop before going into guest mode. No need
 	 * to disable interrupts here.
 	 */
-	ti_work = READ_ONCE(current_thread_info()->flags);
+	ti_work = read_thread_flags();
 	if (!(ti_work & XFER_TO_GUEST_MODE_WORK))
 		return 0;
 
-- 
2.11.0

[RFC PATCH 03/10] sched: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so in the common sched code.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5226cc26a095..7183c2f8abac 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7364,7 +7364,7 @@ void sched_show_task(struct task_struct *p)
 	rcu_read_unlock();
 	pr_cont(" stack:%5lu pid:%5d ppid:%6d flags:0x%08lx\n",
 		free, task_pid_nr(p), ppid,
-		(unsigned long)task_thread_info(p)->flags);
+		read_task_thread_flags(p));
 
 	print_worker_info(KERN_INFO, p);
 	print_stop_info(KERN_INFO, p);
-- 
2.11.0

[RFC PATCH 04/10] alpha: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on alpha.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Richard Henderson <rth@twiddle.net>
---
 arch/alpha/kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/alpha/kernel/signal.c b/arch/alpha/kernel/signal.c
index 948b89789da8..b1b7623ea67e 100644
--- a/arch/alpha/kernel/signal.c
+++ b/arch/alpha/kernel/signal.c
@@ -535,6 +535,6 @@ do_work_pending(struct pt_regs *regs, unsigned long thread_flags,
 			}
 		}
 		local_irq_disable();
-		thread_flags = current_thread_info()->flags;
+		thread_flags = read_thread_flags();
 	} while (thread_flags & _TIF_WORK_MASK);
 }
-- 
2.11.0

[RFC PATCH 05/10] arm: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on arm.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
---
 arch/arm/kernel/signal.c | 2 +-
 arch/arm/mm/alignment.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index a3a38d0a4c85..40c1178bd8c2 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -674,7 +674,7 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 			}
 		}
 		local_irq_disable();
-		thread_flags = current_thread_info()->flags;
+		thread_flags = read_thread_flags();
 	} while (thread_flags & _TIF_WORK_MASK);
 	return 0;
 }
diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index ea81e89e7740..adbb3817d0be 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -990,7 +990,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		 * there is no work pending for this thread.
 		 */
 		raw_local_irq_disable();
-		if (!(current_thread_info()->flags & _TIF_WORK_MASK))
+		if (!(read_thread_flags() & _TIF_WORK_MASK))
 			set_cr(cr_no_alignment);
 	}
 
-- 
2.11.0

[RFC PATCH 06/10] arm64: read thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on arm64.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
---
 arch/arm64/kernel/ptrace.c  | 4 ++--
 arch/arm64/kernel/signal.c  | 2 +-
 arch/arm64/kernel/syscall.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index eb2f73939b7b..a6103b997664 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -1834,7 +1834,7 @@ static void tracehook_report_syscall(struct pt_regs *regs,
 
 int syscall_trace_enter(struct pt_regs *regs)
 {
-	unsigned long flags = READ_ONCE(current_thread_info()->flags);
+	unsigned long flags = read_thread_flags();
 
 	if (flags & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE)) {
 		tracehook_report_syscall(regs, PTRACE_SYSCALL_ENTER);
@@ -1857,7 +1857,7 @@ int syscall_trace_enter(struct pt_regs *regs)
 
 void syscall_trace_exit(struct pt_regs *regs)
 {
-	unsigned long flags = READ_ONCE(current_thread_info()->flags);
+	unsigned long flags = read_thread_flags();
 
 	audit_syscall_exit(regs);
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 6237486ff6bb..ed113659425d 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -945,7 +945,7 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
 		}
 
 		local_daif_mask();
-		thread_flags = READ_ONCE(current_thread_info()->flags);
+		thread_flags = read_thead_flags();
 	} while (thread_flags & _TIF_WORK_MASK);
 }
 
diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
index 263d6c1a525f..badf1789dc3d 100644
--- a/arch/arm64/kernel/syscall.c
+++ b/arch/arm64/kernel/syscall.c
@@ -84,7 +84,7 @@ void syscall_trace_exit(struct pt_regs *regs);
 static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
 			   const syscall_fn_t syscall_table[])
 {
-	unsigned long flags = current_thread_info()->flags;
+	unsigned long flags = read_thread_flags();
 
 	regs->orig_x0 = regs->regs[0];
 	regs->syscallno = scno;
@@ -151,7 +151,7 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
 	 */
 	if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
 		local_daif_mask();
-		flags = current_thread_info()->flags;
+		flags = read_thread_flags();
 		if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP))
 			return;
 		local_daif_restore(DAIF_PROCCTX);
-- 
2.11.0

[RFC PATCH 07/10] microblaze: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on microblaze.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Michal Simek <monstr@monstr.eu>
---
 arch/microblaze/kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/microblaze/kernel/signal.c b/arch/microblaze/kernel/signal.c
index fc61eb0eb8dd..23e8a9336a29 100644
--- a/arch/microblaze/kernel/signal.c
+++ b/arch/microblaze/kernel/signal.c
@@ -283,7 +283,7 @@ static void do_signal(struct pt_regs *regs, int in_syscall)
 #ifdef DEBUG_SIG
 	pr_info("do signal: %p %d\n", regs, in_syscall);
 	pr_info("do signal2: %lx %lx %ld [%lx]\n", regs->pc, regs->r1,
-			regs->r12, current_thread_info()->flags);
+			regs->r12, read_thread_flags());
 #endif
 
 	if (get_signal(&ksig)) {
-- 
2.11.0

[RFC PATCH 08/10] openrisc: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on openrisc.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Stafford Horne <shorne@gmail.com>
Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
---
 arch/openrisc/kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
index 1ebcff271096..a730a914c2b4 100644
--- a/arch/openrisc/kernel/signal.c
+++ b/arch/openrisc/kernel/signal.c
@@ -315,7 +315,7 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
 			}
 		}
 		local_irq_disable();
-		thread_flags = current_thread_info()->flags;
+		thread_flags = read_thread_flags();
 	} while (thread_flags & _TIF_WORK_MASK);
 	return 0;
 }
-- 
2.11.0

Re: [RFC PATCH 08/10] openrisc: snapshot thread flags

[Stafford Horne]

On Wed, Jun 09, 2021 at 01:19:59PM +0100, Mark Rutland wrote:
> Some thread flags can be set remotely, and so even when IRQs are
> disabled, the flags can change under our feet. Generally this is
> unlikely to cause a problem in practice, but it is somewhat unsound, and
> KCSAN will legitimately warn that there is a data race.
> 
> To avoid such issues, we should snapshot the flags prior to using them.
> Let's use the new helpers to do so on openrisc.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Jonas Bonn <jonas@southpole.se>
> Cc: Stafford Horne <shorne@gmail.com>
> Cc: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> ---
>  arch/openrisc/kernel/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/openrisc/kernel/signal.c b/arch/openrisc/kernel/signal.c
> index 1ebcff271096..a730a914c2b4 100644
> --- a/arch/openrisc/kernel/signal.c
> +++ b/arch/openrisc/kernel/signal.c
> @@ -315,7 +315,7 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
>  			}
>  		}
>  		local_irq_disable();
> -		thread_flags = current_thread_info()->flags;
> +		thread_flags = read_thread_flags();
>  	} while (thread_flags & _TIF_WORK_MASK);
>  	return 0;
>  }

This looks good to me.  As per patch 01/10 this adds a READ_ONCE around the
original flags read.

Acked-by: Stafford Horne <shorne@gmail.com>

[RFC PATCH 09/10] powerpc: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on powerpc.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Paul Mackerras <paulus@samba.org>
---
 arch/powerpc/kernel/interrupt.c     | 16 ++++++++--------
 arch/powerpc/kernel/ptrace/ptrace.c |  3 +--
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index e0938ba298f2..f6150a4b3d1b 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -92,7 +92,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
 
 	local_irq_enable();
 
-	if (unlikely(current_thread_info()->flags & _TIF_SYSCALL_DOTRACE)) {
+	if (unlikely(read_thread_flags() & _TIF_SYSCALL_DOTRACE)) {
 		if (unlikely(trap_is_unsupported_scv(regs))) {
 			/* Unsupported scv vector */
 			_exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
@@ -257,7 +257,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 	/* Check whether the syscall is issued inside a restartable sequence */
 	rseq_syscall(regs);
 
-	ti_flags = current_thread_info()->flags;
+	ti_flags = read_thread_flags();
 
 	if (unlikely(r3 >= (unsigned long)-MAX_ERRNO) && is_not_scv) {
 		if (likely(!(ti_flags & (_TIF_NOERROR | _TIF_RESTOREALL)))) {
@@ -284,7 +284,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 	local_irq_disable();
 
 again:
-	ti_flags = READ_ONCE(current_thread_info()->flags);
+	ti_flags = read_thread_flags();
 	while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
 		local_irq_enable();
 		if (ti_flags & _TIF_NEED_RESCHED) {
@@ -300,7 +300,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
 			do_notify_resume(regs, ti_flags);
 		}
 		local_irq_disable();
-		ti_flags = READ_ONCE(current_thread_info()->flags);
+		ti_flags = read_thread_flags();
 	}
 
 	if (IS_ENABLED(CONFIG_PPC_BOOK3S) && IS_ENABLED(CONFIG_PPC_FPU)) {
@@ -373,7 +373,7 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
 	local_irq_save(flags);
 
 again:
-	ti_flags = READ_ONCE(current_thread_info()->flags);
+	ti_flags = read_thread_flags();
 	while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
 		local_irq_enable(); /* returning to user: may enable */
 		if (ti_flags & _TIF_NEED_RESCHED) {
@@ -384,7 +384,7 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
 			do_notify_resume(regs, ti_flags);
 		}
 		local_irq_disable();
-		ti_flags = READ_ONCE(current_thread_info()->flags);
+		ti_flags = read_thread_flags();
 	}
 
 	if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && IS_ENABLED(CONFIG_PPC_FPU)) {
@@ -450,7 +450,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 
 	kuap = kuap_get_and_assert_locked();
 
-	if (unlikely(current_thread_info()->flags & _TIF_EMULATE_STACK_STORE)) {
+	if (unlikely(read_thread_flags() & _TIF_EMULATE_STACK_STORE)) {
 		clear_bits(_TIF_EMULATE_STACK_STORE, &current_thread_info()->flags);
 		ret = 1;
 	}
@@ -463,7 +463,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
 again:
 		if (IS_ENABLED(CONFIG_PREEMPT)) {
 			/* Return to preemptible kernel context */
-			if (unlikely(current_thread_info()->flags & _TIF_NEED_RESCHED)) {
+			if (unlikely(read_thread_flags() & _TIF_NEED_RESCHED)) {
 				if (preempt_count() == 0)
 					preempt_schedule_irq();
 			}
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index 0a0a33eb0d28..d174570a144e 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -260,8 +260,7 @@ long do_syscall_trace_enter(struct pt_regs *regs)
 {
 	u32 flags;
 
-	flags = READ_ONCE(current_thread_info()->flags) &
-		(_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
+	flags = read_thread_flags() & (_TIF_SYSCALL_EMU | _TIF_SYSCALL_TRACE);
 
 	if (flags) {
 		int rc = tracehook_report_syscall_entry(regs);
-- 
2.11.0

Re: [RFC PATCH 09/10] powerpc: snapshot thread flags

[Michael Ellerman]

Mark Rutland <mark.rutland@arm.com> writes:
> Some thread flags can be set remotely, and so even when IRQs are
> disabled, the flags can change under our feet. Generally this is
> unlikely to cause a problem in practice, but it is somewhat unsound, and
> KCSAN will legitimately warn that there is a data race.
>
> To avoid such issues, we should snapshot the flags prior to using them.
> Let's use the new helpers to do so on powerpc.
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Paul Mackerras <paulus@samba.org>
> ---
>  arch/powerpc/kernel/interrupt.c     | 16 ++++++++--------
>  arch/powerpc/kernel/ptrace/ptrace.c |  3 +--
>  2 files changed, 9 insertions(+), 10 deletions(-)

This looks good.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)


But, it clashes terribly with a series I'm planning to take for v5.14
that reworks our interrupt return code.

https://lore.kernel.org/linuxppc-dev/20210610130921.706938-1-npiggin@gmail.com/T/#t

So if you're also targeting v5.14 then it might be best to drop this
patch from the series, and we can do the conversion later.

cheers

Re: [RFC PATCH 09/10] powerpc: snapshot thread flags

[Mark Rutland]

On Tue, Jun 15, 2021 at 11:18:52PM +1000, Michael Ellerman wrote:
> Mark Rutland <mark.rutland@arm.com> writes:
> > Some thread flags can be set remotely, and so even when IRQs are
> > disabled, the flags can change under our feet. Generally this is
> > unlikely to cause a problem in practice, but it is somewhat unsound, and
> > KCSAN will legitimately warn that there is a data race.
> >
> > To avoid such issues, we should snapshot the flags prior to using them.
> > Let's use the new helpers to do so on powerpc.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Paul Mackerras <paulus@samba.org>
> > ---
> >  arch/powerpc/kernel/interrupt.c     | 16 ++++++++--------
> >  arch/powerpc/kernel/ptrace/ptrace.c |  3 +--
> >  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> This looks good.
> 
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> 
> 
> But, it clashes terribly with a series I'm planning to take for v5.14
> that reworks our interrupt return code.
> 
> https://lore.kernel.org/linuxppc-dev/20210610130921.706938-1-npiggin@gmail.com/T/#t
> 
> So if you're also targeting v5.14 then it might be best to drop this
> patch from the series, and we can do the conversion later.

I'll drop this patch for now, and note the clash in the cover letter. It
should be easy enough to respin atop those changes later, and if I don't
get this in for v5.14 I can regenerate it.

Thanks,
Mark.

[RFC PATCH 10/10] x86: snapshot thread flags

[Mark Rutland]

Some thread flags can be set remotely, and so even when IRQs are
disabled, the flags can change under our feet. Generally this is
unlikely to cause a problem in practice, but it is somewhat unsound, and
KCSAN will legitimately warn that there is a data race.

To avoid such issues, we should snapshot the flags prior to using them.
Let's use the new helpers to do so on x86.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/process.c | 8 ++++----
 arch/x86/kernel/process.h | 6 +++---
 arch/x86/mm/tlb.c         | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 5e1f38179f49..d1c4a93c6e26 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -332,7 +332,7 @@ void arch_setup_new_exec(void)
 		clear_thread_flag(TIF_SSBD);
 		task_clear_spec_ssb_disable(current);
 		task_clear_spec_ssb_noexec(current);
-		speculation_ctrl_update(task_thread_info(current)->flags);
+		speculation_ctrl_update(read_thread_flags());
 	}
 }
 
@@ -584,7 +584,7 @@ static unsigned long speculation_ctrl_update_tif(struct task_struct *tsk)
 			clear_tsk_thread_flag(tsk, TIF_SPEC_IB);
 	}
 	/* Return the updated threadinfo flags*/
-	return task_thread_info(tsk)->flags;
+	return read_task_thread_flags(tsk);
 }
 
 void speculation_ctrl_update(unsigned long tif)
@@ -620,8 +620,8 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p)
 {
 	unsigned long tifp, tifn;
 
-	tifn = READ_ONCE(task_thread_info(next_p)->flags);
-	tifp = READ_ONCE(task_thread_info(prev_p)->flags);
+	tifn = read_task_thread_flags(next_p);
+	tifp = read_task_thread_flags(prev_p);
 
 	switch_to_bitmap(tifp);
 
diff --git a/arch/x86/kernel/process.h b/arch/x86/kernel/process.h
index 1d0797b2338a..0b1be8685b49 100644
--- a/arch/x86/kernel/process.h
+++ b/arch/x86/kernel/process.h
@@ -13,9 +13,9 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p);
 static inline void switch_to_extra(struct task_struct *prev,
 				   struct task_struct *next)
 {
-	unsigned long next_tif = task_thread_info(next)->flags;
-	unsigned long prev_tif = task_thread_info(prev)->flags;
-
+	unsigned long next_tif = read_task_thread_flags(next);
+	unsigned long prev_tif = read_task_thread_flags(prev);
+	
 	if (IS_ENABLED(CONFIG_SMP)) {
 		/*
 		 * Avoid __switch_to_xtra() invocation when conditional
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 78804680e923..fcc5e22050d4 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -318,7 +318,7 @@ void switch_mm(struct mm_struct *prev, struct mm_struct *next,
 
 static unsigned long mm_mangle_tif_spec_ib(struct task_struct *next)
 {
-	unsigned long next_tif = task_thread_info(next)->flags;
+	unsigned long next_tif = read_task_thread_flags(next);
 	unsigned long ibpb = (next_tif >> TIF_SPEC_IB) & LAST_USER_MM_IBPB;
 
 	return (unsigned long)next->mm | ibpb;
-- 
2.11.0

Re: [RFC PATCH 10/10] x86: snapshot thread flags

[Thomas Gleixner]

On Wed, Jun 09 2021 at 13:20, Mark Rutland wrote:
> Some thread flags can be set remotely, and so even when IRQs are
> disabled, the flags can change under our feet. Generally this is
> unlikely to cause a problem in practice, but it is somewhat unsound, and
> KCSAN will legitimately warn that there is a data race.
>
> To avoid such issues, we should snapshot the flags prior to using them.
> Let's use the new helpers to do so on x86.

 To avoid such issues, a snapshot of the flags has to be taken prior to
 using them. Some places already use READ_ONCE() for that, others do
 not.

 Convert them all to the new flag accessor helpers.

Other than that.

 Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Re: [RFC PATCH 10/10] x86: snapshot thread flags

[Mark Rutland]

On Sun, Jun 20, 2021 at 12:30:32AM +0200, Thomas Gleixner wrote:
> On Wed, Jun 09 2021 at 13:20, Mark Rutland wrote:
> > Some thread flags can be set remotely, and so even when IRQs are
> > disabled, the flags can change under our feet. Generally this is
> > unlikely to cause a problem in practice, but it is somewhat unsound, and
> > KCSAN will legitimately warn that there is a data race.
> >
> > To avoid such issues, we should snapshot the flags prior to using them.
> > Let's use the new helpers to do so on x86.
> 
>  To avoid such issues, a snapshot of the flags has to be taken prior to
>  using them. Some places already use READ_ONCE() for that, others do
>  not.
> 
>  Convert them all to the new flag accessor helpers.

I'll use that wording consistently throughout the series.

> Other than that.
> 
>  Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks!

Mark.