Re: [PATCH net-next v8 2/7] net: pse-pd: fire lifecycle events on controller register/unregister

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (2) · Medium: 1 (2) · 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] Work can be re-queued after cancel_work_sync() by pse_control
  holders that do not subscribe (baseline decision: discarded, fixed…
- [Medium] A port could be powered on inside PSE_UNREGISTERED while the
  irq is masked (baseline decision: discarded).
- [Low] Fixes: tag missing for the teardown race fixed by the reorder
  (baseline decision: discarded).

Pre-existing issues:
- [High] Shared PSE power domain is freed by devres while other
  controllers still use it (pre-existing).
- [High] Stale pse_control handles reach freed pcdev->pi and pcdev after
  unbind (pre-existing, baseline decision: discarded, fixed later in the…
- [Medium] disable_irq() without a matching enable on a line the PSE core
  may not own (pre-existing, baseline decision: discarded).
- [Medium] pse_controller_register() leaks memory on its error paths
  (pre-existing, baseline decision: discarded).

--

net: pse-pd: fire lifecycle events on controller register/unregister

This patch hooks up the pse_controller_notifier chain.
pse_controller_register() now fires PSE_REGISTERED once the controller is
on pse_controller_list, and pse_controller_unregister() fires
PSE_UNREGISTERED after the controller has been unlinked. It also reorders
pse_controller_unregister(): the irq is disabled and the controller is
unlinked before the event, and cancel_work_sync() and the frees move
after it.

> No subscriber exists yet, so the event itself does nothing. The
> reordering of pse_controller_unregister() around it is not a no-op,
> though: it closes a teardown race that is reachable today, with no
> subscriber involved.

[Severity: Low]
The race described here is real. In the baseline, pse_release_pis() frees
pcdev->pi while the controller is still on pse_controller_list. That lets
of_pse_control_get()->of_pse_match_pi() read the freed pcdev->pi[i].np.

The notes say a separate net series reorders the same function as a
standalone fix. That looks like the right place for a Fixes: tag, because
this commit depends on the notifier from "net: pse-pd: add notifier chain
for controller lifecycle events".

Once the net fix lands, will this paragraph be out of date? Could the
message refer to the net fix instead of saying that this commit closes
the race?

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 84c734ed4553e..dc261beb61702 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -1138,6 +1138,9 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	list_add(&pcdev->list, &pse_controller_list);
>  	mutex_unlock(&pse_list_mutex);
>  
> +	blocking_notifier_call_chain(&pse_controller_notifier,
> +				     PSE_REGISTERED, pcdev);
> +
>  	return 0;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but the error paths in
pse_controller_register() leak memory. Every failure after this call
returns without kfifo_free():

	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);

Failures after of_load_pse_pis() also leak pcdev->pi and its of_node
references. A partial pse_register_pw_ds() leaks pw_d krefs and supplies.

The next patch in the series, "net: pse-pd: unwind allocations when
controller registration fails", adds the free_kfifo and release_pis
labels. It also calls pse_flush_pw_ds() when pse_register_pw_ds() fails.
It still leaks the PI array on purpose once PI regulators exist.

kfifo_alloc() is sized from pcdev->nr_lines before the nr_lines = 1
default is applied. No in-tree driver registers with nr_lines below 2.
Should the kfifo be sized after that default is set?

> @@ -1148,15 +1151,41 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
>   */
>  void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  {
> -	pse_flush_pw_ds(pcdev);
> -	pse_release_pis(pcdev);
> +	/* Raise the interrupt's disable depth before anything is freed.
> +	 * pse_isr() queues notifications and reaches pcdev->pi, and nothing
> +	 * below re-enables it. For a driver that requests its irq after
> +	 * devm_pse_controller_register(), devres has already run free_irq()
> +	 * by the time we get here and this only bumps the depth - the
> +	 * ordering does not rely on that, so a driver requesting the irq
> +	 * earlier is covered too.
> +	 */
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);

[Severity: Medium]
This is a pre-existing issue, but can this disable_irq() mask an
interrupt line that the PSE core never owned?

devm_pse_irq_helper() records the irq even when the request fails:

drivers/net/pse-pd/pse_core.c:devm_pse_irq_helper() {
    ...
	ret = devm_request_threaded_irq(dev, irq, NULL, pse_isr,
					IRQF_ONESHOT | irq_flags,
					irq_name, h);
	if (ret)
		dev_err(pcdev->dev, "Failed to request IRQ %d\n", irq);

	pcdev->irq = irq;
	return ret;
}

Suppose tps23881's request fails with -EBUSY because another device holds
the line exclusively. Probe then unwinds through
devm_pse_controller_release(), and this disable_irq() masks the other
device's interrupt. Nothing re-enables it afterwards.

The new comment is correct for the in-tree success case. tps23881 passes
irq_flags = 0, devres has already run free_irq(), and the next request
goes through irq_startup(), which resets the depth to 0.

>  
> -	cancel_work_sync(&pcdev->ntf_work);
> -	kfifo_free(&pcdev->ntf_fifo);
> +
> +	/* Unlink before the event: of_pse_control_get() walks
> +	 * pse_controller_list and dereferences pcdev->pi[] through
> +	 * of_pse_match_pi(), so no lookup may still reach this controller
> +	 * once its teardown starts. Subscribers are handed pcdev as the
> +	 * event data, so the notifier does not need it on the list.
> +	 */
>  	mutex_lock(&pse_list_mutex);
>  	list_del(&pcdev->list);
>  	mutex_unlock(&pse_list_mutex);
> +
> +	blocking_notifier_call_chain(&pse_controller_notifier,
> +				     PSE_UNREGISTERED, pcdev);

[Severity: Medium]
With disable_irq() now running before the event, can a port be powered on
from inside the PSE_UNREGISTERED walk?

A subscriber's final put can follow this path for a pending PI on the
same power domain:

__pse_control_release()
  regulator_disable()
    _pse_pi_disable()
      pse_pw_d_retry_power_delivery()
        ops->pi_enable()

That decision uses an isr_pd_detected value that can no longer change,
because the irq is already masked.

The commit message says this is intended ("so the call is legal"). In
the final series the power-on also looks brief. A PI can only be pending
after pse_pi_enable() on the exclusive psec->ps consumer, so it always
has a holder. The same walk releases every PHY holder through
__pse_control_release()->regulator_disable()->pse_pi_disable(), and that
clears admin_state_enabled.

Leaving a port powered on for good would need a pending PI with no
holder. Only an earlier failed ops->pi_disable() could produce that. Is
that case worth handling, or is the brief power-on acceptable here?

> +
> +	/* After the event, not before. A subscriber dropping the last
> +	 * pse_control reference reaches __pse_control_release() ->
> +	 * regulator_disable() -> _pse_pi_disable(), which can end up in
> +	 * pse_disable_pi_pol() and queue a notification of its own, so a
> +	 * cancel_work_sync() placed above the walk would not stay drained.
> +	 */
> +	cancel_work_sync(&pcdev->ntf_work);

[Severity: High]
At this point in the series, can ntf_work be queued again after this
drain?

Nothing here blocks pse_control holders that do not subscribe. A PHY
handle from fwnode_mdio can still take this path after
cancel_work_sync():

pse_ethtool_set_config() or pse_ethtool_set_prio()
  regulator op
    _pse_pi_disable()
      pse_disable_pi_pol()
        kfifo_in_spinlocked(&pcdev->ntf_fifo, &ntf, 1, &pcdev->ntf_fifo_lock);
        schedule_work(&pcdev->ntf_work);

The fifo and the work are freed just below. A requeued worker's
pse_control_put()->__pse_control_release() would also read the freed
pcdev->pi[psec->id].

The commit message mentions this ("The drain is not yet final on its
own"). The baseline freed pcdev->pi before cancel_work_sync(), so this
ordering is no worse.

The last patch in the series, "net: phy: own phydev->psec via PSE
notifier and remove fwnode_mdio hook", closes the window:

- The PSE_UNREGISTERED walk clears and puts every phydev->psec under
  pse_phy_lock().
- net/ethtool/pse-pd.c takes pse_phy_lock() around
  pse_get_pse_attributes() and the whole of ethnl_set_pse().
- A WARN_ON(!list_empty(&pcdev->pse_control_head)) follows the drain.

> +
> +	pse_flush_pw_ds(pcdev);

[Severity: High]
This isn't a bug introduced by this patch, but can a shared power domain
be freed here while another controller still uses it?

pse_register_pw_ds() allocates a new domain with
devm_pse_alloc_pw_d(pcdev->dev). That is a devm_kzalloc() on the device
of whichever controller (A) registered first. A second controller (B)
whose PI regulators share the same parent supply only takes a reference:

drivers/net/pse-pd/pse_core.c:pse_register_pw_ds() {
    ...
			if (regulator_is_equal(pw_d->supply, rdev->supply)) {
				present = true;
				pcdev->pi[i].pw_d = pw_d;
				break;
			}
		}
		if (present) {
			kref_get(&pw_d->refcnt);
			continue;
		}
    ...
}

When A unbinds, pse_flush_pw_ds() only drops A's own references, for
example from 2 to 1. __pse_pw_d_release() does not run, so the pw_d stays
in pse_pw_d_map and in B->pi[i].pw_d.

devm_pse_controller_register() calls devres_add() for the unregister
cookie only after pse_controller_register() returns. Devres therefore
frees the pw_d allocation right after pse_controller_unregister(A)
returns.

B and later controllers then use the freed pw_d:

- pse_pw_d_is_sw_pw_control() reads pw_d->budget_eval_strategy.
- pse_pi_allocate_pw_budget() and pse_pi_deallocate_pw_budget() use
  pw_d->supply.
- B's own pse_flush_pw_ds() does xa_load(&pse_pw_d_map,
  pcdev->pi[i].pw_d->id), then kref_put_mutex() on the freed kref.
- A third controller's pse_register_pw_ds() walks pse_pw_d_map and reads
  pw_d->supply.

The later patch "net: pse-pd: unwind allocations when controller
registration fails" says it does not fix shared power domains ("devres
frees the creator's pw_d underneath it anyway"). At the end of the series
pse_flush_pw_ds() only clears the departing controller's own pi[].pw_d.
B's pointer and the xarray entry still point at freed memory.

> +	pse_release_pis(pcdev);

[Severity: High]
This is a pre-existing issue, but at this commit, can PHY-held
pse_control handles reach the freed pcdev->pi and pcdev after unbind?

PHYs keep the handle they got from fwnode_mdio until phy_device_remove(),
and nothing subscribes to PSE_UNREGISTERED yet. Once this frees pcdev->pi
and devres frees pcdev, psec->pcdev points at freed memory.

Unprivileged users can reach this through the ETHTOOL_MSG_PSE_GET path.
pse_ethtool_get_status() dereferences pcdev->ops and pcdev->pi[psec->id].

The final pse_control_put() from phy_device_remove() also runs this on
freed memory:

drivers/net/pse-pd/pse_core.c:__pse_control_release() {
    ...
	if (psec->pcdev->pi[psec->id].admin_state_enabled)
		regulator_disable(psec->ps);
	regulator_put(psec->ps);

	module_put(psec->pcdev->owner);
    ...
}

The commit message says this stays open until phylib releases its
handles. The last patch in the series, "net: phy: own phydev->psec via
PSE notifier and remove fwnode_mdio hook", fixes it:

- fwnode_find_pse_control() is removed.
- During PSE_UNREGISTERED, phy_pse_detach_one() clears and puts each
  matching phydev->psec before cancel_work_sync() and the frees.
- pse_prepare_data(), ethnl_set_pse() and phy_device_remove() hold
  pse_phy_lock().

[ ... ]

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