netdev - Re: [PATCH v2 bpf-next 03/17] bpf: Introduce BPF trampoline

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:   Thu, 7 Nov 2019 19:11:24 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Song Liu <liu.song.a23@...il.com>
Cc:     Song Liu <songliubraving@...com>,
        Alexei Starovoitov <ast@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "x86@...nel.org" <x86@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 03/17] bpf: Introduce BPF trampoline

On Thu, Nov 7, 2019 at 5:10 PM Song Liu <liu.song.a23@...il.com> wrote:
> > > >>>>> +               goto out;
> > > >>>>> +       tr->selector++;
> > > >>>>
> > > >>>> Shall we do selector-- for unlink?
> > > >>>
> > > >>> It's a bit flip. I think it would be more confusing with --
> > > >>
> > > >> Right.. Maybe should use int instead of u64 for selector?
> > > >
> > > > No, since int can overflow.
> > >
> > > I guess it is OK to overflow, no?
> >
> > overflow is not ok, since transition 0->1 should use nop->call patching
> > whereas 1->2, 2->3 should use call->call.
> >
> > In my initial implementation (one I didn't share with anyone) I had
> > trampoline_mutex taken inside bpf_trampoline_update(). And multiple link()
> > operation were allowed. The idea was to attach multiple progs and update
> > trampoline once. But then I realized that I cannot do that since 'unlink +
> > update' where only 'update' is taking lock will not guarantee success. Since
> > other 'link' operations can race and 'update' can potentially fail in
> > arch_prepare_bpf_trampoline() due to new things that 'link' brought in. In that
> > version (since there several fentry/fexit progs can come in at once) I used
> > separate 'selector' ticker to pick the side of the page. Once I realized the
> > issue (to guarantee that unlink+update == always success) I moved mutex all the
> > way to unlink and link and left 'selector' as-is. Just now I realized that
> > 'selector' can be removed.  fentry_cnt + fexit_cnt can be used instead. This
> > sum of counters will change 1 bit at a time. Am I right?
>
> Yeah, I think fentry_cnt + fexit_cnt is cleaner.

... and that didn't work.
It's transition that matters. Either need to remember previous sum value
or have separate selector. imo selector is cleaner, so I'm back to that.