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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Mon, 2026-10-12 at 11:24 +0000, netdev-bot+sashiko@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 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In lan9645x_rcv() (net/dsa/tag_lan9645x.c:459), every tag
>   popped by the hardware is restored as an 802.1Q hwaccel tag with…
> - [Low] The comment above skb_postpull_rcsum() in lan9645x_rcv()
>   (net/dsa/tag_lan9645x.c:416-421) says the bytes to subtract are "what…
> - [Low] lan9645x_rcv() (net/dsa/tag_lan9645x.c:450) restores the popped
>   VLAN tag only when `dsa_port_is_vlan_filtering(dp) && ifh_gap_len`.
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but the new
>   tagger adds another lockless reader of it.
> 
> --
> 
> net: dsa: add tag driver for LAN9645X
> 
> This adds a DSA tag driver for the Microchip LAN9645x switch when a front
> port is used as the CPU (NPI) port. Extraction uses a long prefix plus a
> 28 byte IFH, and injection uses only the IFH. VLAN tags popped by the
> hardware are restored on receive as 802.1Q hwaccel tags, and on transmit
> the VLAN tag is moved from the payload into the IFH.
> 
> > diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> > new file mode 100644
> > index 0000000000000..00339daecfcbd
> > --- /dev/null
> > +++ b/net/dsa/tag_lan9645x.c
> > @@ -0,0 +1,514 @@
> 
> [ ... ]
> 
> > +static struct sk_buff *lan9645x_xmit(struct sk_buff *skb,
> > +                                  struct net_device *ndev)
> > +{
> > +     struct dsa_port *dp = dsa_user_to_port(ndev);
> > +     u32 vlan_tci, tag_type;
> > +     u32 qos_class;
> > +     void *ifh;
> > +
> > +     skb = lan9645x_xmit_get_vlan_info(skb, dsa_port_bridge_dev_get(dp),
> > +                                       &vlan_tci, &tag_type);
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but this adds one more lockless
> reader of dp->bridge in the xmit path.
> 
> dsa_port_bridge_dev_get() does this with no READ_ONCE() and no lifetime
> protection:
> 
>         return dp->bridge ? dp->bridge->dev : NULL;
> 
> On bridge leave, dsa_port_bridge_leave()->dsa_port_bridge_destroy() clears
> the pointer and frees the last reference right away:
> 
>         dp->bridge = NULL;
> 
>         if (!refcount_dec_and_test(&bridge->refcount))
>                 return;
>         ...
>         kfree(bridge);
> 
> There is no RCU grace period between the two. Can a transmit on the user
> port that races with the bridge leave load the old dp->bridge and then read
> ->dev from a freed dsa_bridge?
> 
> The window is narrow. tag_ocelot.c, tag_sja1105.c and the
> dsa_software_vlan_untag() helpers use the same pattern, so the root cause
> is in the DSA core and not in this tagger.
> 

Dismissed, preexisting.

This is the existing DSA core pattern, shared with tag_dsa, tag_ocelot,
tag_sja1105, tag_vsc73xx_8021q and dsa_software_vlan_untag().
dsa_port_bridge_destroy() frees the dsa_bridge without waiting for a
grace period, so a tagger cannot fix this locally. The fix belongs in
the DSA core.


> [ ... ]
> 
> > +     /* 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. The start is derived from the current
> > +      * skb->data rather than saved on entry, because the pskb_may_pull()
> > +      * calls above may have reallocated skb->head.
> > +      */
> > +     skb_postpull_rcsum(skb,
> > +                        skb->data - LAN9645X_TOTAL_TAG_LEN - ifh_gap_len,
> > +                        LAN9645X_TOTAL_TAG_LEN + ifh_gap_len);
> 
> [Severity: Low]
> This isn't a bug, but the comment doesn't list everything the call
> subtracts.
> 
> The length is LAN9645X_TOTAL_TAG_LEN + ifh_gap_len, which is 44 + gap
> bytes. That is 2 bytes of prefix remainder, plus 28 bytes of IFH, plus the
> gap, plus the 14 byte real Ethernet header.
> 
> The code looks right, because dsa_switch_rcv() does skb_push(ETH_HLEN) and
> then eth_type_trans() without adjusting the checksum. Could the comment
> also mention the real Ethernet header? Otherwise someone might later
> shorten the length to match the comment.
> 
> [ ... ]
> 
> > +     dp = dsa_user_to_port(skb->dev);
> > +
> > +     if (dsa_port_is_vlan_filtering(dp) && ifh_gap_len) {
> 
> [Severity: Low]
> Can frames lose their VLAN tag while vlan_filtering is changing?
> 
> dsa_port_vlan_filtering() calls the driver op before it updates the
> software flag:
> 
>         err = ds->ops->port_vlan_filtering(ds, dp->index, vlan_filtering,
>                                            extack);
>         ...
>                 dp->vlan_filtering = vlan_filtering;
> 
> In this series, lan9645x_port_vlan_filtering() programs VLAN_POP_CNT right
> away through lan9645x_vlan_port_apply(). Frames that the hardware pops in
> that window reach lan9645x_rcv() with ifh_gap_len != 0 while
> dsa_port_is_vlan_filtering(dp) is still false. They are delivered with no
> tag.
> 
> The reverse transition has the same problem, including bridge leave via
> lan9645x_vlan_set_hostmode(). Frames popped before the change but handled
> after dp->vlan_filtering is cleared also lose their tag.
> 
> POP_CNT is 0 on non-filtering ports, so the gap is already 0 there. Would
> checking only ifh_gap_len keep the CPU view consistent with what the
> hardware did?
> 
> > +             /* The tag is restored as a C-tag, not as the TAG_TYPE the IFH
> > +              * reports. The classifier recognizes both TPIDs as VLAN tags,
> > +              * so an S-tag has already been used for classification by the
> > +              * time we get here. Restoring it as 802.1AD would make the
> > +              * bridge push it back into the payload and reclassify the frame
> > +              * to the port pvid, on a different VID than the one the
> > +              * hardware forwarded it on.
> > +              */
> > +             __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
> > +     }
> 
> [Severity: Medium]
> Is the 802.1Q-only assumption enforced anywhere? The commit message says:
> 
>   This assumes a bridge vlan_protocol of 802.1Q, which is the only
>   protocol the driver offloads.
> 
> The xmit side of this file does handle non-802.1Q bridges, in
> lan9645x_xmit_get_vlan_info():
> 
>         br_vlan_get_proto(br, &proto);
>         ...
>                 if (ntohs(hdr->h_vlan_proto) == proto &&
>                     ntohs(hdr->h_vlan_TCI) & VLAN_VID_MASK) {
>         ...
>         *tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
>                                              LAN9645X_IFH_TAG_TYPE_C;
> 
> DSA doesn't handle SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL.
> __br_vlan_set_proto() sends that attribute with
> SWITCHDEV_F_SKIP_EOPNOTSUPP, so setting "vlan_protocol 802.1ad" on the
> bridge seems to succeed while the lan9645x ports stay offloaded. The switch
> driver in this series doesn't appear to check the bridge vlan_protocol
> either.
> 
> On such a bridge, every tag restored here has an 0x8100 TPID, and
> __allowed_ingress() in net/bridge/br_vlan.c then does:
> 
>         if (skb->vlan_proto != br->vlan_proto) {
>                 /* Protocol-mismatch, empty out vlan_tci for new tag */
>                 skb_push(skb, ETH_HLEN);
>                 skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
>                                                 skb_vlan_tag_get(skb));
> 
> This pushes the tag back into the payload and classifies the frame to the
> pvid. Wouldn't frames trapped or copied to the CPU then land on the pvid?
> That is the same misclassification the C-tag normalization is meant to
> avoid, only in the opposite configuration.
> 
> Should rcv follow the bridge vlan_protocol the way xmit does? Or should the
> driver refuse 802.1ad bridges?

The driver does not support 802.1ad bridges, and rcv is only one part
of that. The hardware forwarding path does not follow the bridge
vlan_protocol. The classifier treats both TPIDs as VLAN tags, and the
egress TPID follows the ingress tag type. Restoring the tag with the
bridge protocol in rcv would hide one symptom without making 802.1ad
work. On xmit the protocol is taken from the bridge because the stack
picks the TPID of the frames it sends.

In v15 I will refuse VLAN filtering on a bridge whose vlan_protocol is
not 802.1Q. That covers creating or joining a VLAN-aware 802.1ad bridge,
but not a later protocol change, since DSA does not propagate
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL.


> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-dsa_lan9645x_switch_driver_base-v14-0-3d2a72661213%40microchip.com

pw-bot: cr