Re: [PATCH 3/4] net: dsa: Add support for SoC-e SDSA tags

On Thu, 2026-07-30 at 16:37 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The `sdsa_xmit` and `sdsa_rcv` functions leak the SKB on all error paths by returning `NULL` without freeing the buffer, leading to memory exhaustion.
> --
> 
> --- Patch [3]: [PATCH 3/4] net: dsa: Add support for SoC-e SDSA tags ---
> commit d07af353633eb9eecc24042cfdf75f9ecf082368
> Author: Vasilij Strassheim <[email protected]>
> 
> net: dsa: Add support for SoC-e SDSA tags
> 
> Adds a DSA tagger for the SoC-e SDSA header format used between
> the conduit and SoC-e switch ports.
> 
> > diff --git a/net/dsa/tag_sdsa.c b/net/dsa/tag_sdsa.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/net/dsa/tag_sdsa.c
> 
> [ ... ]
> 
> > +static struct sk_buff *sdsa_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	struct dsa_port *dp = dsa_user_to_port(dev);
> > +	u8 *sdsa_header;
> > +
> > +	if (skb_cow_head(skb, SDSA_HLEN) < 0)
> > +		return NULL;
> 
> [Severity: High]
> Does this leak the skb when skb_cow_head() fails? The DSA core expects the
> tagger to consume and free the skb if it returns NULL on transmission. Should
> this call kfree_skb() before returning?
> 

Yes, I will fix all this findings.

> [ ... ]
> 
> > +static struct sk_buff *sdsa_rcv(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	u8 *sdsa_header;
> > +	int source_port;
> > +	u8 frame_type;
> > +
> > +	if (unlikely(!pskb_may_pull(skb, SDSA_HLEN)))
> > +		return NULL;
> 
> [Severity: High]
> Does this error path leak the skb? If a malformed packet causes
> pskb_may_pull() to fail, returning NULL without freeing the buffer could
> allow network traffic to cause memory exhaustion.
> 

Yes, I will fix all this findings.

> > +
> > +	sdsa_header = dsa_etype_header_pos_rx(skb);
> > +
> > +	/* Check that the frame type is TO_CPU. */
> > +	frame_type = FIELD_GET(SDSA_TAG_FRAME_TYPE_MASK, sdsa_header[4]);
> > +	if (frame_type != 0)
> > +		return NULL;
> 
> [Severity: High]
> Should the skb be freed here when the frame type check fails to avoid a
> memory leak?
> 

Yes, I will fix all this findings.

> > +
> > +	/* VLAN-tagged traffic is intentionally unsupported yet. */
> > +	if (sdsa_header[4] & SDSA_TAG_VLAN_BIT)
> > +		return NULL;
> 
> [Severity: High]
> Could unsupported VLAN-tagged traffic trigger a memory leak here?
> 

Yes, I will fix all this findings.

> > +
> > +	/* Determine the source port from the two port fields. */
> > +	source_port = ((sdsa_header[4] & 0x1f) << 5) |
> > +		      ((sdsa_header[5] & 0xf8) >> 3);
> > +
> > +	skb->dev = dsa_conduit_find_user(dev, 0, source_port);
> > +	if (!skb->dev)
> > +		return NULL;
> 
> [Severity: High]
> Does this also leak the skb if the conduit cannot find a corresponding user
> device?

Yes, I will fix all this findings.