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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The driver fails to handle hardware-accelerated VLAN tags on transmit, leading to dropped VLAN tags.
- [High] The driver ignores the return values of bridge VLAN functions, leading to the use of uninitialized stack variables.
- [High] The tag driver leaks the socket buffer (`skb`) when frame padding fails.
--

commit 2c0a796d85ea905a1a209fed6a3f2589d5f8409a
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..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) {
> +		vlan_remove_tag(skb, &tci);
> +		*vlan_tci = tci;

[Severity: High]
Does this code handle hardware-accelerated VLAN tags on transmit?

If the networking stack passes the VLAN tag out-of-band via skb->vlan_tci,
the payload ethertype will be the inner protocol (e.g., IPv4) and the check
above evaluates to false. This ignores the intended VLAN tag.

Could this be changed to use skb_vlan_tag_present(skb) and
skb_vlan_tag_get(skb)?

> +	} else {
> +		rcu_read_lock();
> +		br_vlan_get_pvid_rcu(br, &tci);

[Severity: High]
Does this ignore the return value of br_vlan_get_pvid_rcu()?

If the port has no default PVID configured, it can return an error code,
leaving tci uninitialized. The uninitialized tci is then used to populate
the hardware frame header, potentially injecting stack garbage into the
packet's VLAN TCI.

> +		rcu_read_unlock();
> +		*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);
> +
> +	/* 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;

[Severity: High]
Will this leak the socket buffer when padding fails?

__skb_put_padto() is called with free_on_error set to false. If padding
fails, this returns NULL. The DSA core transmit routine assumes a NULL return
from a tagger's transmit function means the tagger has freed or consumed
the skb, so the core won't free it. This permanently leaks the skb.

Should this use true for free_on_error instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527-dsa_lan9645x_switch_driver_base-v6-0-4d409ae64f3c@microchip.com?part=1