Skip to contentSkip to conversation

markers: bit-field is not thread-safe nor smp-safe

PATCH Applied Lai Jiangshan 3 participants 3 messages Oct 10, 2008

lore
Share & tools
lore
#0 lore raw

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 tmp1

now 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
index 7d1faec..95c62da 100644
@@ -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' */
 };
Acked-by on v1 1/1
#1 lore raw
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_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 tmp1

now 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
index 7d1faec..95c62da 100644
@@ -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
Applied
#2 lore raw
Quoted 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