Re: [PATCH] Bpf Helper Function BPF_FUNC_skb_change_dsfield
From: Yonghong Song <hidden>
Date: 2021-12-21 03:18:50
Also in:
bpf
On 12/20/21 12:40 PM, Tyler Wear wrote:
New bpf helper function BPF_FUNC_skb_change_dsfield "int bpf_skb_change_dsfield(struct sk_buff *skb, u8 mask, u8 value)". BPF_PROG_TYPE_CGROUP_SKB typed bpf_prog which currently can be attached to the ingress and egress path. The helper is needed because this type of bpf_prog cannot modify the skb directly. Used by a bpf_prog to specify DS field values on egress or ingress.
Maybe you can expand a little bit here for your use case? I know DS field might help but a description of your actual use case will make adding this helper more compelling.
quoted hunk ↗ jump to hunk
Signed-off-by: Tyler Wear <redacted> --- include/uapi/linux/bpf.h | 9 ++++++++ net/core/filter.c | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+)diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 556216dc9703..742cea7dcf8c 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h@@ -3742,6 +3742,14 @@ union bpf_attr { * Return * The helper returns **TC_ACT_REDIRECT** on success or * **TC_ACT_SHOT** on error. + * + * long bpf_skb_change_dsfield(struct sk_buff *skb, u8 mask, u8 value) + * Description + * Set DS field of IP header to the specified *value*. The *value* + * is masked with the provided *mask* when ds field is updated.
This description probably not precise. If I understand correctly, the *mask* is used to mask the current skb iph->tos value which is then 'or'ed with *value*. I am also debating the helper name, bpf_skb_change_dsfield vs. bpf_skb_update_dsfield vs. bpf_skb_update_ds. Here, we are actually doing an update instead of completely overwrite the original value. Maybe "update" is better than "change". Maybe we can just do "bpf_skb_update_ds"? We have an existing helper bpf_skb_ecn_set_ce to update ecn. We don't have any helper with suffix "field".
quoted hunk ↗ jump to hunk
+ * Works with IPv6 and IPv4. + * Return + * 1 if the DS field is set, 0 if it is not set. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \@@ -3900,6 +3908,7 @@ union bpf_attr { FN(per_cpu_ptr), \ FN(this_cpu_ptr), \ FN(redirect_peer), \ + FN(skb_change_dsfield), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helperdiff --git a/net/core/filter.c b/net/core/filter.c index 035d66227ae2..71ea943c8059 100644 --- a/net/core/filter.c +++ b/net/core/filter.c@@ -6402,6 +6402,50 @@ BPF_CALL_1(bpf_skb_ecn_set_ce, struct sk_buff *, skb) return INET_ECN_set_ce(skb); } +BPF_CALL_3(bpf_skb_change_dsfield, struct sk_buff *, skb, u8, mask, u8, value) +{ + unsigned int iphdr_len; + + switch (skb_protocol(skb, true)) { + case cpu_to_be16(ETH_P_IP): + iphdr_len = sizeof(struct iphdr); + break; + case cpu_to_be16(ETH_P_IPV6): + iphdr_len = sizeof(struct ipv6hdr); + break; + default: + return 0; + } + + if (skb_headlen(skb) < iphdr_len) + return 0; + + if (skb_cloned(skb) && !skb_clone_writable(skb, iphdr_len)) + return 0; + + switch (skb_protocol(skb, true)) { + case cpu_to_be16(ETH_P_IP): + ipv4_change_dsfield(ipip_hdr(skb), mask, value); + break; + case cpu_to_be16(ETH_P_IPV6): + ipv6_change_dsfield(ipv6_hdr(skb), mask, value); + break; + default: + return 0; + }
There are some repetition here. For example, in the above, 'default' is not possible at all. Is it possible to remove the second 'switch' statement with simple if (...) ipv4_change_dsfield(ipip_hdr(skb), mask, value); else ipv6_change_dsfield(ipv6_hdr(skb), mask, value); ?
quoted hunk ↗ jump to hunk
+ + return 1; +} + +static const struct bpf_func_proto bpf_skb_change_dsfield_proto = { + .func = bpf_skb_change_dsfield, + .gpl_only = false, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_CTX, + .arg2_type = ARG_ANYTHING, + .arg3_type = ARG_ANYTHING, +}; + bool bpf_xdp_sock_is_valid_access(int off, int size, enum bpf_access_type type, struct bpf_insn_access_aux *info) {@@ -7057,6 +7101,8 @@ cg_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_get_listener_sock_proto; case BPF_FUNC_skb_ecn_set_ce: return &bpf_skb_ecn_set_ce_proto; + case BPF_FUNC_skb_change_dsfield: + return &bpf_skb_change_dsfield_proto;
We do need a self test to exercise this helper.
#endif default: return sk_filter_func_proto(func_id, prog);