[PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[Toke Høiland-Jørgensen]

The bpf_fib_lookup() helper performs a neighbour lookup for the destination
IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
that the BPF program will pass the packet up the stack in this case.
However, with the addition of bpf_redirect_neigh() that can be used instead
to perform the neighbour lookup.

However, for that we still need the target ifindex, and since
bpf_fib_lookup() already has that at the time it performs the neighbour
lookup, there is really no reason why it can't just return it in any case.
With this fix, a BPF program can do the following to perform a redirect
based on the routing table that will succeed even if there is no neighbour
entry:

	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);

		return bpf_redirect(fib_params.ifindex, 0);
	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
		return bpf_redirect_neigh(fib_params.ifindex, 0);
	}

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/core/filter.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 05df73780dd3..00fce34a2204 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5192,7 +5192,6 @@ static int bpf_fib_set_fwd_params(struct bpf_fib_lookup *params,
 	memcpy(params->smac, dev->dev_addr, ETH_ALEN);
 	params->h_vlan_TCI = 0;
 	params->h_vlan_proto = 0;
-	params->ifindex = dev->ifindex;
 
 	return 0;
 }
@@ -5289,6 +5288,7 @@ static int bpf_ipv4_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 	dev = nhc->nhc_dev;
 
 	params->rt_metric = res.fi->fib_priority;
+	params->ifindex = dev->ifindex;
 
 	/* xdp and cls_bpf programs are run in RCU-bh so
 	 * rcu_read_lock_bh is not needed here
@@ -5414,6 +5414,7 @@ static int bpf_ipv6_fib_lookup(struct net *net, struct bpf_fib_lookup *params,
 
 	dev = res.nh->fib_nh_dev;
 	params->rt_metric = res.f6i->fib6_metric;
+	params->ifindex = dev->ifindex;
 
 	/* xdp and cls_bpf programs are run in RCU-bh so rcu_read_lock_bh is
 	 * not needed here.
-- 
2.28.0

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[Daniel Borkmann]

On 10/8/20 4:53 PM, Toke Høiland-Jørgensen wrote:
> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
> that the BPF program will pass the packet up the stack in this case.
> However, with the addition of bpf_redirect_neigh() that can be used instead
> to perform the neighbour lookup.
> 
> However, for that we still need the target ifindex, and since
> bpf_fib_lookup() already has that at the time it performs the neighbour
> lookup, there is really no reason why it can't just return it in any case.
> With this fix, a BPF program can do the following to perform a redirect
> based on the routing table that will succeed even if there is no neighbour
> entry:
> 
> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
> 
> 		return bpf_redirect(fib_params.ifindex, 0);
> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
> 	}
> 
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>

ACK, this looks super useful! Could you also add a new flag which would skip
neighbor lookup in the helper while at it (follow-up would be totally fine from
my pov since both are independent from each other)?

Thanks,
Daniel

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[Toke Høiland-Jørgensen]

Daniel Borkmann <daniel@iogearbox.net> writes:

> On 10/8/20 4:53 PM, Toke Høiland-Jørgensen wrote:
>> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
>> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
>> that the BPF program will pass the packet up the stack in this case.
>> However, with the addition of bpf_redirect_neigh() that can be used instead
>> to perform the neighbour lookup.
>> 
>> However, for that we still need the target ifindex, and since
>> bpf_fib_lookup() already has that at the time it performs the neighbour
>> lookup, there is really no reason why it can't just return it in any case.
>> With this fix, a BPF program can do the following to perform a redirect
>> based on the routing table that will succeed even if there is no neighbour
>> entry:
>> 
>> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
>> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
>> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
>> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
>> 
>> 		return bpf_redirect(fib_params.ifindex, 0);
>> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
>> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
>> 	}
>> 
>> Cc: David Ahern <dsahern@gmail.com>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>
> ACK, this looks super useful! Could you also add a new flag which would skip
> neighbor lookup in the helper while at it (follow-up would be totally fine from
> my pov since both are independent from each other)?

Sure, can do. Thought about adding it straight away, but wasn't sure if
it would be useful, since the fib lookup has already done quite a lot of
work by then. But if you think it'd be useful, I can certainly add it.
I'll look at this tomorrow; if you merge this before then I'll do it as
a follow-up, and if not I'll respin with it added. OK? :)

-Toke

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[Daniel Borkmann]

On 10/8/20 10:59 PM, Toke Høiland-Jørgensen wrote:
> Daniel Borkmann <daniel@iogearbox.net> writes:
>> On 10/8/20 4:53 PM, Toke Høiland-Jørgensen wrote:
>>> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
>>> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
>>> that the BPF program will pass the packet up the stack in this case.
>>> However, with the addition of bpf_redirect_neigh() that can be used instead
>>> to perform the neighbour lookup.
>>>
>>> However, for that we still need the target ifindex, and since
>>> bpf_fib_lookup() already has that at the time it performs the neighbour
>>> lookup, there is really no reason why it can't just return it in any case.
>>> With this fix, a BPF program can do the following to perform a redirect
>>> based on the routing table that will succeed even if there is no neighbour
>>> entry:
>>>
>>> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
>>> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
>>> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
>>> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
>>>
>>> 		return bpf_redirect(fib_params.ifindex, 0);
>>> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
>>> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
>>> 	}
>>>
>>> Cc: David Ahern <dsahern@gmail.com>
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>
>> ACK, this looks super useful! Could you also add a new flag which would skip
>> neighbor lookup in the helper while at it (follow-up would be totally fine from
>> my pov since both are independent from each other)?
> 
> Sure, can do. Thought about adding it straight away, but wasn't sure if
> it would be useful, since the fib lookup has already done quite a lot of
> work by then. But if you think it'd be useful, I can certainly add it.
> I'll look at this tomorrow; if you merge this before then I'll do it as
> a follow-up, and if not I'll respin with it added. OK? :)

Sounds good to me; merge depending on David's final verdict in the other thread
wrt commit description.

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[Toke Høiland-Jørgensen]

Daniel Borkmann <daniel@iogearbox.net> writes:

n> On 10/8/20 10:59 PM, Toke Høiland-Jørgensen wrote:
>> Daniel Borkmann <daniel@iogearbox.net> writes:
>>> On 10/8/20 4:53 PM, Toke Høiland-Jørgensen wrote:
>>>> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
>>>> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
>>>> that the BPF program will pass the packet up the stack in this case.
>>>> However, with the addition of bpf_redirect_neigh() that can be used instead
>>>> to perform the neighbour lookup.
>>>>
>>>> However, for that we still need the target ifindex, and since
>>>> bpf_fib_lookup() already has that at the time it performs the neighbour
>>>> lookup, there is really no reason why it can't just return it in any case.
>>>> With this fix, a BPF program can do the following to perform a redirect
>>>> based on the routing table that will succeed even if there is no neighbour
>>>> entry:
>>>>
>>>> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
>>>> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
>>>> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
>>>> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
>>>>
>>>> 		return bpf_redirect(fib_params.ifindex, 0);
>>>> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
>>>> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
>>>> 	}
>>>>
>>>> Cc: David Ahern <dsahern@gmail.com>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>
>>> ACK, this looks super useful! Could you also add a new flag which would skip
>>> neighbor lookup in the helper while at it (follow-up would be totally fine from
>>> my pov since both are independent from each other)?
>> 
>> Sure, can do. Thought about adding it straight away, but wasn't sure if
>> it would be useful, since the fib lookup has already done quite a lot of
>> work by then. But if you think it'd be useful, I can certainly add it.
>> I'll look at this tomorrow; if you merge this before then I'll do it as
>> a follow-up, and if not I'll respin with it added. OK? :)
>
> Sounds good to me; merge depending on David's final verdict in the other thread
> wrt commit description.

Yup, figured that'd be the case - great :)

-Toke

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[David Ahern]

On 10/8/20 7:53 AM, Toke Høiland-Jørgensen wrote:
> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
> that the BPF program will pass the packet up the stack in this case.
> However, with the addition of bpf_redirect_neigh() that can be used instead
> to perform the neighbour lookup.
> 
> However, for that we still need the target ifindex, and since
> bpf_fib_lookup() already has that at the time it performs the neighbour
> lookup, there is really no reason why it can't just return it in any case.
> With this fix, a BPF program can do the following to perform a redirect
> based on the routing table that will succeed even if there is no neighbour
> entry:
> 
> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
> 
> 		return bpf_redirect(fib_params.ifindex, 0);
> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
> 	}
> 

There are a lot of assumptions in this program flow and redundant work.
fib_lookup is generic and allows the caller to control the input
parameters. direct_neigh does a fib lookup based on network header data
from the skb.

I am fine with the patch, but users need to be aware of the subtle details.

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[Toke Høiland-Jørgensen]

David Ahern <dsahern@gmail.com> writes:

> On 10/8/20 7:53 AM, Toke Høiland-Jørgensen wrote:
>> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
>> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
>> that the BPF program will pass the packet up the stack in this case.
>> However, with the addition of bpf_redirect_neigh() that can be used instead
>> to perform the neighbour lookup.
>> 
>> However, for that we still need the target ifindex, and since
>> bpf_fib_lookup() already has that at the time it performs the neighbour
>> lookup, there is really no reason why it can't just return it in any case.
>> With this fix, a BPF program can do the following to perform a redirect
>> based on the routing table that will succeed even if there is no neighbour
>> entry:
>> 
>> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
>> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
>> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
>> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
>> 
>> 		return bpf_redirect(fib_params.ifindex, 0);
>> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
>> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
>> 	}
>> 
>
> There are a lot of assumptions in this program flow and redundant work.
> fib_lookup is generic and allows the caller to control the input
> parameters. direct_neigh does a fib lookup based on network header data
> from the skb.
>
> I am fine with the patch, but users need to be aware of the subtle details.

Yeah, I'm aware they are not equivalent; the code above was just meant
as a minimal example motivating the patch. If you think it's likely to
confuse people to have this example in the commit message, I can remove
it?

-Toke

Re: [PATCH bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails

[David Ahern]

On 10/8/20 1:57 PM, Toke Høiland-Jørgensen wrote:
> David Ahern <dsahern@gmail.com> writes:
> 
>> On 10/8/20 7:53 AM, Toke Høiland-Jørgensen wrote:
>>> The bpf_fib_lookup() helper performs a neighbour lookup for the destination
>>> IP and returns BPF_FIB_LKUP_NO_NEIGH if this fails, with the expectation
>>> that the BPF program will pass the packet up the stack in this case.
>>> However, with the addition of bpf_redirect_neigh() that can be used instead
>>> to perform the neighbour lookup.
>>>
>>> However, for that we still need the target ifindex, and since
>>> bpf_fib_lookup() already has that at the time it performs the neighbour
>>> lookup, there is really no reason why it can't just return it in any case.
>>> With this fix, a BPF program can do the following to perform a redirect
>>> based on the routing table that will succeed even if there is no neighbour
>>> entry:
>>>
>>> 	ret = bpf_fib_lookup(skb, &fib_params, sizeof(fib_params), 0);
>>> 	if (ret == BPF_FIB_LKUP_RET_SUCCESS) {
>>> 		__builtin_memcpy(eth->h_dest, fib_params.dmac, ETH_ALEN);
>>> 		__builtin_memcpy(eth->h_source, fib_params.smac, ETH_ALEN);
>>>
>>> 		return bpf_redirect(fib_params.ifindex, 0);
>>> 	} else if (ret == BPF_FIB_LKUP_RET_NO_NEIGH) {
>>> 		return bpf_redirect_neigh(fib_params.ifindex, 0);
>>> 	}
>>>
>>
>> There are a lot of assumptions in this program flow and redundant work.
>> fib_lookup is generic and allows the caller to control the input
>> parameters. direct_neigh does a fib lookup based on network header data
>> from the skb.
>>
>> I am fine with the patch, but users need to be aware of the subtle details.
> 
> Yeah, I'm aware they are not equivalent; the code above was just meant
> as a minimal example motivating the patch. If you think it's likely to
> confuse people to have this example in the commit message, I can remove
> it?
> 

I would remove it. Any samples or tests in the kernel repo doing those
back-to-back should have a caveat.