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>
 <1a40b0a8-7440-f2d2-d743-64901b5d1bfe@iogearbox.net>
 <20180717225520.GA15643@castle.DHCP.thefacebook.com>
From:   Daniel Borkmann <daniel@iogearbox.net>
Message-ID: <90bc1d99-da1b-065a-cd4f-457b7f17a533@iogearbox.net>
Date:   Wed, 18 Jul 2018 15:07:48 +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: <20180717225520.GA15643@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/18/2018 12:55 AM, Roman Gushchin wrote:
> On Wed, Jul 18, 2018 at 12:38:50AM +0200, Daniel Borkmann wrote:
>> 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.
> 
> Totally agree.
> 
>> First two from this series we could already take in if you prefer.
> 
> That would be nice!

Ok, done, applied 1+2 to bpf-next, thanks Roman!