Re: [PATCH net v4] net: dsa: Fix skb ownership in taggers

[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.
---
net: dsa: Fix skb ownership in taggers

This change reworks the DSA tagger ->xmit() and ->rcv() ownership rules
so that taggers consume the skb on failure rather than leaving the drop
to dsa_user_xmit() and dsa_switch_rcv(). It removes the kfree_skb()
calls in those two callers and adds matching kfree_skb() in every
tagger error path that retains skb ownership.

Closes: https://lore.kernel.org/r/20260610153952.1685895-1-kuba@kernel.org/

> diff --git a/net/dsa/tag.c b/net/dsa/tag.c
> index 79ad105902d9..107e93250b94 100644
> --- a/net/dsa/tag.c
> +++ b/net/dsa/tag.c
> @@ -79,15 +79,16 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  		if (likely(skb->dev)) {
>  			dsa_default_offload_fwd_mark(skb);
>  			nskb = skb;
> +		} else {
> +			/* Just drop the skb if we can't find the user */
> +			kfree_skb(skb);
>  		}
>  	} else {
>  		nskb = cpu_dp->rcv(skb, dev);
>  	}
>  
> -	if (!nskb) {
> -		kfree_skb(skb);
> +	if (!nskb)
>  		return 0;
> -	}

[Critical]
While the ->rcv() path in dsa_switch_rcv() is now consistent with the
new "consumer drops on failure" rule, the dsa_software_vlan_untag()
call later in the same function appears to still follow the old rule.
Can that produce a double-free?

The relevant block, unchanged by this patch, is:

    if (unlikely(cpu_dp->ds->untag_bridge_pvid ||
                 cpu_dp->ds->untag_vlan_aware_bridge_pvid)) {
            nskb = dsa_software_vlan_untag(skb);
            if (!nskb) {
                    kfree_skb(skb);
                    return 0;
            }
            skb = nskb;
    }

The only NULL return inside dsa_software_vlan_untag() is the
skb_vlan_untag() path in net/dsa/tag.h:

    if (!skb_vlan_tag_present(skb) && skb->protocol == htons(proto)) {
            skb = skb_vlan_untag(skb);
            if (!skb)
                    return NULL;
    }

skb_vlan_untag() in net/core/skbuff.c already frees on every error
path before returning NULL:

    err_free:
            kfree_skb(skb);
            return NULL;

That is reachable on a malformed VLAN-tagged frame, for example
through the pskb_may_pull(skb, VLAN_HLEN + sizeof(unsigned short))
check, the skb_share_check() failure, or skb_reorder_vlan_header().

So when dsa_software_vlan_untag() returns NULL, the original skb
has already been freed by skb_vlan_untag(), and the kfree_skb(skb)
in dsa_switch_rcv() then operates on freed memory. Is this the
same anti-pattern the commit message describes as "conceptually
wrong: the pattern elsewhere in the networking code is that
consumers drop their skb:s on failure", and should this call site
also be converted (drop the kfree_skb(skb) here, since the helper
already consumes on failure)?

The path is reachable from any driver that sets
ds->untag_bridge_pvid or ds->untag_vlan_aware_bridge_pvid.
-- 
pw-bot: cr