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] [thread-next>] [day] [month] [year] [list]

Message-ID: <47294480-506a-e22e-7466-3cdc106c395e@linux.dev>
Date: Thu, 5 Oct 2023 23:29:50 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Martynas <m@...bda.lt>
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 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:

			*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).

> 
>>
>> 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.