Re: bug report: MAC src + protocol optiomization failing with 802.1Q frames

[Florian Westphal <fw@strlen.de>]

Antoine C. <acalando@free.fr> wrote:
> ----- Florian Westphal <fw@strlen.de> a écrit :
> > Antoine C. <acalando@free.fr> wrote:
> > > 
> > > This bug does not seem to get a lot of attention but may be it
> > > deserves at least to be filed ?
> > 
> > Its a design bug and so far not a single solution was
> > presented.
> > 
> > And no one answered any of the questions that I asked.
> > 
> > So, whats YOUR opinion?
> > 
> > > > Should "ip saddr 1.2.3.4" match:
> > > > 
> > > > Only in classic ethernet case?
> > > > In VLAN?
> > > > In QinQ?
> > > > 
> > > > What about IP packet in a PPPOE frame?
> > > > What about other L2 protocols?
> 
> As a user, my answer would be of course that "ip xxx" rules 
> work seamlessly whatever the encapsulation is above. I have

Thanks.  I believe that this is the way to go, i.e., if user asks
to match L3, then we should be greedy and attempt to provide the
relevant context.

TL;DR: I think
https://patchwork.ozlabs.org/project/netfilter-devel/patch/20251104145728.517197-4-ericwouds@gmail.com/

is the way to go with only minor changes needed.

Unfortunately this isn't as easy as it sounds.
Lets see where we are:

When user asks for 'ip saddr 1.2.3.4'. we must provide context
for matching to work in first place.

1. Kernel doesn't always know where to start the matching (the network
header offset in this case).  For ethernet and vlan the kernel will
know.

But for other procotols on top of ethernet that might not be the case
especially if the kernel is only forwarding frames.

To make 'ip saddr' work on bridge with other encapsulations, e.g. PPPoE,
this needs kernel changes.

That also means that existing rules behave differently when updating
kernel with new L2 encap protocol support.

2. Even if kernel knows where in the frame the network header starts,
'ip saddr 1.2.3.4' must not match e.g. an ipv6 packet.

This is solved (or rather, supposed to be solved) by nftables userspace:
For bridge family nft will insert a protocol dependency check
internally, in the given example it is
'meta load protocol;cmp eq 0x0800'.

- It excludes non-ipv4 (no false matches). This is wanted behavior.
- It includes VLAN encapsulated frames because kernel (by default)
  removes vlan headers and thus replaces the 0x8100 vlan type with
  the upper protocol.
- If kernel is configured to not do that, then this will only match
  normal (non-vlan) ipv4 ethernet frames.

If we want to make 'ip saddr 1.2.3.4' always match for netdev and bridge
families, then we need to make changes on both userspace and/or kernel:

- kernel must, for bridge and netdev chains, follow the L2 encap trail
  until it finds ip/ip6 protocol or it can't follow any further.

 This means extension to also skip PPPoE, q-in-q and so on and would
 be largely identical to what Eric Woudstras patches already do.

Downside: Behaviour change on kernel upgrade: packets that were
not matched before now will be, UNLESS we don't update
skb->protocol value and e.g. add a new l3proto field to nft_pktinfo.

That in turn is also problematic, because it means that we'd have to
add a new 'meta l3proto' that can extract this from nft_pktinfo
as a replacement of 'meta protocol'.

Semantics would be:
meta protocol == skb->protocol

meta l3proto == "last" ETH_P_ that kernel could figure out.
It is similar to 'ip6 nexthdr' vs. 'meta l4proto';
former is just the first next header value, the latter the last
protocol header with all extension headers, if any, skipped.

The problem is that, given its a new extension, we can't update
nftables to use it for implicit dependencies; else rule add
fails on older kernels.

If we ignore that and pretend we could change this, then
the behaviour of:

ip saddr 1.2.3.4 (with meta l3proto dep)
ip saddr 1.2.3.4 (with current meta protocol)

... are identical between the two options
(i.e., follow dependency trail and update skb->protocol to last
 known ETH_P value) resp. 'stash last known ETH_P in nft_pktinfo)
from user perspective.

That would also mean that nftables netdev family should be
fixed to follow the bridge dependency approach consistently
if possible.