Re: [PATCH net v3 10/14] net: pse-pd: keep the PI data until the PI regulators are gone

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] This patch frees pcdev->pi from a devm action.
- [Low] The commit message has only 'Fixes: d83e13761d5b' and does not
  mention that the fix depends on two earlier patches in the series:…

--

net: pse-pd: keep the PI data until the PI regulators are gone

This patch stops freeing the PSE PI array in pse_controller_unregister().
The array is now freed from a devm action that is registered before the
PI regulators, so devres frees it after the regulators are unregistered.
This also fixes a leak of the PI array when registration fails.

> Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")

[Severity: Low]

Should the commit message say that this patch depends on two earlier
patches in the series?

  "net: pse-pd: do not tie a shared power domain to its creator's devres"
  "net: pse-pd: stop async event sources before freeing PI data in
  unregister"

On the baseline, pse_flush_pw_ds() does not clear pcdev->pi[i].pw_d. The
domain is allocated with devm_kzalloc() in pse_register_pw_ds(), and that
runs after the PI regulators are registered. So devres frees the domain
before it unregisters the regulators.

Suppose this patch is backported without the power domain patch. The
reproducer in the message would then still go through:

pse_pi_is_enabled()
  pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)

It would read pw_d->budget_eval_strategy from freed devm memory, so the
use-after-free would just move from pi[] to pi[].pw_d.

The unregistering gate has the same kind of dependency. Without it, a
consumer enable after unregister would get past the pi[] access and into
pse_disable_pi_pol(). That function uses the kfifo and the work item,
and unregister has already torn both down.

Both prerequisites have their own Fixes tags (50f8b341d268 and
ffef61d6d273), so this only matters for a partial stable backport.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 43f6e304fe5e6..eccbdb3acbee8 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -1123,6 +1128,13 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	if (ret)
>  		return ret;
>  
> +	/* The PI regulator ops use pcdev->pi until devres unregisters the
> +	 * regulators, so free it from devres too, after them.
> +	 */
> +	ret = devm_add_action_or_reset(pcdev->dev, devm_pse_release_pis, pcdev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]

Does this devres ordering cover the case where a consumer enables or
disables the PI, which the commit message also mentions?

regulator_unregister() does not take the rdev lock and does not wait for
consumer calls that are already running. If consumers still hold handles,
it only does WARN_ON(rdev->open_count).

pse_control_get_internal() takes the PSE consumer handle with the
non-devm regulator_get_exclusive():

    psec->ps = regulator_get_exclusive(pcdev->dev,
                       rdev_get_name(pcdev->pi[index].rdev));

The PHY only drops that handle in phy_device_remove()->pse_control_put().
pse_controller_unregister() never walks pcdev->pse_control_head, so these
handles survive the controller unbind.

I can see two ways to hit this. The first is a consumer disable racing
with the unbind:

CPU0 (for example ethtool PSE admin-disable)
pse_ethtool_set_config()
  regulator_disable(psec->ps)
    pse_pi_disable()
      pi = &pcdev->pi[id];
      mutex_lock(&pcdev->lock);
      _pse_pi_disable()
        ops->pi_disable()   /* sleeps on I2C */

CPU1 (unbind)
devres release
  regulator_unregister()
  devm_pse_release_pis()
    kfree(pcdev->pi)        /* without pcdev->lock */
  frees the driver priv that embeds pcdev

When CPU0 returns from ops->pi_disable(), it runs:

      pi->admin_state_enabled = 0;
      mutex_unlock(&pcdev->lock);

Would that write into freed memory? pse_pi_disable() has no
pcdev->unregistering check.

The second needs no race. If the PHY is removed after the controller has
unbound, __pse_control_release() runs:

    if (psec->pcdev->pi[psec->id].admin_state_enabled)
        regulator_disable(psec->ps);

That reads the freed pi array through the freed pcdev. regulator_disable()
then reaches pse_pi_disable(), which uses the same freed data.

pcdev itself is freed in the same teardown. Can any ordering of the PI
array free close these paths? Would the existing pse_control objects have
to be revoked in pse_controller_unregister(), or pcdev refcounted, before
the consumer case is fixed?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103220217.1131971-1-github%40szelinsky.de