[net-next] bpf: avoid rcu_dereference inside bpf_event_mutex lock region

Message ID 20171030205022.3980618-1-yhs@fb.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net-next] bpf: avoid rcu_dereference inside bpf_event_mutex lock region
Related show

Commit Message

Yonghong Song Oct. 30, 2017, 8:50 p.m.
During perf event attaching/detaching bpf programs,
the tp_event->prog_array change is protected by the
bpf_event_mutex lock in both attaching and deteching
functions. Although tp_event->prog_array is a rcu
pointer, rcu_derefrence is not needed to access it
since mutex lock will guarantee ordering.

Verified through "make C=2" that sparse
locking check still happy with the new change.

Also change the label name in perf_event_{attach,detach}_bpf_prog
from "out" to "unlock" to reflect the code action after the label.

Signed-off-by: Yonghong Song <yhs@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Martin KaFai Lau <kafai@fb.com>
---
 kernel/trace/bpf_trace.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Peter,

Could you check whether the below change to remove rcu_dereference_protected
is what you wanted or not?

Thanks!

Comments

Peter Zijlstra Oct. 31, 2017, 9:33 a.m. | #1
On Mon, Oct 30, 2017 at 01:50:22PM -0700, Yonghong Song wrote:
> Could you check whether the below change to remove rcu_dereference_protected
> is what you wanted or not?

Yep that looks fine. Thanks!

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index b65011d..e7685c5 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -767,20 +767,19 @@ int perf_event_attach_bpf_prog(struct perf_event *event,
>  	mutex_lock(&bpf_event_mutex);
>  
>  	if (event->prog)
> -		goto out;
> +		goto unlock;
>  
> -	old_array = rcu_dereference_protected(event->tp_event->prog_array,
> -					      lockdep_is_held(&bpf_event_mutex));
> +	old_array = event->tp_event->prog_array;
>  	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
>  	if (ret < 0)
> -		goto out;
> +		goto unlock;
>  
>  	/* set the new array to event->tp_event and set event->prog */
>  	event->prog = prog;
>  	rcu_assign_pointer(event->tp_event->prog_array, new_array);
>  	bpf_prog_array_free(old_array);
>  
> -out:
> +unlock:
>  	mutex_unlock(&bpf_event_mutex);
>  	return ret;
>  }
> @@ -794,11 +793,9 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  	mutex_lock(&bpf_event_mutex);
>  
>  	if (!event->prog)
> -		goto out;
> -
> -	old_array = rcu_dereference_protected(event->tp_event->prog_array,
> -					      lockdep_is_held(&bpf_event_mutex));
> +		goto unlock;
>  
> +	old_array = event->tp_event->prog_array;
>  	ret = bpf_prog_array_copy(old_array, event->prog, NULL, &new_array);
>  	if (ret < 0) {
>  		bpf_prog_array_delete_safe(old_array, event->prog);
> @@ -810,6 +807,6 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  	bpf_prog_put(event->prog);
>  	event->prog = NULL;
>  
> -out:
> +unlock:
>  	mutex_unlock(&bpf_event_mutex);
>  }
> -- 
> 2.9.5
>

Patch

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b65011d..e7685c5 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -767,20 +767,19 @@  int perf_event_attach_bpf_prog(struct perf_event *event,
 	mutex_lock(&bpf_event_mutex);
 
 	if (event->prog)
-		goto out;
+		goto unlock;
 
-	old_array = rcu_dereference_protected(event->tp_event->prog_array,
-					      lockdep_is_held(&bpf_event_mutex));
+	old_array = event->tp_event->prog_array;
 	ret = bpf_prog_array_copy(old_array, NULL, prog, &new_array);
 	if (ret < 0)
-		goto out;
+		goto unlock;
 
 	/* set the new array to event->tp_event and set event->prog */
 	event->prog = prog;
 	rcu_assign_pointer(event->tp_event->prog_array, new_array);
 	bpf_prog_array_free(old_array);
 
-out:
+unlock:
 	mutex_unlock(&bpf_event_mutex);
 	return ret;
 }
@@ -794,11 +793,9 @@  void perf_event_detach_bpf_prog(struct perf_event *event)
 	mutex_lock(&bpf_event_mutex);
 
 	if (!event->prog)
-		goto out;
-
-	old_array = rcu_dereference_protected(event->tp_event->prog_array,
-					      lockdep_is_held(&bpf_event_mutex));
+		goto unlock;
 
+	old_array = event->tp_event->prog_array;
 	ret = bpf_prog_array_copy(old_array, event->prog, NULL, &new_array);
 	if (ret < 0) {
 		bpf_prog_array_delete_safe(old_array, event->prog);
@@ -810,6 +807,6 @@  void perf_event_detach_bpf_prog(struct perf_event *event)
 	bpf_prog_put(event->prog);
 	event->prog = NULL;
 
-out:
+unlock:
 	mutex_unlock(&bpf_event_mutex);
 }