Re: [PATCH net-next v13 7/9] net: dsa: lan9645x: add mdb management

[Jens Emil Schulz Ostergaard]

On Fri, 2026-10-02 at 21:14 +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 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 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] On a failed MAC table write, lan9645x_mdb_widen_dest() drops
>   the reference on new_pgid right away…
> - [Low] lan9645x_mdb_narrow_dest() commits mdb_entry->pgid = new_pgid and
>   mdb_entry->ports = new_ports before calling lan9645x_mdb_write_dest().
> - [Low] A failed CPU-only membership delete leaks a PGID reference
>   because old_pgid and new_pgid are the same object.
> 
> --
> 
> net: dsa: lan9645x: add mdb management
> 
> This adds the DSA mdb callbacks to the lan9645x driver. IP multicast
> stores the port group mask inline in the mac table entry. L2 multicast
> points at a refcounted PGID whose destination mask is the port group.
> CPU membership is carried by the MAC_CPU_COPY bit, and when the PGIDs
> run out, add and delete handle it differently.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > new file mode 100644
> > index 0000000000000..92ab5f55f7913
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mdb.c
> > @@ -0,0 +1,569 @@
> 
> [ ... ]
> 
> > +static int lan9645x_mdb_widen_dest(struct lan9645x *lan9645x,
> > +                                struct lan9645x_mdb_entry *mdb_entry,
> > +                                enum macaccess_entry_type type,
> > +                                struct lan9645x_pgid_entry *new_pgid,
> > +                                int pgid_index, u16 new_ports)
> > +{
> > +     struct lan9645x_pgid_entry *old_pgid = mdb_entry->pgid;
> > +     int err;
> > +
> > +     lockdep_assert_held(&lan9645x->mdb_lock);
> > +
> > +     err = lan9645x_mdb_write_dest(lan9645x, mdb_entry, type, pgid_index,
> > +                                   new_ports);
> > +     if (err) {
> > +             lan9645x_pgid_entry_put(lan9645x, new_pgid);
> > +             return err;
> > +     }
> 
> [Severity: Medium]
> Can this hand a PGID index to an unrelated group while hardware still
> points at it?
> 
> __lan9645x_mact_learn() issues the command before it polls for
> completion:
> 
>     lan_wr(ANA_MACACCESS_VALID_SET(1) |
>            ANA_MACACCESS_DEST_IDX_SET(port) |
>            ...
>            ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_LEARN),
>            lan9645x, ANA_MACACCESS);
> 
>     return lan9645x_mac_wait_for_completion(lan9645x, NULL);
> 
> That means an error from lan9645x_mdb_write_dest() only tells us the
> outcome is unknown, for example after a failed poll read or a timeout.
> The entry may already point at pgid_index.
> 
> If new_pgid was just created, this put frees its index. A later add
> with a different port mask can get the same index back from
> lan9645x_mdb_pgid_entry_create(). lan9645x_pgid_entry_alloc() then
> rewrites ANA_PGID for that group, so an entry that may have been
> written would forward this group to the other group's ports.
> 
> When the entry already existed (ports != 0), __lan9645x_mdb_add() does
> no further cleanup. For a fresh entry it issues the forget only after
> the PGID has been released, and it ignores the result:
> 
>     lan9645x_mact_forget(lan9645x, mdb_entry->mac,
>                          mdb_entry->vid, type);
>     lan9645x_mdb_entry_dealloc(lan9645x, mdb_entry);
> 
> The comment there says the forget "fails harmlessly if the bus is still
> down". Isn't that the case where an entry pointing at a freed PGID is
> left in hardware?
> 
> The delete side handles the same uncertainty by keeping the reference.
> lan9645x_mdb_narrow_dest() keeps old_pgid on failure, and
> __lan9645x_mdb_del() sets mdb_entry->pgid = NULL when CMD_FORGET fails.
> The v12 changelog gives the reason: "so the index cannot be handed to an
> unrelated group while hardware may still point at it".
> 
> Should the add path also keep the new_pgid reference when the write
> fails?
> 

Correct, an error from the learn command leaves the hardware state
unknown, and the add path dropped the PGID reference anyway. v14 keeps
the reference on a failed write, as the delete path does, so the index
cannot be reused while hardware may point at it.


> > +     mdb_entry->pgid = new_pgid;
> > +     mdb_entry->ports = new_ports;
> > +     lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +     return 0;
> > +}
> 
> [ ... ]
> 
> > +static void lan9645x_mdb_narrow_dest(struct lan9645x *lan9645x,
> > +                                  struct lan9645x_mdb_entry *mdb_entry,
> > +                                  enum macaccess_entry_type type,
> > +                                  struct lan9645x_pgid_entry *new_pgid,
> > +                                  int pgid_index, u16 new_ports)
> > +{
> > +     struct lan9645x_pgid_entry *old_pgid = mdb_entry->pgid;
> > +     int err;
> > +
> > +     lockdep_assert_held(&lan9645x->mdb_lock);
> > +
> > +     mdb_entry->pgid = new_pgid;
> > +     mdb_entry->ports = new_ports;
> 
> [Severity: Low]
> mdb_entry->pgid is switched to new_pgid before the hardware write. If
> that write fails, which PGID does the entry use afterwards? Hardware
> would still use old_pgid, while software records new_pgid.
> 
> The in-place branch in __lan9645x_mdb_del() assumes hardware points at
> mdb_entry->pgid, so it only rewrites ANA_PGID:
> 
>     if (mdb_entry->pgid && refcount_read(&mdb_entry->pgid->refcount) == 1) {
>         mdb_entry->pgid->ports = new_ports &
>                                  ~BIT(lan9645x->num_phys_ports);
>         ...
>         lan_rmw(ANA_PGID_PGID_SET(mdb_entry->pgid->ports), ...);
>         return 0;
>     }
> 
> For example, take a group {A,B,C} on PGID P:
> 
> 1. Deleting C creates Q={A,B} with the last free index, and the MAC
>    write fails.
> 
> 2. Deleting B finds no PGID for {A}, so creation returns -ENOSPC.
> 
> 3. Q has refcount 1, so it is narrowed in place.
> 
> Hardware still forwards through P, so B and C keep receiving the group,
> and nothing further is logged.
> 
> Should the in-place branch, or a later narrowing, rewrite the MAC entry
> after an earlier failed write?
> 
> > +
> > +     err = lan9645x_mdb_write_dest(lan9645x, mdb_entry, type, pgid_index,
> > +                                   new_ports);
> > +     if (err) {
> > +             dev_err(lan9645x->dev,
> > +                     "Narrowing %pM vid %u to mask 0x%x returned %pe\n",
> > +                     mdb_entry->mac, mdb_entry->vid, new_ports,
> > +                     ERR_PTR(err));
> > +             return;
> > +     }
> 
> [Severity: Low]
> Does this early return leak a PGID reference when old_pgid and new_pgid
> are the same object?
> 
> Take a group {A, CPU} on PGID P, whose ports are {A}. When only the CPU
> port module leaves, lan9645x_mdb_pgid_entry_get() strips the CPU bit.
> lan9645x_mdb_pgid_entry_lookup() then returns P after refcount_inc().
> Now new_pgid == old_pgid == P, and P holds two references for a single
> mdb entry.
> 
> If lan9645x_mdb_write_dest() fails here, the put below is skipped. When
> the group is removed later, lan9645x_mdb_entry_dealloc() drops only one
> reference. P and its index then stay reserved until
> lan9645x_mdb_deinit().
> 
> Keeping old_pgid makes sense when it differs from new_pgid. Here,
> though, mdb_entry->pgid already holds P. Should the put still run when
> old_pgid == new_pgid?
> 
> > +
> > +     lan9645x_pgid_entry_put(lan9645x, old_pgid);
> > +}
> 
> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com