Re: [PATCH bpf-next 2/5] bpf: allow for tailcalls in BPF subprograms

From: Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	ast@kernel.org, bpf@vger.kernel.org, netdev@vger.kernel.org,
	bjorn.topel@intel.com, magnus.karlsson@intel.com
Subject: Re: [PATCH bpf-next 2/5] bpf: allow for tailcalls in BPF subprograms
Date: Fri, 17 Jul 2020 13:39:41 +0200	[thread overview]
Message-ID: <CAOuyyO5whrfR5tzrwhT9UPjog6EY4AE28kc6jxReMrSm435vgA@mail.gmail.com> (raw)
In-Reply-To: <d12561c3-23c6-3f48-611f-868be990e1a2@iogearbox.net>

On Fri, Jul 17, 2020 at 1:12 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 7/16/20 11:29 PM, Daniel Borkmann wrote:
> > On 7/16/20 1:36 AM, Maciej Fijalkowski wrote:
> >> Relax verifier's restriction that was meant to forbid tailcall usage
> >> when subprog count was higher than 1.
> >>
> >> Also, do not max out the stack depth of program that utilizes tailcalls.
> >>
> >> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> >> ---
> >>   kernel/bpf/verifier.c | 5 -----
> >>   1 file changed, 5 deletions(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 3c1efc9d08fd..6481342b31ba 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -4172,10 +4172,6 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
> >>       case BPF_FUNC_tail_call:
> >>           if (map->map_type != BPF_MAP_TYPE_PROG_ARRAY)
> >>               goto error;
> >> -        if (env->subprog_cnt > 1) {
> >> -            verbose(env, "tail_calls are not allowed in programs with bpf-to-bpf calls\n");
> >> -            return -EINVAL;
> >> -        }
> >>           break;
> >>       case BPF_FUNC_perf_event_read:
> >>       case BPF_FUNC_perf_event_output:
> >> @@ -10252,7 +10248,6 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> >>                * the program array.
> >>                */
> >>               prog->cb_access = 1;
> >> -            env->prog->aux->stack_depth = MAX_BPF_STACK;
> >>               env->prog->aux->max_pkt_offset = MAX_PACKET_OFF;
> >>               /* mark bpf_tail_call as different opcode to avoid
> >
> > Also, isn't this broken when JIT is not used (as in stack oob access)?
>
> (Similarly for non-x86 archs after this set.)

Honestly at this point I'm not sure how to approach it, but as I said I'm
in a bit of a rush so probably not thinking clearly :)

So in the end we want to allow it *only* for case when underlying arch
is the x86-64 and when JIT is turned on, correct? Is this a matter of
#define's juggling or how do you see it?