Re: [PATCH net-next v3] net: dsa: Fix skb ownership in taggers
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-21 21:27:08
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/ (local)
quoted hunk ↗ jump to hunk
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