On 12/1/22 5:30 AM, Eyal Birger wrote:
Hi Martin, On Thu, Dec 1, 2022 at 7:55 AM Eyal Birger <eyal.birger@xxxxxxxxx> wrote:Hi Martin, On Wed, Nov 30, 2022 at 8:15 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:On 11/29/22 5:20 AM, Eyal Birger wrote:diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_interface_bpf.c new file mode 100644 index 000000000000..757e15857dbf --- /dev/null +++ b/net/xfrm/xfrm_interface_bpf.c @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Unstable XFRM Helpers for TC-BPF hook + * + * These are called from SCHED_CLS BPF programs. Note that it is + * allowed to break compatibility for these functions since the interface they + * are exposed through to BPF programs is explicitly unstable. + */ + +#include <linux/bpf.h> +#include <linux/btf_ids.h> + +#include <net/dst_metadata.h> +#include <net/xfrm.h> + +struct bpf_xfrm_info {No need to introduce a bpf variant of the "struct xfrm_md_info" (more on this later).+ u32 if_id; + int link; +}; + +static struct metadata_dst __percpu *xfrm_md_dst; +__diag_push(); +__diag_ignore_all("-Wmissing-prototypes", + "Global functions as their definitions will be in xfrm_interface BTF"); + +__used noinline +int bpf_skb_get_xfrm_info(struct __sk_buff *skb_ctx, struct bpf_xfrm_info *to)This kfunc is not needed. It only reads the skb->_skb_refdst. The new kfunc bpf_rdonly_cast() can be used. Take a look at the bpf_rdonly_cast() usages in the selftests/bpf/progs/type_cast.c. It was in bpf-next only but should also be in net-next now.I'm somewhat concerned with this approach. Indeed it would remove the kfunc, and the API is declared "unstable", but still the implementation as dst isn't relevant to the user and would make the programs less readable. Also note that the helper can be also used as it is to get the xfrm info at egress from an lwt route (which stores the xfrm_info in the dst lwstate).
Right, the whole skb_xfrm_md_info() can be implemented in bpf prog itself, like checking lwtstate.
If adding a kfunc, how about directly expose skb_xfrm_md_info() itself as a kfunc to bpf prog and directly return a "struct xfrm_md_info *" instead. Then there is no need to copy if_id/link...etc. The bpf prog has no need to initialize the "to" also. Something like this:
__used noinline
const struct xfrm_md_info *bpf_skb_xfrm_md_info(const struct __sk_buff *skb) { ... }
BTF_ID_FLAGS(func, bpf_skb_xfrm_md_info, KF_RET_NULL)
+{ + struct sk_buff *skb = (struct sk_buff *)skb_ctx; + struct xfrm_md_info *info; + + memset(to, 0, sizeof(*to)); + + info = skb_xfrm_md_info(skb); + if (!info) + return -EINVAL; + + to->if_id = info->if_id; + to->link = info->link; + return 0; +} + +__used noinline +int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx, + const struct bpf_xfrm_info *from)Directly use "const struct xfrm_md_info *from" instead. This kfunc can check from->dst_orig != NULL and return -EINVAL. It will then have a consistent API with the bpf_rdonly_cast() mentioned above.See above.Also, when trying this approach with bpf_set_xfrm_info() accepting "const struct xfrm_md_info *from" I fail to load the program: libbpf: prog 'set_xfrm_info': BPF program load failed: Invalid argument libbpf: prog 'set_xfrm_info': -- BEGIN PROG LOAD LOG -- 0: R1=ctx(off=0,imm=0) R10=fp0 ; int set_xfrm_info(struct __sk_buff *skb) 0: (bf) r6 = r1 ; R1=ctx(off=0,imm=0) R6_w=ctx(off=0,imm=0) 1: (b7) r1 = 0 ; R1_w=0 ; struct xfrm_md_info info = {}; 2: (7b) *(u64 *)(r10 -8) = r1 ; R1_w=0 R10=fp0 fp-8_w=00000000 3: (7b) *(u64 *)(r10 -16) = r1 ; R1_w=0 R10=fp0 fp-16_w=00000000 4: (b4) w1 = 0 ; R1_w=0 ; __u32 index = 0; 5: (63) *(u32 *)(r10 -20) = r1 ; R1_w=0 R10=fp0 fp-24=0000???? 6: (bf) r2 = r10 ; R2_w=fp0 R10=fp0 ; 7: (07) r2 += -20 ; R2_w=fp-20 ; if_id = bpf_map_lookup_elem(&dst_if_id_map, &index); 8: (18) r1 = 0xffff888006751c00 ; R1_w=map_ptr(off=0,ks=4,vs=4,imm=0) 10: (85) call bpf_map_lookup_elem#1 ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=4,imm=0) 11: (bf) r1 = r0 ; R0_w=map_value_or_null(id=1,off=0,ks=4,vs=4,imm=0) R1_w=map_value_or_null(id=1,off=0,ks=4,vs=4,imm=0) 12: (b4) w0 = 2 ; R0_w=2 ; if (!if_id) 13: (15) if r1 == 0x0 goto pc+10 ; R1_w=map_value(off=0,ks=4,vs=4,imm=0) 14: (bf) r2 = r10 ; R2_w=fp0 R10=fp0 ; 15: (07) r2 += -16 ; R2_w=fp-16 ; info.if_id = *if_id; 16: (61) r1 = *(u32 *)(r1 +0) ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) ; info.if_id = *if_id; 17: (63) *(u32 *)(r2 +0) = r1 ; R1_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R2_w=fp-16 fp-16_w= ; ret = bpf_skb_set_xfrm_info(skb, &info); 18: (bf) r1 = r6 ; R1_w=ctx(off=0,imm=0) R6_w=ctx(off=0,imm=0) 19: (85) call bpf_skb_set_xfrm_info#81442 arg#1 pointer type STRUCT xfrm_md_info must point to scalar, or struct with scalar Is there some registration I need to do for this struct?
Ah, thanks for trying!hmm... it will need a change to the verifier. likely tag the param with something like "const struct xfrm_md_info *from__nonscalar_ok".
The reason of my earlier suggestion was to avoid the need to duplicate future changes in xfrm_md_info to bpf_xfrm_info and more importantly avoid creating another __sk_buff vs sk_buff like usage confusion.
For now, lets stay with bpf_xfrm_info. This can be changed later.
