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@xxxxxxxxxx> > > --- > > 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.
