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

[Jens Emil Schulz Ostergaard]

On Mon, 2026-10-19 at 10:08 +0000, netdev-bot+sashiko@xxxxxxxxxx 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 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1
> 
> 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), the VLAN tag is
>   restored only if the software VLAN-filtering flag is set: `if…
> - [Low] In lan9645x_rcv() (net/dsa/tag_lan9645x.c), the comment above
>   skb_postpull_rcsum() lists what is subtracted from skb->csum as "what…
> 
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but
>   lan9645x_xmit() calls dsa_port_bridge_dev_get(dp) (`dp->bridge ?
> 
> --
> 
> net: dsa: add tag driver for LAN9645X
> 
> This adds a DSA tag driver for the Microchip LAN9645x switch. A front port
> is used as the NPI (CPU) port. Extraction uses a 16 byte long prefix plus
> a 28 byte IFH, and injection uses only the IFH.
> 
> On receive, a VLAN tag popped by the hardware is restored from the IFH as
> a hwaccel C-tag. On transmit, the VLAN tag of a VLAN-aware port 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 is it safe to dereference
> dp->bridge here?
> 
> lan9645x_xmit() runs without rtnl, and dsa_port_bridge_dev_get() uses
> plain loads with no READ_ONCE() or RCU:
> 
> include/net/dsa.h:dsa_port_bridge_dev_get() {
>         return dp->bridge ? dp->bridge->dev : NULL;
> }
> 
> At the same time, dsa_port_bridge_leave()->dsa_port_bridge_destroy() runs
> under rtnl and frees the dsa_bridge with no grace period:
> 
> net/dsa/port.c:dsa_port_bridge_destroy() {
>         ...
>         dp->bridge = NULL;
> 
>         if (!refcount_dec_and_test(&bridge->refcount))
>                 return;
>         ...
>         kfree(bridge);
> }
> 
> Suppose a transmit loads dp->bridge before the NULL store, and the last
> port leaves the bridge before it reads bridge->dev. Could that read be a
> use-after-free of the dsa_bridge?
> 
> tag_dsa.c, tag_ocelot.c, tag_sja1105.c, tag_vsc73xx_8021q.c and
> dsa_software_vlan_untag() in net/dsa/tag.h already use this pattern. The
> root cause looks like the way DSA core manages the lifetime of struct
> dsa_bridge, not this tagger.


Dismissed, preexisting.

As the review notes, this is the 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 a grace period, so a tagger cannot fix it 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 in lan9645x_rcv() doesn't match the
> length passed here. "what remains of the prefix, the ifh and the gap"
> comes to 2 + 28 + gap bytes.
> 
> The call subtracts LAN9645X_TOTAL_TAG_LEN + ifh_gap_len, which is
> 44 + gap bytes. That range also covers the 14 byte real ethernet header.
> 
> The code looks correct, because dsa_switch_rcv() later calls
> skb_push(ETH_HLEN) and eth_type_trans() without adjusting csum. Could the
> comment also list the real ethernet header?
> 
> [ ... ]
> 
> > +     dp = dsa_user_to_port(skb->dev);
> > +
> > +     if (dsa_port_is_vlan_filtering(dp) && ifh_gap_len) {
> 
> [Severity: Medium]
> Can a frame lose its VLAN tag here while VLAN filtering is being turned
> on?
> 
> lan9645x_rcv() strips the gap whenever the hardware reports a pop, but
> it only restores the tag if the software flag is also set. Turning on
> vlan_filtering happens in this order:
> 
> br_vlan_filter_toggle()
>   br_opt_toggle(br, BROPT_VLAN_ENABLED, !!val)  /* bridge VLAN-aware */
>   switchdev_port_attr_set()
>     dsa_port_vlan_filtering()
>       ds->ops->port_vlan_filtering()
>         lan9645x_port_vlan_filtering()
>           lan9645x_vlan_port_apply()
>             lan9645x_vlan_port_apply_ingress()
>               ANA_VLAN_CFG_VLAN_POP_CNT_SET(1)  /* hw starts popping */
>       dp->vlan_filtering = vlan_filtering;      /* tagger flag set */
> 
> Take a frame extracted after the POP_CNT write but handled before the
> dp->vlan_filtering store. It arrives with ifh_gap_len == 4.
> lan9645x_rcv() strips the gap but skips __vlan_hwaccel_put_tag().
> 
> The bridge is already VLAN-aware, so it sees an untagged frame. It
> classifies the frame to the port pvid, or drops it if there is no pvid,
> and the original VID and PCP are lost.
> 
> The same window opens when a port joins a bridge that already filters,
> through dsa_port_switchdev_sync_attrs()->dsa_port_vlan_filtering().
> 
> The hardware only pops tags on a bridged, VLAN-aware port. Would it be
> enough to restore the tag based on ifh_gap_len alone?

Correct. The next version restores the tag whenever ifh_gap_len is non-zero,
without looking at dp->vlan_filtering.


> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015-dsa_lan9645x_switch_driver_base-v15-0-562f316f78b9%40microchip.com

pw-bot: cr