Avinash Duduskar <[email protected]> writes: > This series adds VLAN awareness to bpf_fib_lookup() in both directions. > BPF_FIB_LOOKUP_VLAN resolves a VLAN egress to its underlying real device > plus the VLAN tag (XDP programs need this because VLAN devices have no XDP > xmit), and BPF_FIB_LOOKUP_VLAN_INPUT runs the lookup as if a tagged frame > had arrived on the matching VLAN subinterface, for iif policy routing and > VRF table selection. > > The l3mdev/VRF flow-init fix that was patch 1 in v1 and v2 has been split > out and sent to bpf on its own, since it is an independent Fixes:-tagged > fix that routes to stable on its own schedule. This series is otherwise > independent of it: on the default CONFIG_INIT_STACK_ALL_ZERO the VRF > selftests pass with or without the fix. Only the one full-lookup VRF arm > ("IPv4 VLAN input, tag selects VRF table") depends on it, and only on > INIT_STACK_ALL_PATTERN or NONE builds, where the uninitialized > flowi_l3mdev otherwise misses the l3mdev rule and the lookup falls > through to the main table. Applying the l3mdev fix first closes that > window. > > Changes v2 -> v3 (all from Toke's review unless noted): > > - Split the l3mdev/VRF flow-init fix out to a standalone bpf submission > (it was patch 1 in v2). > > - Patch 2 (VLAN_INPUT): bpf_fib_vlan_input_dev() returns a > struct net_device * with ERR_PTR() for the -EINVAL case and NULL for > NOT_FWDED, instead of an int return and a **dev out-parameter. > > - Trim the BPF_FIB_LOOKUP_VLAN and BPF_FIB_LOOKUP_VLAN_INPUT UAPI doc > blocks, and drop the in-function comments that restated the commit > message or the flag doc. > > - Patch 1 (VLAN egress): on the skb path without tot_len, the deferred mtu > check now runs against the resolved egress (VLAN) device, not the parent > params->ifindex was swapped to, so a VLAN device with a smaller mtu than > its parent is no longer checked against, or reported as, the parent's > larger mtu. Found by the bpf ci bot; this was an open question in v2. > > - Patch 3 (selftests): re-run every case through bpf_xdp_fib_lookup() as > well, since the feature targets XDP; and flip the no-tot_len mtu arm to > expect the VLAN device's mtu after the fix above. > > Open questions (defaults chosen, noted here in case a maintainer > prefers otherwise): > > 1. An unmatched, down, or foreign-netns tag returns > BPF_FIB_LKUP_RET_NOT_FWDED, matching the DIRECT path when > fib_get_table() finds no table, rather than a new return code. > > 2. BPF_FIB_LOOKUP_OUTPUT | BPF_FIB_LOOKUP_VLAN_INPUT is rejected with > -EINVAL; restricting now keeps relaxing later backward-compatible. > > 3. The name BPF_FIB_LOOKUP_VLAN_INPUT reads oddly next to > BPF_FIB_LOOKUP_OUTPUT. A pair like _VLAN_EGRESS/_VLAN_INGRESS is an > option while nothing is merged.
These three are fine as-is, I think. > 4. The egress flag leaves a VLAN it cannot reduce to a physical parent > plus one tag (QinQ, or a parent in another namespace) as SUCCESS with > the VLAN device's ifindex and the vlan fields zero, like a plain > lookup. The input side instead fails closed (NOT_FWDED) on the > cross-namespace case. An XDP caller cannot xmit on a VLAN device, and > a zero h_vlan_proto does not distinguish this result from a physical > egress, so returning NOT_FWDED would be safer for XDP. But the two > cases differ: a foreign-netns parent is clearly fail-worthy, while a > QinQ egress is still a forwardable route (tc xmits on the inner VLAN > device), so failing it closed would reject a usable route. Should > egress signal NOT_FWDED, for both or only foreign-netns? I left it > best-effort, but will change it if you prefer. This one is a bit more ambiguous. Specifically, the inability for an XDP program to distinguish between a route that actually targets a physical device, and one that targets a VLAN device that couldn't be resolved for whatever reason. Since this is a new feature that's opt-in, I think I would lean towards failing lookups with a new error code (BPF_FIB_LKUP_RET_VLAN_FAILURE, say) if the lookup finds a VLAN device but can't actually resolve the parent. That way the XDP program can repeat the lookup without the BPF_FIB_LOOKUP_VLAN flag if it really wants the ifindex of that VLAN device, but that will be explicit and not hidden. -Toke
