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.
