Re: [PATCH ipsec-next,v2 2/3] xfrm: interface: Add unstable helpers for setting/getting XFRM metadata from TC-BPF

[Martin KaFai Lau <martin.lau@linux.dev>]

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@gmail.com> wrote:
>>
>> Hi Martin,
>>
>> On Wed, Nov 30, 2022 at 8:15 PM Martin KaFai Lau <martin.lau@linux.dev> 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.