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

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]

Date:   Fri, 28 Feb 2020 13:17:08 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, Andrii Nakryiko <andriin@...com>,
        Yonghong Song <yhs@...com>, Song Liu <songliubraving@...com>,
        Martin KaFai Lau <kafai@...com>,
        Jakub Kicinski <kuba@...nel.org>,
        David Miller <davem@...hat.com>,
        Björn Töpel <bjorn.topel@...el.com>,
        John Fastabend <john.fastabend@...il.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH 10/18] bpf: Re-initialize lnode in bpf_ksym_del

On Thu, Feb 27, 2020 at 11:50:36AM -0800, Alexei Starovoitov wrote:
> 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@...nel.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.

aah ok, that should also solve your first question,
because the code above won't be needed anymore

I wish I read this comment before I prepared elabored ascii/code
picture of why the code above is needed ;-))

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

ok, will change

> 
> 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);

ok

> 
> 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?

yes, as I said I only added it because I liked how simple it
turned out to be

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

actualy there're some changes on the list from this week, that touch
the same code, so we might need to take them through Arnaldo's code

I'l double check

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

will respin soon, thanks for review

jirka