Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

[Arnaldo Carvalho de Melo <acme@redhat.com>]

Em Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov escreveu:
> On Wed, Feb 26, 2020 at 02:03:37PM +0100, Jiri Olsa wrote:
> > When bpf_prog is removed from kallsyms it's on the way
> > out to be removed, so we don't care about lnode state.
> > 
> > However the bpf_ksym_del will be used also by bpf_trampoline
> > and bpf_dispatcher objects, which stay allocated even when
> > they are not in kallsyms list, hence the lnode re-init.
> > 
> > The list_del_rcu commentary states that we need to call
> > synchronize_rcu, before we can change/re-init the list_head
> > pointers.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  kernel/bpf/core.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> > index c95424fc53de..1af2109b45c7 100644
> > --- a/kernel/bpf/core.c
> > +++ b/kernel/bpf/core.c
> > @@ -672,6 +672,13 @@ void bpf_ksym_del(struct bpf_ksym *ksym)
> >  	spin_lock_bh(&bpf_lock);
> >  	__bpf_ksym_del(ksym);
> >  	spin_unlock_bh(&bpf_lock);
> > +
> > +	/*
> > +	 * As explained in list_del_rcu, We must call synchronize_rcu
> > +	 * before changing list_head pointers.
> > +	 */
> > +	synchronize_rcu();
> > +	INIT_LIST_HEAD_RCU(&ksym->lnode);
> 
> I don't understand what this is for.
> The comment made it even more confusing.
> What kind of ksym reuse are you expecting?
> 
> Looking at trampoline and dispatcher patches I think cnt == 0
> condition is unnecessary. Just add them to ksym at creation time
> and remove from ksym at destroy. Both are executable code sections.
> Though RIP should never point into them while there are no progs
> I think it's better to keep them in ksym always.
> Imagine sw race conditions in destruction. CPU bugs. What not.
> 
> In patch 3 the name
> bpf_get_prog_addr_region(const struct bpf_prog *prog)
> became wrong and 'const' pointer makes it even more misleading.
> The function is not getting prog addr. It's setting ksym's addr.
> I think it should be called:
> bpf_ksym_set_addr(struct bpf_ksym *ksym);
> __always_inline should be removed too.
> 
> Similar in patch 4:
> static void bpf_get_prog_name(const struct bpf_prog *prog)
> also is wrong for the same reasons.
> It probably should be:
> static void bpf_ksym_set_name(struct bpf_ksym *ksym);
> 
> I'm still not confortable with patch 15 sorting bit.
> next = rb_next(&ksym->tnode.node[0]);
> if (next)
> is too tricky for me. I cannot wrap my head yet.
> Since user space doesn't rely on sorted order could you drop it?
> 
> Do patches 16-18 strongly depend on patches 1-15 ?
> We can take them via bpf-next tree. No problem. Just need Arnaldo's ack.

No problems, sent the acks, we can sort out problems later, but from the
top of my mind I can't antecipate any,

- Arnaldo
 
> Overall looks great. All around important work.
> Please address above and respin. I would like to land it soon.