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

[Paolo Abeni <pabeni@redhat.com>]

On 8/7/26 4:41 PM, JensEmil.SchulzOstergaard@microchip.com wrote:
> 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.
What about something alike the following?

	if (skb_vlan_tag_present(skb)) {
		*vlan_tci = skb_vlan_tag_get(__skb);
	} else if (skb_headlen(skb) >= VLAN_ETH_HLEN &&
		 ntohs(hdr->h_vlan_proto) == proto) {
	// ...

Side note: I *think* the unlikely() on skb_vlan_tag_present() is not
needed; the like-wood depends on the actual traffic pattern.

/P