Re: [PATCH bpf v2 1/2] bpf: Derive source IP addr via bpf_*_fib_lookup()

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

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.