| Message ID | 20171030205022.3980618-1-yhs@fb.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] bpf: avoid rcu_dereference inside bpf_event_mutex lock region | expand |
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 >
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); }
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!