Re: [PATCH net-next v8 1/9] net: dsa: add tag driver for LAN9645X

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver mishandles valid VLAN-tagged packets if the VLAN header resides in paged fragments, silently treating them as untagged.
- [Medium] `vlan_remove_tag()` moves the MAC header in the SKB data buffer but fails to update the `skb->mac_header` offset.
--

commit e26041faee6aff2a556fc66f9a08bab7e7391ab7
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: add tag driver for LAN9645X

Add tag driver for LAN9645x using a front port as CPU port. This mode
is called an NPI port in the datasheet.

> diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> new file mode 100644
> index 0000000000000..c9049e3620a12
> --- /dev/null
> +++ b/net/dsa/tag_lan9645x.c

[ ... ]

> +static void lan9645x_xmit_get_vlan_info(struct sk_buff *skb,
> +					struct net_device *br,
> +					u32 *vlan_tci, u32 *tag_type)
> +{
> +	struct vlan_ethhdr *hdr;
> +	u16 proto, tci;
> +
> +	if (!br || !br_vlan_enabled(br)) {
> +		*vlan_tci = 0;
> +		*tag_type = LAN9645X_IFH_TAG_TYPE_C;
> +		return;
> +	}
> +
> +	hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
> +	br_vlan_get_proto(br, &proto);
> +
> +	if (skb_headlen(skb) >= VLAN_ETH_HLEN &&
> +	    ntohs(hdr->h_vlan_proto) == proto) {

[Severity: High]
Does this silently treat valid VLAN-tagged packets as untagged if the VLAN
header resides in paged fragments?

If a packet is transmitted where the linear part of the SKB is less than
VLAN_ETH_HLEN (for example, exactly ETH_HLEN) and the VLAN tag is in a
fragment, this check fails and bypasses tag extraction.

This seems like it would inject the packet into the switch with the
port's PVID instead of its actual VLAN tag, leaving the original VLAN tag
in the payload. Should this use pskb_may_pull() or __skb_vlan_pop() to
ensure the header is properly handled?

> +		vlan_remove_tag(skb, &tci);

[Severity: Medium]
Does calling vlan_remove_tag() directly leave the SKB in an inconsistent
state?

This internal helper moves the MAC header in the SKB data buffer but does
not appear to update the skb->mac_header offset. If any downstream code
(like a conduit driver or packet sniffer) relies on skb_mac_header(skb),
it might read garbage bytes.

Should __skb_vlan_pop() be used instead to ensure the metadata is correctly
updated?

> +		*vlan_tci = tci;
> +	} else {
> +		rcu_read_lock();
> +		br_vlan_get_pvid_rcu(br, &tci);
> +		rcu_read_unlock();
> +		*vlan_tci = tci;
> +	}
> +
> +	*tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
> +					     LAN9645X_IFH_TAG_TYPE_C;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-dsa_lan9645x_switch_driver_base-v8-0-90228d8bba58@microchip.com?part=1