Re: [v19,nf-next,4/5] netfilter: nft_set_pktinfo_ipv4/6_validate: Add nhoff argument

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "The tracking of feedback in the AI bot is often wrong,
but FWIW this is what it has to say about the submission"

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
netfilter: nft_set_pktinfo_ipv4/6_validate: Add nhoff argument

This commit adds the ability to specify an offset when calling
nft_set_pktinfo_ipv4/6_validate() for cases where the IP header is not
at skb_network_header().

> diff --git a/include/net/netfilter/nf_tables_ipv4.h b/include/net/netfilter/nf_tables_ipv4.h
> index fcf967286e37..bd354937134f 100644
> --- a/include/net/netfilter/nf_tables_ipv4.h
> +++ b/include/net/netfilter/nf_tables_ipv4.h

[ ... ]

> diff --git a/include/net/netfilter/nf_tables_ipv6.h b/include/net/netfilter/nf_tables_ipv6.h
> index c53ac00bb974..1e84a891f268 100644
> --- a/include/net/netfilter/nf_tables_ipv6.h
> +++ b/include/net/netfilter/nf_tables_ipv6.h
> @@ -24,17 +24,17 @@ static inline void nft_set_pktinfo_ipv6(struct nft_pktinfo *pkt)
>  	pkt->fragoff = frag_off;
>  }
>
> -static inline int __nft_set_pktinfo_ipv6_validate(struct nft_pktinfo *pkt)
> +static inline int __nft_set_pktinfo_ipv6_validate(struct nft_pktinfo *pkt, u32 nhoff)
>  {
>  #if IS_ENABLED(CONFIG_IPV6)
>  	unsigned int flags = IP6_FH_F_AUTH;
>  	struct ipv6hdr *ip6h, _ip6h;
> -	unsigned int thoff = 0;
> +	unsigned int thoff = nhoff;
                             ^^^^^

When nhoff != 0, does ipv6_find_hdr() interpret 'thoff' as a relative or
absolute offset?

Looking at the IPv6 header read above:

	ip6h = skb_header_pointer(pkt->skb, skb_network_offset(pkt->skb) + nhoff,
				  sizeof(*ip6h), &_ip6h);

The IPv6 header is at absolute offset 'skb_network_offset(pkt->skb) + nhoff'.

But when ipv6_find_hdr() is called:

	protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, &flags);

ipv6_find_hdr() interprets a non-zero *offset as an absolute offset from
skb->data (it does: ip6 = skb_header_pointer(skb, *offset, ...) and
start = *offset + sizeof(struct ipv6hdr)).

In bridge contexts where skb_network_offset != 0 (typically 14 for Ethernet),
ipv6_find_hdr() will read from the wrong location when nhoff != 0. Shouldn't
this be 'unsigned int thoff = skb_network_offset(pkt->skb) + nhoff;' to match
how the IPv6 header is read?

The IPv4 version correctly computes absolute offsets:

	pkt->thoff = skb_network_offset(pkt->skb) + nhoff + thoff;

All current callers pass nhoff=0, so this is dormant until a future patch
enables non-zero nhoff (which this commit is designed to support).

>  	unsigned short frag_off;
>  	u32 pkt_len, skb_len;
>  	int protohdr;
>
> -	ip6h = skb_header_pointer(pkt->skb, skb_network_offset(pkt->skb),
> +	ip6h = skb_header_pointer(pkt->skb, skb_network_offset(pkt->skb) + nhoff,
>  				  sizeof(*ip6h), &_ip6h);
>  	if (!ip6h)
>  		return -1;

[ ... ]

Florian Westphal raised a concern in v18 about a build warning in
include/uapi/linux/if_pppox.h:153:29 ('array of flexible structures').
He requested a new version to fix this. The author addressed this in v19 by
adding patch 1/5 'net: pppoe: avoid zero-length arrays in struct pppoe_hdr'
that wraps the flexible arrays in '#ifndef __KERNEL__' guards.

Reference: https://lore.kernel.org/netdev/aZynSuGMtK7JOOCj@strlen.de/