Re: [PATCH bpf-next 2/2] bpf/flow_dissector: Introduce BPF_FLOW_DISSECTOR_CONTINUE retcode for flow-dissector bpf progs

From: Stanislav Fomichev <sdf@google.com>
To: Shmulik Ladkani <shmulik@metanetworks.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Jakub Sitnicki <jakub@cloudflare.com>,
	Petar Penkov <ppenkov@google.com>,
	Willem de Bruijn <willemb@google.com>,
	Shmulik Ladkani <shmulik.ladkani@gmail.com>
Subject: Re: [PATCH bpf-next 2/2] bpf/flow_dissector: Introduce BPF_FLOW_DISSECTOR_CONTINUE retcode for flow-dissector bpf progs
Date: Thu, 18 Aug 2022 09:12:43 -0700	[thread overview]
Message-ID: <CAKH8qBsrUL2eV4YcxVYqp+3Fqx+Gx667othK8O-5Lp8r9yM_8w@mail.gmail.com> (raw)
In-Reply-To: <20220818062405.947643-3-shmulik.ladkani@gmail.com>

On Wed, Aug 17, 2022 at 11:24 PM Shmulik Ladkani
<shmulik@metanetworks.com> wrote:
>
> Currently, attaching BPF_PROG_TYPE_FLOW_DISSECTOR programs completely
> replaces the flow-dissector logic with custom dissection logic.
> This forces implementors to write programs that handle dissection for
> any flows expected in the namespace.
>
> It makes sense for flow-dissector bpf programs to just augment the
> dissector with custom logic (e.g. dissecting certain flows or custom
> protocols), while enjoying the broad capabilities of the standard
> dissector for any other traffic.
>
> Introduce BPF_FLOW_DISSECTOR_CONTINUE retcode. Flow-dissector bpf
> programs may return this to indicate no dissection was made, and
> fallback to the standard dissector is requested.

Some historic perspective: the original goal was to explicitly not
fallback to the c code.
It seems like it should be fine with this extra return code.
But let's also extend tools/testing/selftests/bpf/progs/bpf_flow.c
with a case that exercises this new return code?

> Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
> ---
>  include/uapi/linux/bpf.h  | 5 +++++
>  net/core/flow_dissector.c | 3 +++
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 7bf9ba1329be..6d6654da7cef 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5836,6 +5836,11 @@ enum bpf_ret_code {
>          *    represented by BPF_REDIRECT above).
>          */
>         BPF_LWT_REROUTE = 128,
> +       /* BPF_FLOW_DISSECTOR_CONTINUE: used by BPF_PROG_TYPE_FLOW_DISSECTOR
> +        *   to indicate that no custom dissection was performed, and
> +        *   fallback to standard dissector is requested.
> +        */
> +       BPF_FLOW_DISSECTOR_CONTINUE = 129,
>  };

Is it too late to also amend verifier's check_return_code to allow
only a small subset of return types for flow-disccestor program type?

>  struct bpf_sock {
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index a01817fb4ef4..990429c69ccd 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1022,11 +1022,14 @@ bool __skb_flow_dissect(const struct net *net,
>                         prog = READ_ONCE(run_array->items[0].prog);
>                         result = bpf_flow_dissect(prog, &ctx, n_proto, nhoff,
>                                                   hlen, flags);
> +                       if (result == BPF_FLOW_DISSECTOR_CONTINUE)
> +                               goto dissect_continue;
>                         __skb_flow_bpf_to_target(&flow_keys, flow_dissector,
>                                                  target_container);
>                         rcu_read_unlock();
>                         return result == BPF_OK;
>                 }
> +dissect_continue:
>                 rcu_read_unlock();
>         }
>
> --
> 2.37.1
>