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.
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; +unsigned char rcu_pending; unsigned char ptype:1;char name[0 ];/* Contains name'\0'format'\0' */ };
Hi Lai-san,
Quoted 6 lines
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.
hmmm
however, char also doesn't smp-safe because some architecture doesn't have any byte load/store instruction.
It seems bogus solution to me ;)
Quoted 34 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.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; +unsigned char rcu_pending; unsigned char ptype:1;char name[0 ];/* Contains name'\0'format'\0' */ };
Quoted 8 lines · KOSAKI Motohiro
Hi Lai-san,
Quoted 5 lines
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.
Quoted 5 lines
hmmm
however, char also doesn't smp-safe because some architecture doesn't have any byte load/store instruction.It seems bogus solution to me ;)
Hi, KOSAKI-san,
Thank you very much!
char also doesn't smp-safe if the architecture doesn't have
any byte load/store instruction.
We must use int, is it right?
Lai
Quoted 42 lines
Quoted 36 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.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; +unsigned char 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.
All struct marker_entry.rcu_pending accesses are done with the markers_mutex held, except the one done in free_old_closure(). Normally, there should be a
if (entry->rcu_pending)
rcu_barrier_sched();At the beginning of each markers_mutex section (just after get_marker()) to make sure any pending callback is executed at that point before any of rcu_pending or ptype are touched.
I just noticed that the "markers: fix unchecked format" patch has a race with respect to this. I'll post a patch in a jiffy.
Mathieu
Signature · 2 lines
Mathieu Desnoyers OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68