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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Double free in ks8995_xmit() error path when vlan_insert_tag() fails.
- [High] Double free in ks8995_rcv() error path when skb_vlan_untag() fails.
- [High] Undefined behavior from ilog2(0) when receiving standard 802.1Q frames.
- [High] Failure to push non-802.1Q hardware-accelerated VLAN tags into the payload.
--

commit 0b9e2806259ede96c62f73af014408284f18d9b2
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
> index 0000000000000..5bdb182715827
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c
[ ... ]
> +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]
Does this properly handle non-802.1Q hardware-accelerated VLAN tags like
802.1AD?

If an 802.1AD tag is present, it appears the tag is left in the hwaccel area
and the KS8995 special tag is pushed into the payload. The host NIC might
then push the 802.1AD tag onto the wire outside the KS8995 tag, which would
prevent the switch from seeing its special tag as the outermost ethertype.

> +
> +	if (have_hwaccel_tag || hdr->h_vlan_proto != htons(ETH_P_8021Q)) {
> +		skb = vlan_insert_tag(skb, KS8995M_STPID(portmask), tci);
> +		if (!skb)
> +			return NULL;

[Severity: High]
Will returning NULL here cause a double-free?

When vlan_insert_tag() fails to allocate memory, it frees the original skb
internally and returns NULL. ks8995_xmit() then propagates this NULL back to
dsa_user_xmit(). Since dsa_user_xmit() explicitly calls kfree_skb(skb) when
the tagger returns NULL, won't this result in freeing the same skb twice?

[ ... ]
> +static struct sk_buff *ks8995_rcv(struct sk_buff *skb, struct net_device *dev)
> +{
> +	int portmask;
> +	u16 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: High]
Could this also lead to a double-free if skb_vlan_untag() fails?

If skb_vlan_untag() fails, it internally frees the SKB and returns NULL.
ks8995_rcv() propagates this NULL back to dsa_switch_rcv(). The DSA receive
core interprets a NULL return from a tagger as an unhandled packet and
explicitly calls kfree_skb(skb) on the original pointer.

> +
> +	portmask = FIELD_GET(KS8995M_STPID_PORTMASK, etype);
> +	netdev_dbg(dev, "%s: etype %04x portmask %04x (%d)\n",
> +		   __func__, etype, portmask, ilog2(portmask));
> +	skb->dev = dsa_conduit_find_user(dev, 0, ilog2(portmask));

[Severity: High]
Does this trigger undefined behavior when receiving standard 802.1Q frames?

Standard 802.1Q frames with ethertype 0x8100 pass the earlier 
(etype & KS8995M_STPID_STD) != ETH_P_8021Q check because the standard VLAN
ethertype matches the mask 0xFFF0. This yields a portmask of 0. Calling
ilog2(0) is mathematically undefined behavior and invokes __builtin_clzll(0),
which can cause hardware exceptions or return unpredictable values depending
on the architecture.

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