Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a
 generic non-rcu pointer
To:     Roman Gushchin <guro@fb.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        kernel-team@fb.com, Alexei Starovoitov <ast@kernel.org>
References: <20180713194114.2711-1-guro@fb.com>
 <20180713194114.2711-3-guro@fb.com>
 <41b5f919-3155-5bd4-2774-84efd58650e2@iogearbox.net>
 <20180716225715.GB3898@castle.DHCP.thefacebook.com>
From:   Daniel Borkmann <daniel@iogearbox.net>
Message-ID: <1a40b0a8-7440-f2d2-d743-64901b5d1bfe@iogearbox.net>
Date:   Wed, 18 Jul 2018 00:38:50 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <20180716225715.GB3898@castle.DHCP.thefacebook.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 07/17/2018 12:57 AM, Roman Gushchin wrote:
> On Tue, Jul 17, 2018 at 12:30:18AM +0200, Daniel Borkmann wrote:
>> On 07/13/2018 09:41 PM, Roman Gushchin wrote:
>>> bpf_prog_array_free() should take a generic non-rcu pointer
>>> as an argument, as freeing the objects assumes that we're
>>> holding an exclusive rights on it.
>>>
>>> rcu_access_pointer() can be used to convert a __rcu pointer to
>>> a generic pointer before passing it to bpf_prog_array_free(),
>>> if necessary.
>>>
>>> This patch eliminates the following sparse warning:
>>> kernel/bpf/core.c:1556:9: warning: incorrect type in argument 1 (different address spaces)
>>> kernel/bpf/core.c:1556:9:    expected struct callback_head *head
>>> kernel/bpf/core.c:1556:9:    got struct callback_head [noderef] <asn:4>*<noident>
>>>
>>> Fixes: 324bda9e6c5a ("bpf: multi program support for cgroup+bpf")
>>> Signed-off-by: Roman Gushchin <guro@fb.com>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> ---
>>>  drivers/media/rc/bpf-lirc.c |  6 +++---
>>>  include/linux/bpf.h         |  2 +-
>>>  kernel/bpf/cgroup.c         | 11 ++++++-----
>>>  kernel/bpf/core.c           |  5 ++---
>>>  kernel/trace/bpf_trace.c    |  8 ++++----
>>>  5 files changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
>>> index fcfab6635f9c..509b262aa0dc 100644
>>> --- a/drivers/media/rc/bpf-lirc.c
>>> +++ b/drivers/media/rc/bpf-lirc.c
>>> @@ -135,7 +135,7 @@ static int lirc_bpf_attach(struct rc_dev *rcdev, struct bpf_prog *prog)
>>>  		goto unlock;
>>>  
>>>  	rcu_assign_pointer(raw->progs, new_array);
>>> -	bpf_prog_array_free(old_array);
>>> +	bpf_prog_array_free(rcu_access_pointer(old_array));
>>
>> Taking this one as an example, why can't we already do the rcu_dereference() on the
>> 'old_array = raw->progs' where we fetch the old_array initially? Then we also wouldn't
>> need the rcu_access_pointer() on bpf_prog_array_free() and yet another rcu_dereference()
>> inside the bpf_prog_array_copy() from your later patch?
> 
> We can, but then we have to change bpf_prog_array_copy() args annotation,
> and also all places, where it's called.
> IMO, basically all local variables and function args marked as __rcu
> should be not marked as RCU, but fixing them all is beyond this patchset.

Right, agree, the __rcu markings seem somewhat arbitrary. :-( I think we need to
investigate this a bit deeper and do a proper audit on the whole bpf prog array's
RCU handling (probably won't get to it in next two weeks but put onto backlog just
in case it's still unresolved till then). That said, given this has been there for
quite a while and it's rc5 now, I think we could start out on bpf-next with the
obvious candidates which should be okay even if it ends up bigger. First two from
this series we could already take in if you prefer.

Thanks,
Daniel