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

Eric Woudstra <[email protected]> wrote:
> In nft_do_chain_bridge() pktinfo is only fully populated for plain packets
> and packets encapsulated in single 802.1q or 802.1ad.
> 
> When implementing the software bridge-fastpath and testing all possible
> encapulations, there can be more encapsulations:
> 
> The packet could (also) be encapsulated in PPPoE, or the packet could be
> encapsulated in an inner 802.1q, combined with an outer 802.1ad or 802.1q
> encapsulation.
> 
> nft_flow_offload_eval() also examines the L4 header, with the L4 protocol
> known from the conntrack-tuplehash. To access the header it uses
> nft_thoff(), but for these packets it returns zero.
> 
> Introduce nft_set_bridge_pktinfo() to help populate pktinfo with the
> offsets, without setting pkt->tprot and the corresponding pkt->flags.
> 
> This will not change rule processing, but does make these offsets
> available for code that is not checking pkt->flags to use the offsets,
> like nft_flow_offload_eval().

Thanks.  I think this is fine and we can extend/change this later on.

> +	case htons(ETH_P_8021Q): {
> +		struct vlan_hdr *vhdr;
> +
> +		if (!pskb_may_pull(skb, VLAN_HLEN))
> +			return -1;
> +		vhdr = (struct vlan_hdr *)(skb->data);
> +		*proto = vhdr->h_vlan_encapsulated_proto;
> +		skb_set_network_header(skb, VLAN_HLEN);

Could you move the skb_set_network_header() calls
to places where we know that we have an ip/ipv6 proto
in the upper header?

You already return the length of the encap header
anyway, might as well restrict the skb nh update
to when it will be useful.

Or was there another reason to do it this way?

If you prefer you can only resend this patch, I believe there
are two different use cases:

This patch as requirement for future ft offload of ip/ip6 packets
in bridged pppoe/qinq and this patch as starting point to eventually
allow 'ip saddr 1.2.3.4' to auto-match for PPPoE, Vlan, etc.