Re: [PATCH v14 nf-next 3/3] netfilter: nft_chain_filter: Add bridge double vlan and pppoe

[Florian Westphal <fw@strlen.de>]

Eric Woudstra <ericwouds@gmail.com> wrote:

[ skb->protocl munging ]

> But in nft_do_chain_bridge() it is needed in the case of matching 'ip
> saddr', 'ip daddr', 'ip6 saddr' or 'ip6 daddr'. I suspect all ip/ip6
> matches are suffering.

Thats because of implicit dependency insertion on userspace side:
# ip saddr 1.2.3.4 counter ip daddr 3.4.5.6
bridge test-bridge input
  [ meta load protocol => reg 1 ]
  [ cmp eq reg 1 0x00000008 ]
  [ payload load 4b @ network header + 12 => reg 1 ]
  ...

So, if userspace would NOT do that it would 'just work'.

Pablo, whats your take on this?
We currently don't have a 'nhproto' field in nft_pktinfo
and there is no space to add one.

We could say that things work as expected, and that
 ip saddr 1.2.3.4

should not magically match packets in e.g. pppoe encap.
I suspect it will start to work if you force it to match in pppoe, e.g.
ether type 0x8864 ip saddr ...

so nft won't silently add the skb->protocol dependency.

Its not a technical issue but about how matching is supposed to work
in a bridge.

If its supposed to work automatically we need to either:
1. munge skb->protocol in kernel, even tough its wrong (we don't strip
   the l2 headers).
2. record the real l3 protocol somewhere and make it accessible, then
   fix the dependency generation in userspace to use the 'new way' (meta
   l3proto)?
3. change the dependency generation to something else.
   But what? 'ether type ip' won't work either for 8021ad etc.
   'ip version' can't be used for arp.

> I haven't found where yet, but It seems nft is checking skb->protocol,
> before it tries to match the ip(6) saddr/daddr.

Yes, userspace inserts this, see 'nft --debug=netlink add rule bridge ..'