netdev - Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5) for BPF_TRAMP_F_CALL

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]

Message-ID: <CAADnVQ+fg-HMM=TtsrZx1kJQpy7-fckcgkN00L-Gp5Aa-CzmQQ@mail.gmail.com>
Date:   Fri, 19 Mar 2021 17:33:04 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Stanislav Fomichev <sdf@...gle.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf] bpf: use NOP_ATOMIC5 instead of emit_nops(&prog, 5)
 for BPF_TRAMP_F_CALL_ORIG

On Fri, Mar 19, 2021 at 5:25 PM Stanislav Fomichev <sdf@...gle.com> wrote:
>
> On Fri, Mar 19, 2021 at 5:14 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Fri, Mar 19, 2021 at 5:00 PM Stanislav Fomichev <sdf@...gle.com> wrote:
> > >
> > > __bpf_arch_text_poke does rewrite only for atomic nop5, emit_nops(xxx, 5)
> > > emits non-atomic one which breaks fentry/fexit with k8 atomics:
> > >
> > > P6_NOP5 == P6_NOP5_ATOMIC (0f1f440000 == 0f1f440000)
> > > K8_NOP5 != K8_NOP5_ATOMIC (6666906690 != 6666666690)
> > >
> > > Can be reproduced by doing "ideal_nops = k8_nops" in "arch_init_ideal_nops()
> > > and running fexit_bpf2bpf selftest.
> > >
> > > Fixes: e21aa341785c ("bpf: Fix fexit trampoline.")
> > > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > > ---
> > >  arch/x86/net/bpf_jit_comp.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> > > index 72b5a57e9e31..b35fc8023884 100644
> > > --- a/arch/x86/net/bpf_jit_comp.c
> > > +++ b/arch/x86/net/bpf_jit_comp.c
> > > @@ -2012,7 +2012,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> > >                 /* remember return value in a stack for bpf prog to access */
> > >                 emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
> > >                 im->ip_after_call = prog;
> > > -               emit_nops(&prog, 5);
> > > +               memcpy(prog, ideal_nops[NOP_ATOMIC5], X86_PATCH_SIZE);
> > > +               prog += X86_PATCH_SIZE;
> >
> > I'm well aware, but ideal_nops are pretty much gone already.
> > The changes are already in the -tip tree.
> > So I decided to reduce the conflicts for the merge window.
> >
> > Do you actually see the breakage or it's purely theoretical?
> We do see it, but it's on our tree that pulls from bpf.
> And it obviously doesn't have that "x86: Remove dynamic NOP selection" yet.
> Thanks for the pointer, I guess I can just wait for the real merge then.

If it breaks the real users we have to land the fix, but let me ask how
come that you run with k8 cpu? k8 does other nasty things.
Do you run with all of amd errata?