On 2/8/21 4:20 PM, Jesper Dangaard Brouer wrote:
On Mon, 8 Feb 2021 14:57:13 +0100
Jesper Dangaard Brouer <brouer@xxxxxxxxxx> wrote:
On Fri, 5 Feb 2021 01:06:35 +0100
Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
On 2/2/21 5:26 PM, Jesper Dangaard Brouer wrote:
BPF end-user on Cilium slack-channel (Carlo Carraro) wants to use
bpf_fib_lookup for doing MTU-check, but *prior* to extending packet size,
by adjusting fib_params 'tot_len' with the packet length plus the expected
encap size. (Just like the bpf_check_mtu helper supports). He discovered
that for SKB ctx the param->tot_len was not used, instead skb->len was used
(via MTU check in is_skb_forwardable() that checks against netdev MTU).
Fix this by using fib_params 'tot_len' for MTU check. If not provided (e.g.
zero) then keep existing TC behaviour intact. Notice that 'tot_len' for MTU
check is done like XDP code-path, which checks against FIB-dst MTU.
[...]
- if (!rc) {
- struct net_device *dev;
-
- dev = dev_get_by_index_rcu(net, params->ifindex);
+ if (rc == BPF_FIB_LKUP_RET_SUCCESS && !check_mtu) {
+ /* When tot_len isn't provided by user,
+ * check skb against net_device MTU
+ */
if (!is_skb_forwardable(dev, skb))
rc = BPF_FIB_LKUP_RET_FRAG_NEEDED;
... so using old cached dev from above will result in wrong MTU check &
subsequent passing of wrong params->mtu_result = dev->mtu this way.
Yes, you are right, params->ifindex have a chance to change in the calls.
So, our attempt to save an ifindex lookup (dev_get_by_index_rcu) is not
correct.
So one
way to fix is that we would need to pass &dev to bpf_ipv{4,6}_fib_lookup().
Ok, I will try to code it up, and see how ugly it looks, but I'm no
longer sure that it is worth saving this ifindex lookup, as it will
only happen if BPF-prog didn't specify params->tot_len.
I guess we can still do this as an "optimization", but the dev/ifindex
will very likely be another at this point.
I would say for sake of progress, lets ship your series w/o this optimization so
it can land, and we revisit this later on independent from here. Actually DavidA
back then acked the old poc patch I posted, so maybe that's worth a revisit as
well but needs more testing first.
Thanks,
Daniel
