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

[netdev-bot+sashiko@kernel.org]

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?

> +	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