From mboxrd@z Thu Jan 1 00:00:00 1970 From: Roman Gushchin Subject: Re: [PATCH bpf] bpf: fix a rcu usage warning in bpf_prog_array_copy_core() Date: Tue, 14 Aug 2018 17:08:44 -0700 Message-ID: <20180815000841.GA25304@castle.DHCP.thefacebook.com> References: <20180814180112.293058-1-yhs@fb.com> <20180814235944.5nufzmj6d2rkknvs@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: Yonghong Song , , , , To: Alexei Starovoitov Return-path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:44040 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727742AbeHOC6x (ORCPT ); Tue, 14 Aug 2018 22:58:53 -0400 Content-Disposition: inline In-Reply-To: <20180814235944.5nufzmj6d2rkknvs@ast-mbp> Sender: netdev-owner@vger.kernel.org List-ID: 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 > > 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!