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 issu=
e(s) to consider:
> - [High] The `sdsa_xmit` and `sdsa_rcv` functions leak the SKB on all err=
or paths by returning `NULL` without freeing the buffer, leading to memory =
exhaustion.
> --
>=20
> --- Patch [3]: [PATCH 3/4] net: dsa: Add support for SoC-e SDSA tags ---
> commit d07af353633eb9eecc24042cfdf75f9ecf082368
> Author: Vasilij Strassheim <[email protected]>
>=20
> net: dsa: Add support for SoC-e SDSA tags
>=20
> Adds a DSA tagger for the SoC-e SDSA header format used between
> the conduit and SoC-e switch ports.
>=20
> > 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
>=20
> [ ... ]
>=20
> > +static struct sk_buff *sdsa_xmit(struct sk_buff *skb, struct net_devic=
e *dev)
> > +{
> > +	struct dsa_port *dp =3D dsa_user_to_port(dev);
> > +	u8 *sdsa_header;
> > +
> > +	if (skb_cow_head(skb, SDSA_HLEN) < 0)
> > +		return NULL;
>=20
> [Severity: High]
> Does this leak the skb when skb_cow_head() fails? The DSA core expects th=
e
> tagger to consume and free the skb if it returns NULL on transmission. Sh=
ould
> this call kfree_skb() before returning?
>=20

Yes, I will fix all this findings.

> [ ... ]
>=20
> > +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;
>=20
> [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.
>=20

Yes, I will fix all this findings.

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

Yes, I will fix all this findings.

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

Yes, I will fix all this findings.

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

Yes, I will fix all this findings.