netdev - Re: [PATCH v8 bpf-next 0/7] bpf: tailcalls in BPF subprograms

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-next>] [day] [month] [year] [list]

Message-ID: <CAADnVQLEYHZLeu-d4nV5Px6t+tVtYEgg8AfPE5-GwAS1uizc0w@mail.gmail.com>
Date:   Thu, 17 Sep 2020 20:26:34 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Andrii Nakryiko <andriin@...com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH v8 bpf-next 0/7] bpf: tailcalls in BPF subprograms

On Wed, Sep 16, 2020 at 2:16 PM Maciej Fijalkowski
<maciej.fijalkowski@...el.com> wrote:
>
> Changelog:
>
> v7->v8:
> - teach bpf_patch_insn_data to adjust insn_idx of tailcall insn
> - address case of clearing tail call counter when bpf2bpf with tailcalls
>   are mixed
> - drop unnecessary checks against cbpf in JIT
> - introduce more tailcall_bpf2bpf[X] selftests that are making sure the
>   combination of tailcalls with bpf2bpf calls works correctly
> - add test cases to test_verifier to make sure logic from
>   check_max_stack_depth that limits caller's stack depth down to 256 is
>   correct
> - move the main patch to appear after the one that limits the caller's
>   stack depth so that 'has_tail_call' can be used by 'tail_call_reachable'
>   logic

Thanks a lot for your hard work on this set. 5 month effort!
I think it's a huge milestone that will enable cilium, cloudflare, katran to
use bpf functions. Removing always_inline will improve performance.
Switching to global functions with function-by-function verification
will drastically improve program load times.
libbpf has full support for subprogram composition and call relocations.
Until now these verifier and libbpf features were impossible to use in XDP
programs, since most of them use tail_calls.
It's great to see all these building blocks finally coming together.

I've applied the set with few changes.
In patch 4 I've removed ifdefs and redundant ().
In patch 5 removed redundant !tail_call_reachable check.
In patch 6 replaced CONFIG_JIT_ALWAYS_ON dependency with
jit_requested && IS_ENABLED(CONFIG_X86_64).
It's more user friendly.
I also added patch 7 that checks that ld_abs and tail_call are only
allowed in subprograms that return 'int'.
I felt that the fix is simple enough, so I just pushed it, since
without it the set is not safe. Please review it here:
https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=09b28d76eac48e922dc293da1aa2b2b85c32aeee
I'll address any issues in the followups.
Because of the above changes I tweaked patch 8 to increase test coverage
with ld_abs and combination of global/static subprogs.
Also did s/__attribute__((noinline))/__noinline/.

John and Daniel,
I wasn't able to test it on cilium programs.
When you have a chance please give it a thorough run.
tail_call poke logic is delicate.

Lorenz,
if you can test it on cloudflare progs would be awesome.

Thanks a lot everyone!