netdev - Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a generic non-rcu pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <90bc1d99-da1b-065a-cd4f-457b7f17a533@iogearbox.net>
Date:   Wed, 18 Jul 2018 15:07:48 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Roman Gushchin <guro@...com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...com, Alexei Starovoitov <ast@...nel.org>
Subject: Re: [PATCH v2 bpf 3/5] bpf: bpf_prog_array_free() should take a
 generic non-rcu pointer

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@...com>
>>>>> Cc: Alexei Starovoitov <ast@...nel.org>
>>>>> Cc: Daniel Borkmann <daniel@...earbox.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!