netdev - Re: [PATCH v6 bpf-next 0/6] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200801071357.GA19421@ranger.igk.intel.com>
Date:   Sat, 1 Aug 2020 09:13:57 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     ast@...nel.org, bpf@...r.kernel.org, netdev@...r.kernel.org,
        bjorn.topel@...el.com, magnus.karlsson@...el.com
Subject: Re: [PATCH v6 bpf-next 0/6] bpf: tailcalls in BPF subprograms

On Sat, Aug 01, 2020 at 03:03:19AM +0200, Daniel Borkmann wrote:
> On 7/31/20 2:03 AM, Maciej Fijalkowski wrote:
> > v5->v6:
> > - propagate only those poke descriptors that individual subprogram is
> >    actually using (Daniel)
> > - drop the cumbersome check if poke desc got filled in map_poke_run()
> > - move poke->ip renaming in bpf_jit_add_poke_descriptor() from patch 4
> >    to patch 3 to provide bisectability (Daniel)
> 
> I did a basic test with Cilium on K8s with this set, spawning a few Pods
> and checking connectivity & whether we're not crashing since it has bit more
> elaborate tail call use. So far so good. I was inclined to push the series
> out, but there is one more issue I noticed and didn't notice earlier when
> reviewing, and that is overall stack size:
> 
> What happens when you create a single program that has nested BPF to BPF
> calls e.g. either up to the maximum nesting or one call that is using up
> the max stack size which is then doing another BPF to BPF call that contains
> the tail call. In the tail call map, you have the same program in there.
> This means we create a worst case stack from BPF size of max_stack_size *
> max_tail_call_size, that is, 512*32. So that adds 16k worst case. For x86
> we have a stack of arch/x86/include/asm/page_64_types.h:
> 
>   #define THREAD_SIZE_ORDER       (2 + KASAN_STACK_ORDER)
>  #define THREAD_SIZE  (PAGE_SIZE << THREAD_SIZE_ORDER)
> 
> So we end up with 16k in a typical case. And this will cause kernel stack
> overflow; I'm at least not seeing where we handle this situation in the
> set. Hm, need to think more, but maybe this needs tracking of max stack
> across tail calls to force an upper limit..

My knee jerk reaction would be to decrement the allowed max tail calls,
but not sure if it's an option and if it would help.

Otherwise I'm not sure how to pass around the stack size just like we're
doing it in this set with tail call counter that sits in %rax.

> 
> Thanks,
> Daniel