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
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 <[email protected]>
Cc: Boqun Feng <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Will Deacon <[email protected]>
---
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
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 <[email protected]>
Cc: Ivan Kokshaysky <[email protected]>
Cc: Matt Turner <[email protected]>
Cc: Richard Henderson <[email protected]>
---
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
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 <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
---
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
On Wed, 9 Jun 2021 at 14:20, Mark Rutland <[email protected]> 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 <[email protected]>
> Cc: Boqun Feng <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Marco Elver <[email protected]>
> Cc: Paul E. McKenney <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Will Deacon <[email protected]>
Acked-by: Marco Elver <[email protected]>
> ---
> 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
On Thu, Jun 10, 2021 at 11:01:34AM +0200, Marco Elver wrote:
> On Wed, 9 Jun 2021 at 14:20, Mark Rutland <[email protected]> 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 <[email protected]>
> > Cc: Boqun Feng <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Cc: Marco Elver <[email protected]>
> > Cc: Paul E. McKenney <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Will Deacon <[email protected]>
>
> Acked-by: Marco Elver <[email protected]>
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.
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 <[email protected]>
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 <[email protected]>
Thanks!
Mark.