Re: [PATCH v2] net/sched: act_nat: only rewrite IPv4 packets

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/sched: act_nat: only rewrite IPv4 packets

This commit restricts act_nat to only process IPv4 packets to prevent
non-IPv4 payloads from being incorrectly parsed and modified as IPv4
headers. It adds a protocol check using skb_protocol() to skip inline
VLAN tags.

> diff --git a/net/sched/act_nat.c b/net/sched/act_nat.c
> index abb332dee836d..cb3e7a415da74 100644
> --- a/net/sched/act_nat.c
> +++ b/net/sched/act_nat.c
> @@ -142,6 +142,9 @@ TC_INDIRECT_SCOPE int tcf_nat_act(struct sk_buff *skb,
>  	egress = parms->flags & TCA_NAT_FLAG_EGRESS;
>  
>  	noff = skb_network_offset(skb);
> +	if (skb_protocol(skb, false) != htons(ETH_P_IP))
> +		goto out;
> +

[Severity: High]
Will this check skip valid IPv4 packets that have hardware-accelerated VLAN
tags?

When a packet has a hardware-accelerated VLAN tag, skb_vlan_tag_present(skb)
is true. In this case, skb_protocol(skb, false) evaluates to skb->vlan_proto
(such as ETH_P_8021Q), causing the check to evaluate to true and jump to the
out label.

Since hardware-accelerated VLAN tags are stripped by the MAC driver,
skb_network_offset(skb) correctly points to the inner IPv4 header and
skb->protocol already holds htons(ETH_P_IP). Should this code check
skb->protocol != htons(ETH_P_IP) directly instead to avoid skipping
hardware-VLAN-tagged IPv4 packets?

>  	if (!pskb_may_pull(skb, sizeof(*iph) + noff))
>  		goto drop;
>
-- 
pw-bot: cr