| Message ID | 20231212115301.7a9c9a64@gandalf.local.home (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fff88fa0fbc7067ba46dde570912d63da42c59a9 |
| Headers | show |
| Series | ring-buffer: Fix a race in rb_time_cmpxchg() for 32 bit archs | expand |
On 2023-12-12 11:53, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > Mathieu Desnoyers pointed out an issue in the rb_time_cmpxchg() for 32 bit > architectures. That is: > > static bool rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set) > { > unsigned long cnt, top, bottom, msb; > unsigned long cnt2, top2, bottom2, msb2; > u64 val; > > /* The cmpxchg always fails if it interrupted an update */ > if (!__rb_time_read(t, &val, &cnt2)) > return false; > > if (val != expect) > return false; > > <<<< interrupted here! > > cnt = local_read(&t->cnt); > > The problem is that the synchronization counter in the rb_time_t is read > *after* the value of the timestamp is read. That means if an interrupt > were to come in between the value being read and the counter being read, > it can change the value and the counter and the interrupted process would > be clueless about it! > > The counter needs to be read first and then the value. That way it is easy > to tell if the value is stale or not. If the counter hasn't been updated, > then the value is still good. > > Link: https://lore.kernel.org/linux-trace-kernel/20231211201324.652870-1-mathieu.desnoyers@efficios.com/ > > Cc: stable@vger.kernel.org > Fixes: 10464b4aa605e ("ring-buffer: Add rb_time_t 64 bit operations for speeding up 32 bit") > Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > --- > kernel/trace/ring_buffer.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 1d9caee7f542..e110cde685ea 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -706,6 +706,9 @@ static bool rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set) > unsigned long cnt2, top2, bottom2, msb2; > u64 val; > > + /* Any interruptions in this function should cause a failure */ > + cnt = local_read(&t->cnt); > + > /* The cmpxchg always fails if it interrupted an update */ > if (!__rb_time_read(t, &val, &cnt2)) > return false; > @@ -713,7 +716,6 @@ static bool rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set) > if (val != expect) > return false; > > - cnt = local_read(&t->cnt); > if ((cnt & 3) != cnt2) > return false; >
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c index 1d9caee7f542..e110cde685ea 100644 --- a/kernel/trace/ring_buffer.c +++ b/kernel/trace/ring_buffer.c @@ -706,6 +706,9 @@ static bool rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set) unsigned long cnt2, top2, bottom2, msb2; u64 val; + /* Any interruptions in this function should cause a failure */ + cnt = local_read(&t->cnt); + /* The cmpxchg always fails if it interrupted an update */ if (!__rb_time_read(t, &val, &cnt2)) return false; @@ -713,7 +716,6 @@ static bool rb_time_cmpxchg(rb_time_t *t, u64 expect, u64 set) if (val != expect) return false; - cnt = local_read(&t->cnt); if ((cnt & 3) != cnt2) return false;