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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Uninitialized variable `tci` in `lan9645x_xmit_get_vlan_info` leads to garbage VLAN ID injection.
- [High] Missing `skb_unshare` or `skb_cow_head` causes silent data corruption on shared/cloned SKBs.
- [High] Hardware-offloaded VLAN tags are ignored on transmit, causing VLAN-tagged packets to be sent on the wrong VLAN.
- [High] Incorrect MAC header extraction in RX path breaks DSA core processing and CHECKSUM_COMPLETE.
- [Medium] Missing `.needed_tailroom` causes unnecessary SKB reallocations for small packets.
--

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

net: dsa: add tag driver for LAN9645X

This commit adds a new DSA tag driver for LAN9645x switches using an NPI port.
It implements asymmetric tagging with a long prefix on extraction (RX) and an
internal frame header (IFH) on both RX and TX.

> diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> new file mode 100644
> index 0000000000000..0b639228215e3
> --- /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) {

Does this code ignore hardware-offloaded VLAN tags? If the stack passes a
packet with the VLAN tag in skb->vlan_tci (indicated by
skb_vlan_tag_present()), this only checks for an inline payload tag. The
hardware-accelerated tag might be skipped, falling back to the bridge
port's untagged PVID instead.

> +		vlan_remove_tag(skb, &tci);
> +		*vlan_tci = tci;
> +	} else {
> +		rcu_read_lock();
> +		br_vlan_get_pvid_rcu(br, &tci);
> +		rcu_read_unlock();

If the bridge port does not have a PVID configured, br_vlan_get_pvid_rcu()
returns an error (e.g., -EINVAL or -ENOENT). Since the return value isn't
checked, could tci remain uninitialized here and cause garbage VLAN IDs to be
injected into the hardware's internal frame header?

> +		*vlan_tci = tci;
> +	}
> +
> +	*tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
> +					     LAN9645X_IFH_TAG_TYPE_C;
> +}
> +
> +static struct sk_buff *lan9645x_xmit(struct sk_buff *skb,
> +				     struct net_device *ndev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(ndev);
> +	struct dsa_switch *ds = dp->ds;
> +	u32 cpu_port = ds->num_ports;
> +	u32 vlan_tci, tag_type;
> +	u32 qos_class;
> +	void *ifh;
> +
> +	lan9645x_xmit_get_vlan_info(skb, dsa_port_bridge_dev_get(dp), &vlan_tci,
> +				    &tag_type);

Since this function can modify the SKB data buffer directly (e.g., in
vlan_remove_tag() moving the MAC header, and skb_push() overwriting
headroom), is it possible for the SKB to be shared or cloned here?

If so, skipping skb_cow_head() or skb_unshare() before modifying the SKB
might corrupt the packet data for other consumers holding a reference to
the clone.

> +
> +	/* We need to make sure frame has the proper size after IFH is stripped
> +	 * by hw.
> +	 */
> +	if (__skb_put_padto(skb, ETH_ZLEN, false))
> +		return NULL;

[ ... ]

> +static struct sk_buff *lan9645x_rcv(struct sk_buff *skb,
> +				    struct net_device *ndev)
> +{

[ ... ]

> +	skb_pull(skb, ifh_gap_len);
> +	skb_reset_mac_header(skb);
> +	skb_set_network_header(skb, ETH_HLEN);
> +	skb_reset_mac_len(skb);
> +
> +	/* Reset skb->data past the actual ethernet header. */
> +	skb_pull(skb, ETH_HLEN);
> +
> +	/* We must deliver the skb so skb->csum only covers the data beyond the
> +	 * real ethernet header. The fake ethernet header in the prefix is
> +	 * not part of skb->csum already. We must subtract what remains of the
> +	 * prefix, the ifh and the gap.
> +	 */
> +	skb_postpull_rcsum(skb,
> +			   skb->data - LAN9645X_TOTAL_TAG_LEN - ifh_gap_len,
> +			   LAN9645X_TOTAL_TAG_LEN + ifh_gap_len);

Does this second skb_pull() advance skb->data past the inner MAC header?

The networking stack (via eth_type_trans) expects skb->data to point to the
MAC header so it can read the EtherType and pull the MAC header itself.
Advancing it here could cause eth_type_trans to incorrectly strip the first
14 bytes of the IP header.

Additionally, because skb->data was already advanced, the offset calculation
for skb_postpull_rcsum() might evaluate to the start of the outer fake
payload, subtracting the wrong bytes instead of the prefix and tag.

> +
> +	skb->dev = dsa_conduit_find_user(ndev, 0, src_port);
> +	if (WARN_ON_ONCE(!skb->dev)) {

[ ... ]

> +static const struct dsa_device_ops lan9645x_netdev_ops = {
> +	.name = LAN9645X_NAME,
> +	.proto = DSA_TAG_PROTO_LAN9645X,
> +	.xmit = lan9645x_xmit,
> +	.rcv = lan9645x_rcv,
> +	.needed_headroom = LAN9645X_TOTAL_TAG_LEN,
> +};

To avoid unnecessary SKB reallocations in the transmit fast path for small
packets, should .needed_tailroom be defined here?

The lan9645x_xmit() function calls __skb_put_padto(skb, ETH_ZLEN, false),
so adding .needed_tailroom = ETH_ZLEN - ETH_HLEN would allow the network core
to preallocate this space and avoid triggering pskb_expand_head().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-dsa_lan9645x_switch_driver_base-v5-0-968fbf34ffa3@microchip.com?part=1