Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1739052 > unrolled thread

[PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

Back to article view | Back to linux.kernel

Contents

  [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts Mark Rutland <mark.rutland@arm.com> - 2017-09-25 15:30 +0200
    Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t.  interrupts Tejun Heo <tj@kernel.org> - 2017-09-25 17:20 +0200
      Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t.  interrupts Mark Rutland <mark.rutland@arm.com> - 2017-09-25 17:40 +0200
        Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t.  interrupts Tejun Heo <tj@kernel.org> - 2017-09-25 17:50 +0200
          Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t.  interrupts Christopher Lameter <cl@linux.com> - 2017-09-26 08:50 +0200
            Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t.  interrupts Thomas Gleixner <tglx@linutronix.de> - 2017-09-26 09:50 +0200

#1739052 — [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

As raw_cpu_generic_read() is a plain read from a raw_cpu_ptr() address,
it's possible (albeit unlikely) that the compiler will split the access
across multiple instructions.

In this_cpu_generic_read() we disable preemption but not interrupts
before calling raw_cpu_generic_read(). Thus, an interrupt could be taken
in the middle of the split load instructions. If a this_cpu_write() or
RMW this_cpu_*() op is made to the same variable in the interrupt
handling path, this_cpu_read() will return a torn value.

Avoid this by using READ_ONCE() to inhibit tearing.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christoph Lameter <cl@linux.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Pranith Kumar <bobby.prani@gmail.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: linux-arch@vger.kernel.org
---
 include/asm-generic/percpu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/percpu.h b/include/asm-generic/percpu.h
index 0504ef8..79a8a58 100644
--- a/include/asm-generic/percpu.h
+++ b/include/asm-generic/percpu.h
@@ -67,7 +67,7 @@
 
 #define raw_cpu_generic_read(pcp)					\
 ({									\
-	*raw_cpu_ptr(&(pcp));						\
+	READ_ONCE(*raw_cpu_ptr(&(pcp)));				\
 })
 
 #define raw_cpu_generic_to_op(pcp, val, op)				\
-- 
1.9.1

[toc] | [next] | [standalone]

#1739115 — Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

Hello, Mark.

On Mon, Sep 25, 2017 at 02:24:32PM +0100, Mark Rutland wrote:
> As raw_cpu_generic_read() is a plain read from a raw_cpu_ptr() address,
> it's possible (albeit unlikely) that the compiler will split the access
> across multiple instructions.
> 
> In this_cpu_generic_read() we disable preemption but not interrupts
> before calling raw_cpu_generic_read(). Thus, an interrupt could be taken
> in the middle of the split load instructions. If a this_cpu_write() or
> RMW this_cpu_*() op is made to the same variable in the interrupt
> handling path, this_cpu_read() will return a torn value.
> 
> Avoid this by using READ_ONCE() to inhibit tearing.

That's why there are irq-safe variants of the operations.  Adding
READ_ONCE() doesn't generically guarantee that the reads won't be
split - e.g. there are arch which simply can't load a 64bit value with
a single instruction.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1739123 — Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

On Mon, Sep 25, 2017 at 08:18:27AM -0700, Tejun Heo wrote:
> Hello, Mark.
> 
> On Mon, Sep 25, 2017 at 02:24:32PM +0100, Mark Rutland wrote:
> > As raw_cpu_generic_read() is a plain read from a raw_cpu_ptr() address,
> > it's possible (albeit unlikely) that the compiler will split the access
> > across multiple instructions.
> > 
> > In this_cpu_generic_read() we disable preemption but not interrupts
> > before calling raw_cpu_generic_read(). Thus, an interrupt could be taken
> > in the middle of the split load instructions. If a this_cpu_write() or
> > RMW this_cpu_*() op is made to the same variable in the interrupt
> > handling path, this_cpu_read() will return a torn value.
> > 
> > Avoid this by using READ_ONCE() to inhibit tearing.
> 
> That's why there are irq-safe variants of the operations. 

Unfortunately, the generic this_cpu_read(), which is intended to be
irq-safe, is not:

#define this_cpu_generic_read(pcp)                                      \
({                                                                      \
        typeof(pcp) __ret;                                              \
        preempt_disable_notrace();                                      \
        __ret = raw_cpu_generic_read(pcp);                              \
        preempt_enable_notrace();                                       \
        __ret;                                                          \
})

I guess it'd be preferable to manipulate that in-place.

> Adding READ_ONCE() doesn't generically guarantee that the reads won't
> be split - e.g. there are arch which simply can't load a 64bit value
> with a single instruction.

True.

In which case, it really sounds like this_cpu_generic_read() needs to
disable interrupts too...

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1739132 — Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

Hello,

On Mon, Sep 25, 2017 at 04:33:02PM +0100, Mark Rutland wrote:
> Unfortunately, the generic this_cpu_read(), which is intended to be
> irq-safe, is not:
> 
> #define this_cpu_generic_read(pcp)                                      \
> ({                                                                      \
>         typeof(pcp) __ret;                                              \
>         preempt_disable_notrace();                                      \
>         __ret = raw_cpu_generic_read(pcp);                              \
>         preempt_enable_notrace();                                       \
>         __ret;                                                          \
> })

I see.  Yeah, that looks like the bug there.

> I guess it'd be preferable to manipulate that in-place.
> 
> > Adding READ_ONCE() doesn't generically guarantee that the reads won't
> > be split - e.g. there are arch which simply can't load a 64bit value
> > with a single instruction.
> 
> In which case, it really sounds like this_cpu_generic_read() needs to
> disable interrupts too...

Can you please spin up a patch for this?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1739559 — Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

On Mon, 25 Sep 2017, Tejun Heo wrote:

> Hello,
>
> On Mon, Sep 25, 2017 at 04:33:02PM +0100, Mark Rutland wrote:
> > Unfortunately, the generic this_cpu_read(), which is intended to be
> > irq-safe, is not:
> >
> > #define this_cpu_generic_read(pcp)                                      \
> > ({                                                                      \
> >         typeof(pcp) __ret;                                              \
> >         preempt_disable_notrace();                                      \
> >         __ret = raw_cpu_generic_read(pcp);                              \
> >         preempt_enable_notrace();                                       \
> >         __ret;                                                          \
> > })
>
> I see.  Yeah, that looks like the bug there.

This is a single fetch operation of a value that needs to be atomic. It
really does not matter if an interrupt happens before or after that load
because it could also occur before or after the preempt_enable/disable
without the code being able to distinguish that case.

The fetch of a scalar value from memory is an atomic operation and that is
required from all arches. There is an exception for double word fetches.
Maybe we would need to special code that case but so far this does not
seem to have been an issue.

[toc] | [prev] | [next] | [standalone]

#1739636 — Re: [PATCH] percpu: make this_cpu_generic_read() atomic w.r.t. interrupts

On Tue, 26 Sep 2017, Christopher Lameter wrote:
> On Mon, 25 Sep 2017, Tejun Heo wrote:
> 
> > Hello,
> >
> > On Mon, Sep 25, 2017 at 04:33:02PM +0100, Mark Rutland wrote:
> > > Unfortunately, the generic this_cpu_read(), which is intended to be
> > > irq-safe, is not:
> > >
> > > #define this_cpu_generic_read(pcp)                                      \
> > > ({                                                                      \
> > >         typeof(pcp) __ret;                                              \
> > >         preempt_disable_notrace();                                      \
> > >         __ret = raw_cpu_generic_read(pcp);                              \
> > >         preempt_enable_notrace();                                       \
> > >         __ret;                                                          \
> > > })
> >
> > I see.  Yeah, that looks like the bug there.
> 
> This is a single fetch operation of a value that needs to be atomic. It
> really does not matter if an interrupt happens before or after that load
> because it could also occur before or after the preempt_enable/disable
> without the code being able to distinguish that case.
> 
> The fetch of a scalar value from memory is an atomic operation and that is
> required from all arches. There is an exception for double word fetches.

this_cpu_read_8() is a double word fetch on many 32bit architectures.

> Maybe we would need to special code that case but so far this does not
> seem to have been an issue.

Just because nobody ran into problem with that it is a non issue? That's
just hillarious.

It's obviously not correct and needs to be fixed _before_ someone has to go
through the pain of debugging such a problem.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web