Re: [PATCH net-next v6 3/5] net: dsa: tag_ks8995: Add the KS8995 tag handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Double free of the socket buffer on the error path in `ks8995_rcv()` when `skb_vlan_untag()` fails.
- [High] Unhandled hardware-accelerated VLAN tags bypass clearing and break the required DSA tagging order on transmission.
--

commit e089d604a4cbc6ed8eab92ba9931985670f7afbe
Author: Linus Walleij <linusw@kernel.org>

net: dsa: tag_ks8995: Add the KS8995 tag handling

The KS8995 100Mbit switch can do proper DSA per-port tagging with the proper
set-up. This adds the code to handle ingress and egress KS8995 tags.

> diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c
> new file mode 100644
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +static struct sk_buff *ks8995_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
> +	bool have_hwaccel_tag = false;
> +	u16 tci = 0, portmask;
> +
> +	/* Prepare the special KS8995 tags */
> +	portmask = dsa_xmit_port_mask(skb, dev);
> +
> +	if (skb_vlan_tag_present(skb) && skb->vlan_proto == htons(ETH_P_8021Q)) {
> +		tci = skb_vlan_tag_get(skb);
> +		__vlan_hwaccel_clear_tag(skb);
> +		have_hwaccel_tag = true;
> +	}

[Severity: High]
If a packet has a different hardware-accelerated tag (e.g., 802.1ad Provider
Bridge), does it bypass this clearing logic?

The KS8995 switch architecture requires the special DSA tag (STPID) to be the
first tag in the Ethernet frame immediately following the MAC addresses.

If the non-802.1Q hwaccel tag is left intact, the conduit network driver will
insert the pending hwaccel tag immediately after the MAC addresses, placing it
before the KS8995 DSA tag.

Could this cause the switch to fail to parse the DSA tag, leading to misrouted
or dropped packets?

[ ... ]

> +static struct sk_buff *ks8995_rcv(struct sk_buff *skb, struct net_device *dev)
> +{
> +	int portmask;
> +	u16 etype;
> +
> +	/* We are expecting all received packets to have a mangled VLAN
> +	 * TPID, so drop anything else. Because of the non-standard TPID,
> +	 * don't even bother looking for a tag in the hwaccel area.
> +	 *
> +	 * We have to inspect the ethertype directly because skb->protocol
> +	 * will contain garbage.
> +	 */
> +	etype = ntohs(*(__be16 *)dsa_etype_header_pos_rx(skb));
> +	if ((etype & KS8995M_STPID_STD) != ETH_P_8021Q) {
> +		netdev_dbg(dev, "%s: dropped ethertype 0x%04x\n",
> +			   __func__, etype);
> +		return NULL;
> +	}
> +	netdev_dbg(dev, "%s: received ethertype %04x\n",
> +		   __func__, etype);
> +
> +	/* Move the custom DSA+VLAN tag into the hwaccel area and strip
> +	 * it from the skb head
> +	 */
> +	skb = skb_vlan_untag(skb);
> +	if (!skb) {
> +		netdev_err(dev, "%s: unable to untag skb\n", __func__);
> +		return NULL;
> +	}

[Severity: Critical]
Does this result in a double-free?

If skb_vlan_untag() fails, it frees the skb internally and returns NULL.
ks8995_rcv() then returns NULL to the DSA core function dsa_switch_rcv().

The API contract for the DSA .rcv callback dictates that it must not free the
original skb if it returns an error (NULL), because dsa_switch_rcv() will call
kfree_skb() on the original skb pointer when receiving a NULL return.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-ks8995-to-ksz8-v6-0-d91eb43edcc5@kernel.org?part=3