Re: [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core()
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+6e72317008eef84a2...@syzkaller.appspotmail.com
>>> Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the
>>> cgroup storage")
>>> Cc: Roman Gushchin
>>> Signed-off-by: Yonghong Song
Applied to bpf, thanks Yonghong!
Re: [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core()
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+6e72317008eef84a2...@syzkaller.appspotmail.com
> > > > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to
> > > > the cgroup storage")
> > > > Cc: Roman Gushchin
> > > > Signed-off-by: Yonghong Song
> > >
> > > makes sense to me
> > > Acked-by: Alexei Starovoitov
> > >
> > > 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
Thanks!
Re: [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core()
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+6e72317008eef84a2...@syzkaller.appspotmail.com
> > > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the
> > > cgroup storage")
> > > Cc: Roman Gushchin
> > > Signed-off-by: Yonghong Song
> >
> > makes sense to me
> > Acked-by: Alexei Starovoitov
> >
> > 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.
Re: [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core()
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+6e72317008eef84a2...@syzkaller.appspotmail.com
> > Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the
> > cgroup storage")
> > Cc: Roman Gushchin
> > Signed-off-by: Yonghong Song
>
> makes sense to me
> Acked-by: Alexei Starovoitov
>
> 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!
Re: [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core()
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+6e72317008eef84a2...@syzkaller.appspotmail.com
> Fixes: 394e40a29788 ("bpf: extend bpf_prog_array to store pointers to the
> cgroup storage")
> Cc: Roman Gushchin
> Signed-off-by: Yonghong Song
makes sense to me
Acked-by: Alexei Starovoitov
Roman, would you agree?
