Re: [PATCH net 06/10] net: pse-pd: do not tie a shared power domain to its creator's devres

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 0

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] The shared power domain still depends on its creator's devres
  through its supply handle.
- [Medium] A partial failure of pse_register_pw_ds() is never unwound, so
  the left-over domains stay live and keep being reused.
- [Medium] pse_flush_pw_ds() now clears pcdev->pi[i].pw_d to NULL while
  the PI regulators are still registered.

--

net: pse-pd: do not tie a shared power domain to its creator's devres

This patch allocates PSE power domains with kzalloc() instead of devm
memory and frees them on the last kref_put(). pse_flush_pw_ds() now
clears pi[].pw_d under pcdev->lock before the put, and
pse_ethtool_set_prio() checks pw_d under that lock.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index d5f863834179c..0d1e5d5039f8a 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -898,10 +899,18 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
>  	int i;
>  
>  	for (i = 0; i < pcdev->nr_lines; i++) {
> -		if (!pcdev->pi[i].pw_d)
> +		/* Clear it before the put that may free the domain. The PI
> +		 * regulator ops and pse_isr() hold pcdev->lock while they
> +		 * use it.
> +		 */
> +		mutex_lock(&pcdev->lock);
> +		pw_d = pcdev->pi[i].pw_d;
> +		pcdev->pi[i].pw_d = NULL;
> +		mutex_unlock(&pcdev->lock);

[Severity: Medium]
Can the PI regulator ops still run here and see pw_d == NULL?

The PI regulators are devm registered before the pcdevp devres entry.
That means they stay registered until after pse_controller_unregister()
returns:

pse_controller_unregister()
    pse_flush_pw_ds(pcdev);
    pse_release_pis(pcdev);

During that window a consumer, such as a PHY's pse_control driven through
ethtool, can call pse_pi_enable(). It would then skip the software power
control branch:

	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {
		...
	}

	ret = ops->pi_enable(pcdev, id);

So the port is powered without the PD detection gate and without a
budget charge.

The disable side has a similar gap. _pse_pi_disable() calls
pse_pi_deallocate_pw_budget(), which returns early:

	if (!pi->pw_d || !pi->pw_allocated_mW)
		return;

As a result, regulator_free_power_budget() is never called for a supply
that another controller may still share.

Before this patch, pw_d stayed valid during this window. The commit message
says "The PI regulator ops and pse_isr() use it under the same lock". Does
that cover only memory safety, and not this new no-domain state?

Separately, there is a pre-existing issue that this patch did not
introduce: pse_release_pis() frees pcdev->pi right after the flush while
the regulators are still live. The pse_isr() part does not seem to apply
to in-tree drivers. For tps23881, devm_pse_irq_helper() is registered
after the controller, so devres frees the IRQ first.

> +		if (!pw_d)
>  			continue;

[ ... ]

> @@ -974,7 +989,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
>  			continue;
>  		}
>  
> -		pw_d = devm_pse_alloc_pw_d(pcdev->dev);
> +		pw_d = pse_alloc_pw_d();
>  		if (IS_ERR(pw_d)) {
>  			ret = PTR_ERR(pw_d);
>  			goto out;
> @@ -983,6 +998,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)
>  		supply = regulator_get(&rdev->dev, rdev->supply_name);

[Severity: High]
Is the shared domain still tied to its creator through this supply handle?

The consumer device here is &rdev->dev, where rdev is a PI regulator of
the creating controller. devm_pse_pi_regulator_register() registers that
regulator with devm_regulator_register() on the creator's device.

In the regulator core, _regulator_get_common()->link_and_create_debugfs()
stores the pointer without taking a reference:

	regulator->dev = dev;

The only reference comes from the DL_FLAG_STATELESS device link, and
device_del() of the PI rdev purges that link.

With this patch, controller B can keep the domain alive after creator A
unbinds. devres then unregisters and frees A's PI regulators:

regulator_unregister()
    device_unregister(&rdev->dev)
        regulator_dev_release()

pw_d->supply stays on the supply's consumer_list with ->dev pointing at
freed memory. Reading /sys/kernel/debug/regulator/regulator_summary then
reaches regulator_summary_show_subtree():

	if (consumer->dev && consumer->dev->class == &regulator_class)

Would that be a use-after-free read of the freed struct device?

When B finally puts the domain, destroy_regulator() passes the stale
pointer to device_link_remove(regulator->dev, &rdev->dev). The address may
have been reused by another consumer of the same supply. Could that drop
that consumer's device link?

A domain left over from a failed pse_register_pw_ds() has the same stale
->dev, and it stays in pse_pw_d_map where later controllers can pick it up.

The new pse_alloc_pw_d() comment says the domain "must not be tied to the
lifetime of the controller that happened to create it". Should the supply
handle be acquired with a consumer device that the creator does not own?
The rest of the series leaves this code unchanged.

>  		if (IS_ERR(supply)) {
>  			xa_erase(&pse_pw_d_map, pw_d->id);
> +			kfree(pw_d);
>  			ret = PTR_ERR(supply);
>  			goto out;
>  		}

[Severity: Medium]
What happens to the PIs handled before the failing one?

When pse_alloc_pw_d() or regulator_get() fails for PI i, only the current
iteration is cleaned up. Each of PIs 0..i-1 did one of two things:

- took kref_get() on an existing domain, possibly another controller's
- created a new domain in pse_pw_d_map holding a supply handle

pse_controller_register() then returns without a flush:

	ret = pse_register_pw_ds(pcdev);
	if (ret)
		return ret;

devm_pse_controller_register() only calls devres_free(pcdevp), so
pse_controller_unregister()->pse_flush_pw_ds() never runs.

Does this leak the extra references on other controllers' domains, so they
can never be released?

The new domains stay in the global map. Their supply consumer's ->dev is
the failed controller's PI rdev, and devres unregisters that rdev as probe
unwinds. A later pse_register_pw_ds() can find such a domain through
regulator_is_equal() and kref_get() it, so its refcount never returns to
zero.

The commit message says:

    A domain left over from a failed pse_register_pw_ds() is now leaked
    instead of freed while still in the map.

The leaked domain is still published in the map and still carries a
dangling consumer device. Would calling pse_flush_pw_ds(pcdev) on the
pse_register_pw_ds() error path in pse_controller_register() be enough?
pcdev->lock is already initialized there, and the flush already skips PIs
whose pw_d is NULL.

[ ... ]

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