Re: [PATCH bpf-next v5 1/3] bpf: Add BPF_FIB_LOOKUP_VLAN flag to bpf_fib_lookup() helper

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Avinash Duduskar <avinash.duduskar@gmail.com> writes:

>> > +	if (flags & BPF_FIB_LOOKUP_VLAN)
>> > +		return -EINVAL;
>> > +
>>
>> This is fine, but we should probably reject the input flag as well in
>> the next patch (for symmetry).
>
> I dug into this and I don't think the two are symmetric. The egress
> reject is right for exactly the reason you gave: in tc you can redirect
> to the VLAN device directly, so reducing the egress to the physical
> parent is only needed for XDP. But that is a transmit argument, and
> VLAN_INPUT never touches the egress or redirect side. It only sets the
> lookup's ingress (flowi_iif), which picks the iif policy rule and the VRF
> table, and there is no XDP-only constraint there for the symmetry to
> mirror.
>
> tc also has a real user for it. In __netif_receive_skb_core() the tcx
> ingress hook runs before vlan_do_receive() demuxes the frame, so a clsact
> program on the physical port sees a tagged frame with skb->dev still the
> physical device and the tag in skb->vlan_tci. That is exactly the
> physical-ifindex-plus-tag input VLAN_INPUT takes, and it wants the
> subinterface-scoped answer. The 2/3 selftest already runs the VLAN_INPUT
> cases on the tc path, including the VRF-table-selection ones, and they
> pass, so this isn't a theoretical tc path.
>
> So I would keep VLAN_INPUT allowed on both. If you would rather hold a
> uniform "both VLAN flags are XDP-only" line under the same
> restrict-now-relax-later rule as the TBID and OUTPUT combos, I will add
> the reject, but unlike the egress case it removes a working tc path
> rather than one tc cannot use. Happy either way, just let me know.

Hmm, OK, I'm fine with keeping it available for TC then.

-Toke