Re: [PATCH net v2 07/14] 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 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0 (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] At this commit, the shared power domain still depends on its
  creator's lifetime through pw_d->supply.
- [Medium] At this commit, a partial failure in pse_register_pw_ds()
  leaks the power domains that were already set up.

Pre-existing issues:
- [High] At this commit, pse_controller_unregister() calls
  pse_flush_pw_ds() and then pse_release_pis(), which kfree()s pcdev->pi…
- [Low] pse_register_pw_ds() publishes pcdev->pi[i].pw_d (line 978 for a
  shared domain, line 1007 for a new one) holding only pse_pw_d_mutex,…

--

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

This commit allocates PSE power domains with kzalloc() instead of
devm_kzalloc(), and frees them from __pse_pw_d_release() 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 36f7dba8190a2..13d6bd29525f3 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -893,10 +894,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: High]
This is a pre-existing issue, but pcdev->lock here protects the pw_d
pointer, not the pcdev->pi array that holds it. Can the PI regulator ops
or pse_isr() still reach a freed pcdev->pi after this?

pse_controller_unregister() does:

	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	if (pcdev->irq)
		disable_irq(pcdev->irq);

pse_release_pis() ends with kfree(pcdev->pi) and does not take
pcdev->lock.

The IRQ is disabled only after the free. The devm-registered PI
regulators are unregistered even later by devres, because
devm_pse_controller_release() runs before them in reverse devres order.

In that window, a sysfs "state" read or a consumer's regulator_disable()
can reach pse_pi_is_enabled() or pse_pi_disable(). They do:

	mutex_lock(&pcdev->lock);
	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {

and that reads the freed array. If the driver's IRQ outlives unregister,
pse_isr() also reads pcdev->pi[i] under the same lock.

Later commits in the series fix this. "net: pse-pd: stop async event
sources before freeing PI data in unregister" moves disable_irq() and
cancel_work_sync() to the start. "net: pse-pd: keep the PI data until the
PI regulators are gone" frees pcdev->pi from a devm action registered
before the PI regulators, and sets pcdev->unregistering under
pcdev->lock. One of those commit messages shows a KASAN
slab-use-after-free in pse_pi_is_enabled().

[Severity: Low]
This is also a pre-existing issue. The comment says pw_d users rely on
pcdev->lock, but the writer in pse_register_pw_ds() doesn't take it. It
publishes the pointer while holding only pse_pw_d_mutex:

		if (pcdev->supp_budget_eval_strategies)
			pw_d->budget_eval_strategy = pcdev->supp_budget_eval_strategies;
		else
			pw_d->budget_eval_strategy = PSE_BUDGET_EVAL_STRAT_DISABLED;
		kref_init(&pw_d->refcnt);
		pcdev->pi[i].pw_d = pw_d;

The shared-domain branch does the same with pcdev->pi[i].pw_d = pw_d.

devm_pse_pi_regulator_register() has already registered the PI
regulators earlier in pse_controller_register().

So a concurrent sysfs "state" read could run pse_pi_is_enabled() and see
a non-NULL pw_d with a stale budget_eval_strategy. Would it then fall back
to the pse_pi_is_hw_enabled() path?

The impact looks limited to one wrong is_enabled answer during probe. The
publish path is still unlocked at the end of the series.

>  			continue;
>  
> -		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
> +		pw_d = xa_load(&pse_pw_d_map, pw_d->id);

[ ... ]

> @@ -969,7 +984,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;
> @@ -978,6 +993,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's lifetime through
pw_d->supply?

regulator_get() is called with &rdev->dev, and rdev is this controller's
devm-registered PI regulator. The regulator core records rdev->dev as the
consumer device and adds the handle to the parent supply's consumer_list.
The handle is then stored in pw_d->supply.

With this patch the domain can outlive its creator. Suppose the creating
controller unbinds while another controller still holds a reference. Its
PI rdev is unregistered and freed, but pw_d->supply->dev still points to
it.

Would a read of the debugfs regulator_summary then dereference the freed
device through consumer->dev in regulator_summary_show_subtree()? And
would the final put also use the stale dev pointer?

__pse_pw_d_release()
  regulator_put(pw_d->supply)
    destroy_regulator()

This conflicts with the new kernel-doc on pse_alloc_pw_d(), which says
the domain "must not be tied to the lifetime of the controller that
happened to create it".

The later commit "net: pse-pd: use each PI's own supply handle for the
power budget" removes the supply member from struct pse_power_domain. Its
commit message shows a KASAN slab-use-after-free in
regulator_summary_show_subtree().

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

[Severity: Medium]
Does a failure partway through this loop leak the domains already set up
for earlier PIs?

Each earlier iteration either kref_init()s a new kzalloc'd domain or
kref_get()s a shared one, and stores it in pcdev->pi[j].pw_d. If
pse_alloc_pw_d() or regulator_get() then fails for a later PI, the code
jumps to:

out:
	mutex_unlock(&pse_pw_d_mutex);
	return ret;

That label does no unwinding, and pse_controller_register() returns
without calling pse_flush_pw_ds():

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

The domain is no longer devm memory, so nothing frees it. The struct, its
supply handle and its xarray slot all leak, and a shared domain keeps one
extra reference.

The commit message mentions this ("is now leaked instead of freed while
still in the map"). The later commit "net: pse-pd: unwind allocations
when controller registration fails" adds an unwind loop using
pse_pi_take_pw_d() and pse_pw_d_leave() before pse_pw_d_mutex is dropped.

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