[PATCH bpf-next 0/2] flow_dissector: Allow bpf flow-dissector progs to request fallback to normal dissection

[PATCH bpf-next 0/2] flow_dissector: Allow bpf flow-dissector progs to request fallback to normal dissection

[Shmulik Ladkani]

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.

Shmulik Ladkani (2):
  flow_dissector: Make 'bpf_flow_dissect' return the bpf program retcode
  bpf/flow_dissector: Introduce BPF_FLOW_DISSECTOR_CONTINUE retcode for
    flow-dissector bpf progs

 include/linux/skbuff.h    |  4 ++--
 include/uapi/linux/bpf.h  |  5 +++++
 net/bpf/test_run.c        |  2 +-
 net/core/flow_dissector.c | 16 ++++++++++------
 4 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.37.1

[PATCH bpf-next 1/2] flow_dissector: Make 'bpf_flow_dissect' return the bpf program retcode

[Shmulik Ladkani]

Let 'bpf_flow_dissect' callers know the bpf program's retcode and act
accordingly.

Signed-off-by: Shmulik Ladkani <shmulik.ladkani@gmail.com>
---
 include/linux/skbuff.h    |  4 ++--
 net/bpf/test_run.c        |  2 +-
 net/core/flow_dissector.c | 13 +++++++------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index ca8afa382bf2..87921996175c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1460,8 +1460,8 @@ void skb_flow_dissector_init(struct flow_dissector *flow_dissector,
 			     unsigned int key_count);
 
 struct bpf_flow_dissector;
-bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
-		      __be16 proto, int nhoff, int hlen, unsigned int flags);
+u32 bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
+		     __be16 proto, int nhoff, int hlen, unsigned int flags);
 
 bool __skb_flow_dissect(const struct net *net,
 			const struct sk_buff *skb,
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index d11209367dd0..401b807a08d2 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1440,7 +1440,7 @@ int bpf_prog_test_run_flow_dissector(struct bpf_prog *prog,
 	bpf_test_timer_enter(&t);
 	do {
 		retval = bpf_flow_dissect(prog, &ctx, eth->h_proto, ETH_HLEN,
-					  size, flags);
+					  size, flags) == BPF_OK;
 	} while (bpf_test_timer_continue(&t, 1, repeat, &ret, &duration));
 	bpf_test_timer_leave(&t);
 
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 764c4cb3fe8f..a01817fb4ef4 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -866,8 +866,8 @@ static void __skb_flow_bpf_to_target(const struct bpf_flow_keys *flow_keys,
 	}
 }
 
-bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
-		      __be16 proto, int nhoff, int hlen, unsigned int flags)
+u32 bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
+		     __be16 proto, int nhoff, int hlen, unsigned int flags)
 {
 	struct bpf_flow_keys *flow_keys = ctx->flow_keys;
 	u32 result;
@@ -892,7 +892,7 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
 	flow_keys->thoff = clamp_t(u16, flow_keys->thoff,
 				   flow_keys->nhoff, hlen);
 
-	return result == BPF_OK;
+	return result;
 }
 
 static bool is_pppoe_ses_hdr_valid(const struct pppoe_hdr *hdr)
@@ -1008,6 +1008,7 @@ bool __skb_flow_dissect(const struct net *net,
 			};
 			__be16 n_proto = proto;
 			struct bpf_prog *prog;
+			u32 result;
 
 			if (skb) {
 				ctx.skb = skb;
@@ -1019,12 +1020,12 @@ bool __skb_flow_dissect(const struct net *net,
 			}
 
 			prog = READ_ONCE(run_array->items[0].prog);
-			ret = bpf_flow_dissect(prog, &ctx, n_proto, nhoff,
-					       hlen, flags);
+			result = bpf_flow_dissect(prog, &ctx, n_proto, nhoff,
+						  hlen, flags);
 			__skb_flow_bpf_to_target(&flow_keys, flow_dissector,
 						 target_container);
 			rcu_read_unlock();
-			return ret;
+			return result == BPF_OK;
 		}
 		rcu_read_unlock();
 	}
-- 
2.37.1

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

[Shmulik Ladkani]

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.

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,
 };
 
 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

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

[Stanislav Fomichev]

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
>

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

[Shmulik Ladkani]

On Thu, 18 Aug 2022 09:12:43 -0700
Stanislav Fomichev <sdf@google.com> wrote:

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

OK, will re-submit with a test.

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

Well, wouldn't that be too late now? there might be progs out there with
different codes. In any case, I don't think adding this is related to this
series.

Best,
Shmulik