Eric Woudstra <[email protected]> wrote:
This an AI-generated review, with no edits.
I added my own 2 cents at the end.
> diff --git a/net/netfilter/nft_chain_filter.c
> b/net/netfilter/nft_chain_filter.c
> index d4d5eadaba9c..66ef30c60e56 100644
> --- a/net/netfilter/nft_chain_filter.c
> +++ b/net/netfilter/nft_chain_filter.c
> @@ -227,21 +227,68 @@ static inline void nft_chain_filter_inet_fini(void) {}
> #endif /* CONFIG_NF_TABLES_IPV6 */
>
> #if IS_ENABLED(CONFIG_NF_TABLES_BRIDGE)
> +static int nft_set_bridge_pktinfo(struct nft_pktinfo *pkt, struct sk_buff
> *skb,
> + const struct nf_hook_state *state,
> + __be16 *proto)
> +{
> + nft_set_pktinfo(pkt, skb, state);
> +
> + switch (*proto) {
> + case htons(ETH_P_PPP_SES): {
[ ... ]
> + }
> + case htons(ETH_P_8021Q): {
> + struct vlan_hdr *vhdr, _vhdr;
> +
> + vhdr = skb_header_pointer(skb, 0, sizeof(_vhdr), &_vhdr);
> + if (!vhdr) {
> + *proto = 0;
> + return -1;
> + }
> + *proto = vhdr->h_vlan_encapsulated_proto;
> + return VLAN_HLEN;
> + }
> + }
> + return 0;
> +}
The commit message states support for "inner 802.1q, combined with an
outer 802.1ad or 802.1q encapsulation", but the code only handles
ETH_P_8021Q (0x8100). Should there be a case for ETH_P_8021AD (0x88A8)?
When a packet has an 802.1ad outer VLAN tag, proto will be ETH_P_8021AD
but won't match any case in the switch statement. The function returns 0
with proto unchanged, then nft_do_chain_bridge() calls
nft_set_pktinfo_unspec(), setting thoff to 0.
Other netfilter code handles both protocols, for example:
net/netfilter/nf_flow_table_ip.c:
switch (tuple->encap[i].proto) {
case htons(ETH_P_8021Q):
case htons(ETH_P_8021AD):
if (skb_vlan_push(...))
For double VLAN scenarios like outer 802.1q + inner 802.1q, the function
strips the outer tag and sets proto to ETH_P_8021Q (from the inner VLAN).
When nft_do_chain_bridge() switches on this proto value, it doesn't match
ETH_P_IP or ETH_P_IPV6, so it falls through to the default case and calls
nft_set_pktinfo_unspec().
This means for a packet with two 802.1q tags followed by IPv4, the offset
returned is 4 (one VLAN header), but nft_set_pktinfo_unspec() is called
and thoff becomes 0 instead of 8.
Should nft_set_bridge_pktinfo() handle nested encapsulations recursively,
or should nft_do_chain_bridge() check if the updated proto is still a
VLAN type and call nft_set_bridge_pktinfo() again?
[ fw: I oppose auto-following nested headers an would prefer to add
any vlan-vlan-vlan-vlan-vlan-pppoe or whatever incrementally after
someone explains why thats sane.
Wrt. the rest, I suspect the LLM gets confused due to vlan offloading,
the code is fine if vlan offloading is on, we have outer vlan tag in
the skb and eth_hdr(skb)->h_proto will be ETH_P_8021Q.
Neverthelss: should this support configurations where vlan tag offload
is off? If it should, then ETH_P_8021AD / ETH_P_8021Q combo has to be
handled. That could be done in a followup change, of course.
So, *I* don't see a need to send another iteration of the patch.
Eric, whats your take? Supersede? Ignore LLM? Follwup patch?
