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

Message-ID: <20200921160537.GA31703@ranger.igk.intel.com>
Date:   Mon, 21 Sep 2020 18:05:37 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        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 Thu, Sep 17, 2020 at 08:26:34PM -0700, Alexei Starovoitov wrote:
> 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!

Thanks for the whole collaboration! It was quite a ride :)

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

Thank you for this last touch! I went through patches and I agree with the
changes.

> 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

LGTM.

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