Re: [PATCH net-next v3] 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 patch moves SKB drop responsibility from dsa_user_xmit() and
dsa_switch_rcv() into the per-tagger ->xmit() and ->rcv() callbacks, so
that taggers consume the skb on their own error paths and the call sites
no longer free a returned NULL skb.

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..cfc8f5a0cbd9 100644
> --- a/net/dsa/tag.c
> +++ b/net/dsa/tag.c
> @@ -84,10 +84,8 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>  		nskb = cpu_dp->rcv(skb, dev);
>  	}
>  
> -	if (!nskb) {
> -		kfree_skb(skb);
> +	if (!nskb)
>  		return 0;
> -	}
>  
>  	skb = nskb;
>  	skb_push(skb, ETH_HLEN);

[High]
Does the new contract that taggers consume the skb on error cover the
inline METADATA_HW_PORT_MUX branch in dsa_switch_rcv() as well?  That
branch is not a tagger callback, it is open-coded in dsa_switch_rcv():

	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
		unsigned int port = md_dst->u.port_info.port_id;

		skb_dst_drop(skb);
		if (!skb_has_extensions(skb))
			skb->slow_gro = 0;

		skb->dev = dsa_conduit_find_user(dev, 0, port);
		if (likely(skb->dev)) {
			dsa_default_offload_fwd_mark(skb);
			nskb = skb;
		}
	} else {
		nskb = cpu_dp->rcv(skb, dev);
	}

	if (!nskb)
		return 0;

When dsa_conduit_find_user() returns NULL (for example a port_id that
does not match any registered DSA user, such as the CPU port index, an
invalid id from the conduit driver, or a port being torn down), nskb
stays NULL and skb is not freed anywhere, so the new

	if (!nskb)
		return 0;

path looks like it leaks the skb on every such packet.

Pre-patch, the removed kfree_skb(skb) at the !nskb site covered this
case.  Should the metadata-mux branch free the skb itself when
skb->dev is NULL, or should the unconditional kfree_skb(skb) at the
!nskb site be kept for this path?

[ ... ]
-- 
pw-bot: cr