| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f4aa0e19-6c91-3d39-ca8a-7840bf46625d@linux.dev>
Date: Thu, 1 Dec 2022 12:18:45 -0800
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Eyal Birger <eyal.birger@...il.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org,
linux-kselftest@...r.kernel.org, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
steffen.klassert@...unet.com, herbert@...dor.apana.org.au,
andrii@...nel.org, daniel@...earbox.net, nicolas.dichtel@...nd.com,
razor@...ckwall.org, mykolal@...com, ast@...nel.org,
song@...nel.org, yhs@...com, john.fastabend@...il.com,
kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
jolsa@...nel.org, shuah@...nel.org
Subject: Re: [PATCH ipsec-next,v2 2/3] xfrm: interface: Add unstable helpers
for setting/getting XFRM metadata from TC-BPF
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@...il.com> wrote:
>>
>> Hi Martin,
>>
>> On Wed, Nov 30, 2022 at 8:15 PM Martin KaFai Lau <martin.lau@...ux.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.
Powered by blists - more mailing lists