jump_label_inc may return before the code is patched

PATCH In review Gleb Natapov 3 participants 8 messages Oct 18, 2011 – Dec 6, 2011 Static branch/call

Share & tools

Gleb Natapov

author

2011-10-18 17:56 UTC #0 lore raw

PATCH 1/1 jump_label_inc may return before the code is patched

If cpu A calls jump_label_inc() just after atomic_add_return() is called by cpu B, atomic_inc_not_zero() will return value greater then zero and jump_label_inc() will return to a caller before jump_label_update() finishes its job on cpu B.

Signed-off-by Gleb Natapov <[email protected]>

kernel/jump_label.c +2 -1

index a8ce450..e6f1f24 100644
@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }

--
Gleb.

Jason Baron

Acked-by on v1 1/1

2011-10-18 20:54 UTC #1 lore raw

Quoted 11 lines · Gleb Natapov

If cpu A calls jump_label_inc() just after atomic_add_return() is called by cpu B, atomic_inc_not_zero() will return value greater then zero and jump_label_inc() will return to a caller before jump_label_update() finishes its job on cpu B.
Signed-off-by Gleb Natapov <[email protected]>
kernel/jump_label.c +0 -0
index a8ce450..e6f1f24 100644

kernel/jump_label.c void jump_label_inc(struct jump_label_key *key) · line 74

Quoted 10 lines

kernel/jump_label.c +2 -1

@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }

agreed, we shouldn't return before the update happens...did this cause any actual problem in practice? Or just observed from code inspection?

Acked-by Jason Baron <[email protected]>

Thanks,

-Jason

Gleb Natapov

author

2011-10-19 07:03 UTC #2 lore raw

Quoted 14 lines · Jason Baron

Quoted 13 lines · Gleb Natapov
If cpu A calls jump_label_inc() just after atomic_add_return() is called by cpu B, atomic_inc_not_zero() will return value greater then zero and jump_label_inc() will return to a caller before jump_label_update() finishes its job on cpu B.
Signed-off-by Gleb Natapov <[email protected]>
kernel/jump_label.c +0 -0
index a8ce450..e6f1f24 100644
@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 

Quoted 12 lines

kernel/jump_label.c void jump_label_inc(struct jump_label_key *key) · line 74
Quoted 8 lines
kernel/jump_label.c +2 -1
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }
 
agreed, we shouldn't return before the update happens...did this cause any actual problem in practice? Or just observed from code inspection?

It did. I am debugging strange oopses in perf code that happen when I create and destroy hw events on multiple cpus in parallel. At some point I started to suspect that jump labels are not working properly and printks confirmed that sometimes static_branch() is not taken when ->enabled is not zero. I compiled kernel without jump label support and, as far as my testing goes, oopses disappeared. Then I found the bug by looking at the code. Strangely enough this patch alone didn't fix oopses yet, so some other problems are probably lurking somewhere. I am going on vocation till next week. Will debug furtherer upon return.

Quoted 5 lines

Acked-by Jason Baron <[email protected]>
Thanks,
-Jason

--
Gleb.

Steven Rostedt

reviewer Acked-by on v1 1/1

2011-10-18 21:07 UTC #3 lore raw

In reply to Gleb Natapov

Quoted 4 lines · Gleb Natapov

If cpu A calls jump_label_inc() just after atomic_add_return() is called by cpu B, atomic_inc_not_zero() will return value greater then zero and jump_label_inc() will return to a caller before jump_label_update() finishes its job on cpu B.

OK, I see what you are saying. There's a race here that jump_label_inc may return before jump labels are actually activated. I have no issue with this change. This guarantees that jump labels will be active on return of jump_label_inc().

I'm assuming that jump_label_update() does memory barries, as it does modify code, and would be itself a synchronization point.

Acked-by Steven Rostedt <[email protected]>

-- Steve

Quoted 19 lines

Signed-off-by Gleb Natapov <[email protected]>

kernel/jump_label.c +2 -2

index a8ce450..e6f1f24 100644
@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }
 
--

Gleb.

Gleb Natapov

author

2011-11-02 15:21 UTC #4 lore raw

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · Gleb Natapov
If cpu A calls jump_label_inc() just after atomic_add_return() is called by cpu B, atomic_inc_not_zero() will return value greater then zero and jump_label_inc() will return to a caller before jump_label_update() finishes its job on cpu B.
OK, I see what you are saying. There's a race here that jump_label_inc may return before jump labels are actually activated. I have no issue with this change. This guarantees that jump labels will be active on return of jump_label_inc().

Quoted 5 lines

I'm assuming that jump_label_update() does memory barries, as it does modify code, and would be itself a synchronization point.
Acked-by Steven Rostedt <[email protected]>

So through what tree this should go to Linus' git?

Quoted 19 lines

-- Steve

Quoted 17 lines

Signed-off-by Gleb Natapov <[email protected]>

kernel/jump_label.c +2 -1

index a8ce450..e6f1f24 100644
@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }

Quoted 3 lines

Quoted 2 lines
--
Gleb.

--
Gleb.

Steven Rostedt

reviewer

2011-11-02 15:42 UTC #5 lore raw

Quoted 12 lines · Gleb Natapov

Quoted 11 lines · Steven Rostedt
Quoted 4 lines · Gleb Natapov
If cpu A calls jump_label_inc() just after atomic_add_return() is called by cpu B, atomic_inc_not_zero() will return value greater then zero and jump_label_inc() will return to a caller before jump_label_update() finishes its job on cpu B.
OK, I see what you are saying. There's a race here that jump_label_inc may return before jump labels are actually activated. I have no issue with this change. This guarantees that jump labels will be active on return of jump_label_inc().

Quoted 6 lines

Quoted 5 lines
I'm assuming that jump_label_update() does memory barries, as it does modify code, and would be itself a synchronization point.
Acked-by Steven Rostedt <[email protected]>
So through what tree this should go to Linus' git?

I'll pull it in. I'm currently scrambling around getting back up to speed after a 11 day conference spree.

-- Steve

tip-bot for Gleb Natapov

author

2011-11-18 23:13 UTC #6 lore raw

[tip:perf/core] jump_label: jump_label_inc may return before the code is patched

In reply to Gleb Natapov

Commit-ID:  c8452afb7426f7e21388492f40227582e3e83879
Gitweb:     http://git.kernel.org/tip/c8452afb7426f7e21388492f40227582e3e83879
Author:     Gleb Natapov <[email protected]>
AuthorDate: Tue, 18 Oct 2011 19:55:51 +0200
Committer:  Steven Rostedt <[email protected]>
CommitDate: Mon, 7 Nov 2011 11:02:34 -0500

jump_label: jump_label_inc may return before the code is patched

Link http://lkml.kernel.org/r/[email protected] open here

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Acked-by Jason Baron <[email protected]>

Signed-off-by Gleb Natapov <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/jump_label.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/jump_label.c +2 -1

index a8ce450..e6f1f24 100644
@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }

tip-bot for Gleb Natapov

author

2011-12-06 06:27 UTC #7 lore raw

[tip:perf/core] jump_label: jump_label_inc may return before the code is patched

In reply to Gleb Natapov

Commit-ID:  bbbf7af4bf8fc69bc751818cf30521080fa47dcb
Gitweb:     http://git.kernel.org/tip/bbbf7af4bf8fc69bc751818cf30521080fa47dcb
Author:     Gleb Natapov <[email protected]>
AuthorDate: Tue, 18 Oct 2011 19:55:51 +0200
Committer:  Steven Rostedt <[email protected]>
CommitDate: Mon, 5 Dec 2011 13:28:46 -0500

jump_label: jump_label_inc may return before the code is patched

Link http://lkml.kernel.org/r/[email protected] open here

Cc [email protected]

Cc Peter Zijlstra <[email protected]>

Acked-by Jason Baron <[email protected]>

Signed-off-by Gleb Natapov <[email protected]>

Signed-off-by Steven Rostedt <[email protected]>

---
 kernel/jump_label.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/jump_label.c +2 -1

index bbdfe2a..66ff710 100644
@@ -66,8 +66,9 @@ void jump_label_inc(struct jump_label_key *key)
 		return;
 
 	jump_label_lock();
-	if (atomic_add_return(1, &key->enabled) == 1)
+	if (atomic_read(&key->enabled) == 0)
 		jump_label_update(key, JUMP_LABEL_ENABLE);
+	atomic_inc(&key->enabled);
 	jump_label_unlock();
 }