On 2/8/21 5:27 PM, Jesper Dangaard Brouer wrote:
On Mon, 8 Feb 2021 16:41:24 +0100
Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
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.
I would really really like to make progress for this patchset. I
unfortunately finished coding this up (and tested with selftests)
before I noticed this request (without optimizations).
I guess, I can revert my recent work by pulling in V12 of the patch.
I'll do it tomorrow, as I want to have time to run my tests before
re-sending patchset.
I'm okay either way - it was just a suggestion and not a request, of course.
If you already have it ready in the meantime, that is also not a problem, no
need to revert again.
Thanks,
Daniel
