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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless traversal of `pl->pcs_list` and unprotected modification of `pl->pcs` in `phylink_start()` and `phylink_resume()`.
--

--- Patch [9]: [PATCH net-next v11 09/12] net: phylink: add .pcs_link_down PCS OP ---
commit 7a2f38b91902153c2c82a86f0b98d881f66246ff
Author: Christian Marangi <[email protected]>

net: phylink: add .pcs_link_down PCS OP

This commit adds a .pcs_link_down operation to permit a PCS driver to define
specific actions to tear down the link between the MAC and the PCS. It hooks
into phylink_link_down() to call this new operation when tearing down the link.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 69d4f6675db0f..2e8d11573e3ad 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1683,6 +1689,9 @@ static void phylink_link_down(struct phylink *pl)
>  
>  	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
>  				   pl->cur_interface);
> +
> +	phylink_pcs_link_down(pl->pcs);
> +
>  	phylink_info(pl, "Link is Down\n");
>  }

[Severity: High]
Does this sequence permit concurrent list traversal and modification of
pl->pcs_list?

Looking at phylink_start(), it initiates the configuration path without
acquiring pl->state_mutex:

phylink_start()
  phylink_mac_initial_config(pl, true)
    phylink_major_config()
      list_for_each_entry(pcs, &pl->pcs_list, list) {
        ...
      }

Concurrently, pcs_provider_notify() modifies the list under pl->state_mutex
but without rtnl_lock:

pcs_provider_notify()
  mutex_lock(&pl->state_mutex);
  ...
  phylink_add_pcs(pl, pcs);
    list_add_tail(&pcs->list, &pl->pcs_list);

Can this lead to list corruption if a PCS provider probe races with
a network interface startup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9