Re: [PATCH net-next 06/12] netfilter: flowtable: store ethertype in flowtable context

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
netfilter: flowtable: store ethertype in flowtable context

This adds an ether_type field to struct nf_flowtable_ctx, sets it while
parsing the layer 2 header in nf_flow_skb_encap_protocol(), and replaces
the local inner_proto computations in nf_flow_tuple_encap() and
nf_flow_skb_encap_protocol() with reads of ctx->ether_type.  The IPv4 and
IPv6 lookup helpers now compare ctx->ether_type themselves instead of
passing an expected protocol down.

No Link: tags in the commit header.

> diff --git a/net/netfilter/nf_flow_table_ip.c b/net/netfilter/nf_flow_table_ip.c
> index 7692ae7aa8539..3f417a43bd122 100644
> --- a/net/netfilter/nf_flow_table_ip.c
> +++ b/net/netfilter/nf_flow_table_ip.c
> @@ -161,7 +162,6 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
>  				struct sk_buff *skb,
>  				struct flow_offload_tuple *tuple)
>  {
> -	__be16 inner_proto = skb->protocol;
>  	struct vlan_ethhdr *veth;
>  	struct pppoe_hdr *phdr;
>  	struct ipv6hdr *ip6h;
> @@ -179,19 +179,17 @@ static void nf_flow_tuple_encap(struct nf_flowtable_ctx *ctx,
>  		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
>  		tuple->encap[i].id = ntohs(veth->h_vlan_TCI);
>  		tuple->encap[i].proto = skb->protocol;
> -		inner_proto = veth->h_vlan_encapsulated_proto;
>  		offset += VLAN_HLEN;
>  		break;
>  	case htons(ETH_P_PPP_SES):
>  		phdr = (struct pppoe_hdr *)skb_network_header(skb);
>  		tuple->encap[i].id = ntohs(phdr->sid);
>  		tuple->encap[i].proto = skb->protocol;
> -		inner_proto = *((__be16 *)(phdr + 1));
>  		offset += PPPOE_SES_HLEN;
>  		break;
>  	}
>  
> -	switch (inner_proto) {
> +	switch (ctx->ether_type) {
>  	case htons(ETH_P_IP):
>  		iph = (struct iphdr *)(skb_network_header(skb) + offset);
>  		if (ctx->tun.inner_proto == IPPROTO_IPIP) {

[Severity: Medium]

Does this hunk change behaviour for PPPoE frames rather than just moving
the ethertype into the context?

Before this patch, the PPPoE arm fed the raw PPP protocol id into the
switch:

net/netfilter/nf_flow_table_ip.c:nf_flow_tuple_encap() {
	case htons(ETH_P_PPP_SES):
		...
		inner_proto = *((__be16 *)(phdr + 1));
		offset += PPPOE_SES_HLEN;
		break;
	}

	switch (inner_proto) {
	case htons(ETH_P_IP):
	...
}

PPP_IP is 0x0021 and PPP_IPV6 is 0x0057, so inner_proto could never equal
htons(ETH_P_IP) (0x0800) or htons(ETH_P_IPV6) (0x86dd), and PPPoE frames
always landed in the default: arm.  That left
tuple->tun.{src_v4,dst_v4,src_v6,dst_v6} and tuple->tun.inner_proto zero
even when nf_flow_skb_encap_protocol() -> nf_flow_ip4_tunnel_proto() had
already set ctx->tun.inner_proto = IPPROTO_IPIP.

After the patch, ctx->ether_type for PPPoE is the value normalised by
__nf_flow_pppoe_proto() in include/net/netfilter/nf_flow_table.h:

	switch (proto) {
	case htons(PPP_IP):
		return htons(ETH_P_IP);
	case htons(PPP_IPV6):
		return htons(ETH_P_IPV6);
	}

so these tunnel arms now execute for PPPoE traffic and populate
tuple->tun.

struct flow_offload_tunnel tun sits before the __hash marker in struct
flow_offload_tuple, so it is part of the rhashtable lookup key, and the
control path always installs non-zero endpoints for tunnel flows:

net/netfilter/nf_flow_table_core.c:flow_offload_fill_tuple() {
	flow_tuple->tun = route->tuple[dir].in.tun;
	flow_tuple->encap_num = route->tuple[dir].in.num_encaps;
}

filled from DEV_PATH_TUN in nft_dev_path_info(), which can be walked
together with DEV_PATH_PPPOE in the same path stack (ipip or ip6tnl over a
PPPoE underlay).  That means PPPoE encapsulated IPIP/IP6IP6 rx flows
previously missed their flowtable entry on every packet and now start
matching.

Should the commit message mention this, and should a Fixes: tag be added
pointing at ab427db17885 ("netfilter: flowtable: Add IPIP rx sw
acceleration") and d98103575dcd ("netfilter: flowtable: Add IP6IP6 rx sw
acceleration"), which introduced the raw PPP id versus ETH_P_* comparison?
As written, the message only says "Store the ether_type in the context
after parsing the layer 2 header for the first time and then use it later
on", which reads as a pure cleanup.

The behaviour persists to the end of the series (nf_flow_tuple_encap() at
839e6caf13c7 still switches on ctx->ether_type), so no later patch
documents or reverts it.

> @@ -389,22 +387,27 @@ static bool nf_flow_skb_encap_protocol(struct nf_flowtable_ctx *ctx,
>  			return false;
>  
>  		veth = (struct vlan_ethhdr *)skb_mac_header(skb);
> -		if (veth->h_vlan_encapsulated_proto == proto) {
> -			ctx->offset += VLAN_HLEN;
> -			inner_proto = proto;
> -			ret = true;
> -		}
> +		ctx->ether_type = veth->h_vlan_encapsulated_proto;
> +		ctx->offset += VLAN_HLEN;
> +		ret = true;
>  		break;
>  	case htons(ETH_P_PPP_SES):
> -		if (nf_flow_pppoe_proto(skb, &inner_proto) &&
> -		    inner_proto == proto) {
> -			ctx->offset += PPPOE_SES_HLEN;
> -			ret = true;
> -		}
> +		if (!nf_flow_pppoe_proto(skb, &ether_type))
> +			return false;
> +
> +		ctx->ether_type = ether_type;
> +		ctx->offset += PPPOE_SES_HLEN;
> +		ret = true;
> +		break;

This is the store of the normalised ETH_P_* value that nf_flow_tuple_encap()
later consumes for PPPoE frames.

[ ... ]