| Message ID | 20180814180112.293058-1-yhs@fb.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core() | expand |
On Tue, Aug 14, 2018 at 11:01:12AM -0700, Yonghong Song wrote: > Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers > to the cgroup storage") refactored the bpf_prog_array_copy_core() > to accommodate new structure bpf_prog_array_item which contains > bpf_prog array itself. > > In the old code, we had > perf_event_query_prog_array(): > mutex_lock(...) > bpf_prog_array_copy_call(): > prog = rcu_dereference_check(array, 1)->progs > bpf_prog_array_copy_core(prog, ...) > mutex_unlock(...) > > With the above commit, we had > perf_event_query_prog_array(): > mutex_lock(...) > bpf_prog_array_copy_call(): > bpf_prog_array_copy_core(array, ...): > item = rcu_dereference(array)->items; > ... > mutex_unlock(...) > > The new code will trigger a lockdep rcu checking warning. > The fix is to change rcu_dereference() to rcu_dereference_check() > to prevent such a warning. > > Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") > Cc: Roman Gushchin <guro@fb.com> > Signed-off-by: Yonghong Song <yhs@fb.com> makes sense to me Acked-by: Alexei Starovoitov <ast@kernel.org> Roman, would you agree?
On Tue, Aug 14, 2018 at 04:59:45PM -0700, Alexei Starovoitov wrote: > On Tue, Aug 14, 2018 at 11:01:12AM -0700, Yonghong Song wrote: > > Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers > > to the cgroup storage") refactored the bpf_prog_array_copy_core() > > to accommodate new structure bpf_prog_array_item which contains > > bpf_prog array itself. > > > > In the old code, we had > > perf_event_query_prog_array(): > > mutex_lock(...) > > bpf_prog_array_copy_call(): > > prog = rcu_dereference_check(array, 1)->progs > > bpf_prog_array_copy_core(prog, ...) > > mutex_unlock(...) > > > > With the above commit, we had > > perf_event_query_prog_array(): > > mutex_lock(...) > > bpf_prog_array_copy_call(): > > bpf_prog_array_copy_core(array, ...): > > item = rcu_dereference(array)->items; > > ... > > mutex_unlock(...) > > > > The new code will trigger a lockdep rcu checking warning. > > The fix is to change rcu_dereference() to rcu_dereference_check() > > to prevent such a warning. > > > > Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com > > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") > > Cc: Roman Gushchin <guro@fb.com> > > Signed-off-by: Yonghong Song <yhs@fb.com> > > makes sense to me > Acked-by: Alexei Starovoitov <ast@kernel.org> > > Roman, would you agree? > rcu_dereference_check(<>, 1) always looks a bit strange to me, but if it's the only reasonable way to silence the warning, of course I'm fine with it. Thanks!
On Tue, Aug 14, 2018 at 05:08:44PM -0700, Roman Gushchin wrote: > On Tue, Aug 14, 2018 at 04:59:45PM -0700, Alexei Starovoitov wrote: > > On Tue, Aug 14, 2018 at 11:01:12AM -0700, Yonghong Song wrote: > > > Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers > > > to the cgroup storage") refactored the bpf_prog_array_copy_core() > > > to accommodate new structure bpf_prog_array_item which contains > > > bpf_prog array itself. > > > > > > In the old code, we had > > > perf_event_query_prog_array(): > > > mutex_lock(...) > > > bpf_prog_array_copy_call(): > > > prog = rcu_dereference_check(array, 1)->progs > > > bpf_prog_array_copy_core(prog, ...) > > > mutex_unlock(...) > > > > > > With the above commit, we had > > > perf_event_query_prog_array(): > > > mutex_lock(...) > > > bpf_prog_array_copy_call(): > > > bpf_prog_array_copy_core(array, ...): > > > item = rcu_dereference(array)->items; > > > ... > > > mutex_unlock(...) > > > > > > The new code will trigger a lockdep rcu checking warning. > > > The fix is to change rcu_dereference() to rcu_dereference_check() > > > to prevent such a warning. > > > > > > Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com > > > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") > > > Cc: Roman Gushchin <guro@fb.com> > > > Signed-off-by: Yonghong Song <yhs@fb.com> > > > > makes sense to me > > Acked-by: Alexei Starovoitov <ast@kernel.org> > > > > Roman, would you agree? > > > > rcu_dereference_check(<>, 1) always looks a bit strange to me, > but if it's the only reasonable way to silence the warning, > of course I'm fine with it. do you have better suggestion? This patch is a fix for the regression introduced in your earlier patch, so I think the only fair path forward is either to Ack it or to send an alternative patch asap.
On Wed, Aug 15, 2018 at 02:30:11PM -0700, Alexei Starovoitov wrote: > On Tue, Aug 14, 2018 at 05:08:44PM -0700, Roman Gushchin wrote: > > On Tue, Aug 14, 2018 at 04:59:45PM -0700, Alexei Starovoitov wrote: > > > On Tue, Aug 14, 2018 at 11:01:12AM -0700, Yonghong Song wrote: > > > > Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers > > > > to the cgroup storage") refactored the bpf_prog_array_copy_core() > > > > to accommodate new structure bpf_prog_array_item which contains > > > > bpf_prog array itself. > > > > > > > > In the old code, we had > > > > perf_event_query_prog_array(): > > > > mutex_lock(...) > > > > bpf_prog_array_copy_call(): > > > > prog = rcu_dereference_check(array, 1)->progs > > > > bpf_prog_array_copy_core(prog, ...) > > > > mutex_unlock(...) > > > > > > > > With the above commit, we had > > > > perf_event_query_prog_array(): > > > > mutex_lock(...) > > > > bpf_prog_array_copy_call(): > > > > bpf_prog_array_copy_core(array, ...): > > > > item = rcu_dereference(array)->items; > > > > ... > > > > mutex_unlock(...) > > > > > > > > The new code will trigger a lockdep rcu checking warning. > > > > The fix is to change rcu_dereference() to rcu_dereference_check() > > > > to prevent such a warning. > > > > > > > > Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com > > > > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") > > > > Cc: Roman Gushchin <guro@fb.com> > > > > Signed-off-by: Yonghong Song <yhs@fb.com> > > > > > > makes sense to me > > > Acked-by: Alexei Starovoitov <ast@kernel.org> > > > > > > Roman, would you agree? > > > > > > > rcu_dereference_check(<>, 1) always looks a bit strange to me, > > but if it's the only reasonable way to silence the warning, > > of course I'm fine with it. > > do you have better suggestion? > This patch is a fix for the regression introduced in your earlier patch, > so I think the only fair path forward is either to Ack it or > to send an alternative patch asap. > As I said, I've nothing against. Acked-by: Roman Gushchin <guro@fb.com> Thanks!
On 08/15/2018 02:08 AM, Roman Gushchin wrote: > On Tue, Aug 14, 2018 at 04:59:45PM -0700, Alexei Starovoitov wrote: >> On Tue, Aug 14, 2018 at 11:01:12AM -0700, Yonghong Song wrote: >>> Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers >>> to the cgroup storage") refactored the bpf_prog_array_copy_core() >>> to accommodate new structure bpf_prog_array_item which contains >>> bpf_prog array itself. >>> >>> In the old code, we had >>> perf_event_query_prog_array(): >>> mutex_lock(...) >>> bpf_prog_array_copy_call(): >>> prog = rcu_dereference_check(array, 1)->progs >>> bpf_prog_array_copy_core(prog, ...) >>> mutex_unlock(...) >>> >>> With the above commit, we had >>> perf_event_query_prog_array(): >>> mutex_lock(...) >>> bpf_prog_array_copy_call(): >>> bpf_prog_array_copy_core(array, ...): >>> item = rcu_dereference(array)->items; >>> ... >>> mutex_unlock(...) >>> >>> The new code will trigger a lockdep rcu checking warning. >>> The fix is to change rcu_dereference() to rcu_dereference_check() >>> to prevent such a warning. >>> >>> Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com >>> Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") >>> Cc: Roman Gushchin <guro@fb.com> >>> Signed-off-by: Yonghong Song <yhs@fb.com> Applied to bpf, thanks Yonghong!
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 4d09e610777f..3f5bf1af0826 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -1579,7 +1579,7 @@ static bool bpf_prog_array_copy_core(struct bpf_prog_array __rcu *array, struct bpf_prog_array_item *item; int i = 0; - item = rcu_dereference(array)->items; + item = rcu_dereference_check(array, 1)->items; for (; item->prog; item++) { if (item->prog == &dummy_bpf_prog.prog) continue;
Commit 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") refactored the bpf_prog_array_copy_core() to accommodate new structure bpf_prog_array_item which contains bpf_prog array itself. In the old code, we had perf_event_query_prog_array(): mutex_lock(...) bpf_prog_array_copy_call(): prog = rcu_dereference_check(array, 1)->progs bpf_prog_array_copy_core(prog, ...) mutex_unlock(...) With the above commit, we had perf_event_query_prog_array(): mutex_lock(...) bpf_prog_array_copy_call(): bpf_prog_array_copy_core(array, ...): item = rcu_dereference(array)->items; ... mutex_unlock(...) The new code will trigger a lockdep rcu checking warning. The fix is to change rcu_dereference() to rcu_dereference_check() to prevent such a warning. Reported-by: syzbot+6e72317008eef84a216b@syzkaller.appspotmail.com Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the cgroup storage") Cc: Roman Gushchin <guro@fb.com> Signed-off-by: Yonghong Song <yhs@fb.com> --- kernel/bpf/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)