netdev - Re: [PATCH bpf v2 1/2] bpf: Derive source IP addr via bpf_*_fib

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <daa2a632-a100-4a34-9584-44506d6a39f0@app.fastmail.com>
Date: Fri, 06 Oct 2023 09:03:01 +0200
From: Martynas <m@...bda.lt>
To: "Martin KaFai Lau" <martin.lau@...ux.dev>
Cc: "Daniel Borkmann" <daniel@...earbox.net>, netdev <netdev@...r.kernel.org>,
 "Nikolay Aleksandrov" <razor@...ckwall.org>, bpf@...r.kernel.org
Subject: Re: [PATCH bpf v2 1/2] bpf: Derive source IP addr via bpf_*_fib_lookup()



On Fri, Oct 6, 2023, at 8:29 AM, Martin KaFai Lau wrote:
> On 10/5/23 1:16 PM, Martynas wrote:
>>>> @@ -5992,6 +5995,19 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
>>>>    	params->rt_metric = res.f6i->fib6_metric;
>>>>    	params->ifindex = dev->ifindex;
>>>>    
>>>> +	if (flags & BPF_FIB_LOOKUP_SET_SRC) {
>>>> +		if (res.f6i->fib6_prefsrc.plen) {
>>>> +			*(struct in6_addr *)params->ipv6_src = res.f6i->fib6_prefsrc.addr;
>
> A nit. just noticed. Similar to the "*dst" assignment a few lines above:

SGTM.

>
> 			*src = res.f6i->fib6_prefsrc.addr;
>
>>>> +		} else {
>>>> +			err = ipv6_bpf_stub->ipv6_dev_get_saddr(net, dev,
>>>> +								&fl6.daddr, 0,
>>>> +								(struct in6_addr *)
>>>> +								params->ipv6_src);
>
> Same here. Use the "src".
>
>>>> +			if (err)
>>>> +				return BPF_FIB_LKUP_RET_NO_SRC_ADDR;
>>>
>>> This error also implies BPF_FIB_LKUP_RET_NO_NEIGH. I don't have a clean way of
>>> improving the API. May be others have some ideas.
>>>
>>> Considering dev has no saddr is probably (?) an unlikely case, it should be ok
>>> to leave it as is but at least a comment in the uapi will be needed. Otherwise,
>>> the bpf prog may use the 0 dmac as-is.
>> 
>> I expect that a user of the helper checks that err == 0 before using any of the output params.
>
> For example, the bpf prog gets BPF_FIB_LKUP_RET_NO_NEIGH and learns 
> neigh is not 
> available but ipv6_dst (and the optional ipv6_src) is still valid.
>
> If the bpf prog gets BPF_FIB_LKUP_RET_NO_SRC_ADDR, intuitively, only 
> ipv6_src is 
> not available. The bpf prog will continue to use the ipv6_dst and dmac 
> (which is 
> actually 0).
>

Thinking out loud, we could make BPF_FIB_LOOKUP_SRC to require BPF_FIB_LOOKUP_SKIP_NEIGH, but then for some cases a user would be required to call the helper twice. This is a no-go due perf and instruction count reasons.

Nothing betters comes than explicitly documenting the behavior in the uapi comments.

>> 
>>>
>>> I feel the current bpf_ipv[46]_fib_lookup helper is doing many things
>>> in one
>>> function and then requires different BPF_FIB_LOOKUP_* bits to select
>>> what/how to
>>> do. In the future, it may be worth to consider breaking it into smaller
>>> kfunc(s). e.g. the __ipv[46]_neigh_lookup could be in its own kfunc.
>>>
>> 
>> Yep, good idea. At least it seems that the neigh lookup could live in its own function.
>
> To be clear, it could be independent of this set.
>
> Thanks.