| Message ID | 20201008145314.116800-1-toke@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] bpf_fib_lookup: return target ifindex even if neighbour lookup fails | expand |
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
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.
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
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
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.
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
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.
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.
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(-)