Re: [PATCH net-next v6 09/12] net: phylink: add .pcs_link_down PCS OP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Memory leak/Use-After-Free due to missing notifier unregistration.
- [High] Hardware Teardown Sequence Error violating symmetric LIFO teardown.
- [High] Data Race and list corruption in `pcs_list`.
- [Low] Incorrect grammar in callback documentation.
--

commit db8b84b7398b2a4cfe29ace55d1aefced615f8e2
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add .pcs_link_down PCS OP

This commit introduces a new pcs_link_down callback for PCS drivers to define
specific teardown operations. It updates phylink_link_down to invoke this
new operation before taking down the MAC. This allows PCS drivers to perform
necessary resets or counter clearing when the link goes down.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 65b67c33e12fd..38428b375eca3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1735,6 +1741,8 @@ static void phylink_link_down(struct phylink *pl)
>  
>  	phylink_deactivate_lpi(pl);
>  
> +	phylink_pcs_link_down(pl->pcs);
> +
>  	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
>  				   pl->cur_interface);

[Severity: High]
Does this break the symmetric LIFO teardown order? During phylink_link_up(),
the PCS is initialized before the MAC. Calling phylink_pcs_link_down() before
mac_link_down() leaves the MAC nominally active while the underlying PCS
connection is severed. Could this inconsistency lead to MAC hardware lockups,
AXI bus hangs, or transmit queue timeouts?

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index b65f91c78d8cc..79a1496fa519c 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -528,6 +528,7 @@ struct phylink_pcs {
>   * @pcs_an_restart: restart 802.3z BaseX autonegotiation.
>   * @pcs_link_up: program the PCS for the resolved link configuration
>   *               (where necessary).
> + * @pcs_link_down: torn down link between MAC and PCS.

[Severity: Low]
this isn't a bug, but there is a grammar mistake here (torn down instead of
tear down).

[Severity: Critical]
Is there a missing notifier unregistration for the fwnode_pcs_nb block?
Looking at phylink_create(), it registers the embedded notifier block:

drivers/net/phy/phylink.c:phylink_create() {
        ...
        register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
        ...
}

However, phylink_destroy() and the error paths in phylink_create() do not
seem to unregister it. When the phylink struct is freed via kfree(pl), the
embedded notifier remains linked in the global chain. Will any subsequent
PCS provider registration system-wide iterate over the freed memory and cause
a kernel panic? (Note: introduced by the earlier patch net: phylink: support
late PCS provider attach in the same series).

[Severity: High]
Is there a data race when mutating pcs_list in phylink_destroy()?

The pcs_provider_notify() callback asynchronously adds newly discovered PCS
providers to pl->pcs_list while holding the rtnl_lock(). However,
phylink_destroy() iterates over and deletes entries from pl->pcs_list without
holding the rtnl_lock():

drivers/net/phy/phylink.c:phylink_destroy() {
        ...
        /* Remove every PCS from phylink PCS list */
        list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
                list_del(&pcs->list);
        ...
}

If there is a concurrent PCS provider registration via the global notifier
chain, could this lead to unsynchronized mutations and list corruption?
(Note: introduced by the earlier patch net: phylink: support late PCS
provider attach in the same series).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609151212.29469-1-ansuelsmth@gmail.com?part=9