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

[<JensEmil.SchulzOstergaard@microchip.com>]

On Thu, 2026-08-06 at 14:38 +0000, sashiko-bot@kernel.org wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The tag driver drops hardware-accelerated VLAN tags on transmit, leading to switch misclassification and double tagging.
> --
> 
> --- Patch [1]: [PATCH net-next v11 1/9] net: dsa: add tag driver for LAN9645X ---
> commit 97f24952991b201e3d0f1948d98e9534914b9db3
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
> 
> net: dsa: add tag driver for LAN9645X
> 
> This commit introduces a new DSA tag driver for the LAN9645X switch family,
> using an NPI port for CPU connection. It supports prepending a 28-byte
> internal frame header (IFH) on injection and handling a long prefix on
> extraction.
> 
> > diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> > new file mode 100644
> > index 0000000000000..6a33216c7792f
> > --- /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;
> > +     } else {
> > +             rcu_read_lock();
> > +             br_vlan_get_pvid_rcu(br, &tci);
> > +             rcu_read_unlock();
> > +             *vlan_tci = tci;
> > +     }
> 
> [Severity: High]
> Does this code drop hardware-accelerated VLAN tags on transmit?
> 
> When the network stack transmits a frame through the DSA user port with a
> hardware-accelerated VLAN tag, the tag is stored out-of-band in skb->vlan_tci.
> 
> By only checking the inline Ethernet header, ntohs(hdr->h_vlan_proto) will
> evaluate to the inner network protocol (like IPv4) instead of ETH_P_8021Q.
> This causes the check to fail and fall back to using the bridge PVID instead
> of extracting the existing out-of-band VLAN tag.
> 
> Because the out-of-band tag is ignored and never removed from the socket
> buffer, will the conduit driver eventually insert the original VLAN tag into
> the payload, resulting in a double-tagged frame on the wire?

If the conduit sets NETIF_F_HW_VLAN_CTAG_TX (or STAG_TX) in
conduit->vlan_features, then I think the analysis is correct.

__dev_queue_xmit() calls validate_xmit_skb() -> validate_xmit_vlan()
before ndo_start_xmit():

	if (skb_vlan_tag_present(skb) &&
	    !vlan_hw_offload_capable(features, skb->vlan_proto))
		skb = __vlan_hwaccel_push_inside(skb);

vlan_hw_offload_capable() requires NETIF_F_HW_VLAN_CTAG_TX (or STAG_TX).
A DSA user port inherits its features from the conduit

user->features = conduit->vlan_features | NETIF_F_HW_TC;

and user->hw_features only ever gains NETIF_F_HW_TC and
NETIF_F_HW_VLAN_CTAG_FILTER, so ethtool cannot turn the TX offload on.

But if a conduit sets those offload flags, the tag driver could receive
an skb with the vlan tag in the hwaccel area.

I will normalise the tag into the payload at the top of
lan9645x_xmit_get_vlan_info(), following the precedent in
sja1105_pvid_tag_control_pkt():

if (unlikely(skb_vlan_tag_present(skb))) {
        skb = __vlan_hwaccel_push_inside(skb);
        if (!skb)
                return NULL;
}

But the outcome would probably be worse than a double-tagged frame? We
prepend the dsa tag, and the conduit would not know anything about that, so
my guess is it would insert the vlan tag into the dsa tag.

> 
> > +
> > +     *tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
> > +                                          LAN9645X_IFH_TAG_TYPE_C;
> > +}
> 
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a@microchip.com?part=1