[PATCH] markers: bit-field is not thread-safe nor smp-safe
Lai Jiangshan
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.
see also: http://en.wikipedia.org/wiki/Bit_field
Signed-off-by: Lai Jiangshan <la...@cn.fujitsu.com>
---
diff --git a/kernel/marker.c b/kernel/marker.c
index 7d1faec..4777218 100644
--- a/kernel/marker.c
+++ b/kernel/marker.c
@@ -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' */
};
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
KOSAKI Motohiro
>
> 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 ;)
Lai Jiangshan
> Hi Lai-san,
>
>> 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 ;)
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
Mathieu Desnoyers
>
> 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
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Lai Jiangshan
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
Signed-off-by: Lai Jiangshan <la...@cn.fujitsu.com>
---
diff --git a/kernel/marker.c b/kernel/marker.c
index 7d1faec..95c62da 100644
--- a/kernel/marker.c
+++ b/kernel/marker.c
@@ -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' */
};
--
Mathieu Desnoyers
>
> 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 !
Acked-by: Mathieu Desnoyers <mathieu....@polymtl.ca>
--
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
Ingo Molnar
* Mathieu Desnoyers <mathieu....@polymtl.ca> wrote:
> * Lai Jiangshan (la...@cn.fujitsu.com) 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 !
>
> Acked-by: Mathieu Desnoyers <mathieu....@polymtl.ca>
applied to tip/tracing/markers, thanks guys!
Ingo