| Message ID | 20220818062405.947643-3-shmulik.ladkani@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | flow_dissector: Allow bpf flow-dissector progs to request fallback to normal dissection | expand |
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 >
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
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(); }
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(+)