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

[Roman Gushchin]

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@xxxxxx>
> >>> Cc: Alexei Starovoitov <ast@xxxxxxxxxx>
> >>> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx>
> >>> ---
> >>>  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!

Maybe it will be enough to land the cgroup local storage patchset,
what is my primary goal now. After that I'll try to look at __rcu
annotations too.

Thanks!