markers: bit-field is not thread-safe nor smp-safe
PATCH 1/1 markers: bit-field is not thread-safe nor smp-safe
bit-field is not thread-safe nor smp-safe.
struct marker_entry.rcu_pending is not protected by any lock in rcu-callback free_old_closure().
so we must turn it into a safe type.
detail:
I suppose rcu_pending and ptype are store in struct marker_entry.tmp1
free_old_closure() side: change ptype side:
| load struct marker_entry.tmp1
--------------------------------|--------------------------------
| change ptype bit in tmp1
load struct marker_entry.tmp1 |
change rcu_pending bit in tmp1 |
store tmp1 |
--------------------------------|--------------------------------
| store tmp1now this result equals that free_old_closure() do not change rcu_pending bit, bug! This bug will cause redundant rcu_barrier_sched() called. not too harmful.
----- corresponding:
free_old_closure() side: change ptype side:
load struct marker_entry.tmp1 |
--------------------------------|--------------------------------
| load struct marker_entry.tmp1
change rcu_pending bit in tmp1 |
| change ptype bit in tmp1
| store tmp1
--------------------------------|--------------------------------
store tmp1 |now this result equals that change ptype side do not change ptype bit, bug! this bug cause marker_probe_cb() access to invalid memory. oops!
see also: http://en.wikipedia.org/wiki/Bit_field
---
kernel/marker.c +1 -1
@@ -62,7 +62,7 @@ struct marker_entry {int refcount;/* Number of times armed. 0 if disarmed. */ struct rcu_head rcu;void *oldptr; - unsigned char rcu_pending:1; +int rcu_pending; unsigned char ptype:1;char name[0 ];/* Contains name'\0'format'\0' */ };
Quoted 7 lines · * Lai Jiangshan ([email protected]) wrote:
bit-field is not thread-safe nor smp-safe.
struct marker_entry.rcu_pending is not protected by any lock in rcu-callback
free_old_closure().
so we must turn it into a safe type.
hrm, yes, you are right. I first test for
if (entry->rcu_pending)
rcu_barrier_sched();To check if I must execute the rcu callback, and _this_ races against the entry->rcu_pending = 0; within the callback.
Your fix is therefore needed.
Thanks !
Quoted 56 lines
detail:
I suppose
rcu_pendingand ptype are store in struct marker_entry.tmp1
free_old_closure()side: change ptype side:| load struct marker_entry.tmp1 --------------------------------|-------------------------------- | change ptype bit in tmp1 load struct marker_entry.tmp1 | change rcu_pending bit in tmp1 | store tmp1 | --------------------------------|-------------------------------- | store tmp1now this result equals that
free_old_closure()do not changercu_pendingbit, bug! This bug will cause redundantrcu_barrier_sched()called. not too harmful.----- corresponding:
free_old_closure()side: change ptype side:load struct marker_entry.tmp1 | --------------------------------|-------------------------------- | load struct marker_entry.tmp1 change rcu_pending bit in tmp1 | | change ptype bit in tmp1 | store tmp1 --------------------------------|-------------------------------- store tmp1 |now this result equals that change ptype side do not change ptype bit, bug! this bug cause
marker_probe_cb()access to invalid memory. oops!see also: http://en.wikipedia.org/wiki/Bit_field
Signed-off-by Lai Jiangshan <[email protected]>---
kernel/marker.c +1
-1@@ -62,7 +62,7 @@ struct marker_entry {int refcount;/* Number of times armed. 0 if disarmed. */ struct rcu_head rcu;void *oldptr; - unsigned char rcu_pending:1; +int rcu_pending; unsigned char ptype:1;char name[0 ];/* Contains name'\0'format'\0' */ };
Signature · 2 lines
Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Ingo Molnar
AppliedQuoted 15 lines · Mathieu Desnoyers
Quoted 7 lines · * Lai Jiangshan ([email protected]) wrote:
bit-field is not thread-safe nor smp-safe.
struct marker_entry.rcu_pending is not protected by any lock in rcu-callback
free_old_closure().
so we must turn it into a safe type.hrm, yes, you are right. I first test for
if (entry->rcu_pending) rcu_barrier_sched();
Quoted 8 lines
To check if I must execute the rcu callback, and _this_ races against the
entry->rcu_pending= 0; within the callback.Your fix is therefore needed.
Thanks !
applied to tip/tracing/markers, thanks guys!
Ingo