Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

[Carlo Szelinsky]

On Mon, 05 Oct 2026 16:43:05 +0000, sashiko-bot@kernel.org wrote:
> [Severity: High]
> Does setting pcdev->pi[i].pw_d to NULL here without holding pcdev->lock
> introduce a race condition with concurrent sysfs accesses?

Yes, agreed. In v9 pi[].pw_d is cleared under pcdev->lock, so a
regulator op that holds the lock sees either the old domain or NULL,
never a change in between.

Carlo

[PATCH net-next v8 0/7] net: pse-pd: decouple controller lookup from MDIO probe

[Carlo Szelinsky]

This is v8 of the series Corey started as an RFC [3]; I took it over
from v2 [1]. It takes the PSE controller lookup out of the MDIO probe
path, so a modular PSE controller driver no longer makes the PHY (and any
DSA switch behind it) spin on -EPROBE_DEFER until the PSE module loads.

v7 [10] drew an LLM review [11]. Four of its findings are fixed here,
one I disagree with (patch 5 below), and the rest I answered in that
thread. v6 [7] drew one too [8]; v7 folded the patches it was mostly
about into the phy patch, so no commit carries the rtnl detour or the
deferred release. Per Documentation/process/maintainer-netdev.rst I also
ran LLM review locally on v7 and v8 before posting.

Patch 2 reorders pse_controller_unregister() around the new event,
because a subscriber runs arbitrary teardown inside it. The controller
is unlinked from pse_controller_list before the event and before
anything is freed, so a lookup racing the teardown resolves nothing
rather than a controller whose pcdev->pi[] pse_release_pis() is about to
free. v6 disclosed that as pre-existing and reachable only from phy
registration; it is reachable from more than that here, because after
the last patch a second controller registering runs a PSE_REGISTERED
walk that calls of_pse_control_get() for every phy on mdio_bus_type that
has a DT node and no handle yet, which happens on any board with two PSE
controllers. disable_irq() moves up for the same reason: pse_isr()
queues notifications and reaches pcdev->pi. On tps23881, the only
in-tree user of devm_pse_irq_helper(), devres has already run free_irq()
by then, so the move is what makes the ordering hold for a driver that
requests its irq earlier rather than leaving it to devres.

[9] makes a related reordering for net, independently of any subscriber,
so pse_controller_unregister() conflicts if [9] is applied. The order is
not identical: [9] leaves the unlink below cancel_work_sync() and
pse_flush_pw_ds(), which it can, having no event to place. Here the
event has to sit after the unlink and before the frees, and
cancel_work_sync() after the event, so the unlink moves up to just after
disable_irq(). The merged function wants this order, which already
includes [9]'s reordering:

    if (pcdev->irq)
            disable_irq(pcdev->irq);
    mutex_lock(&pse_list_mutex);
    list_del(&pcdev->list);
    mutex_unlock(&pse_list_mutex);
    blocking_notifier_call_chain(&pse_controller_notifier,
                                 PSE_UNREGISTERED, pcdev);
    cancel_work_sync(&pcdev->ntf_work);
    WARN_ON(!list_empty(&pcdev->pse_control_head));
    pse_flush_pw_ds(pcdev);
    pse_release_pis(pcdev);
    kfifo_free(&pcdev->ntf_fifo);

I am happy to send that as a follow-up on top of the merge if that is
easier than carrying it in the conflict.

Kory, a specific ask on patch 2. cancel_work_sync() sits below the event,
not above it, because __pse_control_release() can re-enter your budget
code. regulator_disable() on a PI that is still on runs
_pse_pi_disable(). With the static strategy that retries a pending port
on the same power domain, and if the domain is still over budget it sheds
a lower priority one through pse_disable_pi_pol() - which queues a
notification and calls schedule_work() from inside the walk. Draining
before the walk would leave that work racing kfifo_free(). Draining after
it also keeps the worker's own transient reference, taken by
pse_control_find_by_id(), from becoming the last one once
pse_release_pis() has freed the array.

I staged that case in QEMU (below) and confirmed it with a dump_stack()
from pse_disable_pi_pol(), so it is a path I have hit rather than only
reasoned about. The ordering rests on your design though, so please take
a look at it.

Patch 5: each PSE PI regulator is registered with a "vpwr" supply. The
regulator core deliberately treats an unresolved supply at registration as
non-fatal, so pse_controller_register() completes and PSE_REGISTERED fires
for a controller whose PIs cannot be handed out yet: regulator_get_exclusive()
in pse_control_get_internal() resolves the supply itself and keeps
returning -EPROBE_DEFER until the vpwr provider appears. Before this series
the MDIO layer propagated that and deferred probe retried it. After it,
phylib has no event left to retry on, and the port would silently lose PSE
for good.

Patch 5 checks every PI's supply before registering any PI regulator, so
the PSE driver's own probe defers and deferred probe handles the
ordering. It checks exactly the PIs the registration loop creates a
regulator for, including a controller with no pse-pis node, and it
follows both stages the core uses - the PI node, then the controller
device - because a vpwr-supply written once on the controller node is
invisible from the PI node but resolves at stage two.

The review of v7 found two things wrong with that. The PI-node lookup has
to be taken only once the PI's own phandle resolves, or
of_get_child_regulator() walks the whole controller subtree and answers
one PI from a sibling's supply before the controller node is ever
consulted; that is fixed. The core reaches the same sibling walk, but
only after the controller's own vpwr-supply has had its turn.

It also asked for the loop to move ahead of setup_pi_matrix(), because
pd692x0 claims a power budget there that nothing gives back when
registration fails later. I tried that and it is wrong: pd692x0's pse-pi
nodes name the manager regulators setup_pi_matrix() itself registers, so
checking earlier defers on a supply only that driver can bring up and it
never probes. I added a controller of that shape to the test setup and
confirmed it both ways. The loop stays where it is and the changelog says
what the placement does not clean up.

-EPERM is now treated as resolved, since it only means the provider is
held exclusively. The check still stops short of the core's
device_is_bound() gate, so a probe interleaving with the provider's own
can slip through, and nothing retries that PI until the next
PSE_REGISTERED. fw_devlink only prevents that when the vpwr-supply sits
on the controller node: one in a pse-pi node gives the controller just a
SYNC_STATE_ONLY proxy link, which does not hold its probe back.

Patch 3 exists because patch 5 makes -EPROBE_DEFER an ordinary return
from pse_controller_register(). That function has no unwind past
kfifo_alloc(): the kfifo leaks on every failure, and past
of_load_pse_pis(), which cleans up after itself, so do the PI array and
its OF references - once today and on each retry after patch 5 - and a
partial pse_register_pw_ds() leaves devm-allocated power domains in the
global xarray for the next registration to trip over.

Patch 3 adds the unwind, at two depths: pse_pi_ops index pcdev->pi[], and
the PI regulators are devm-registered, so once one exists the array
cannot be freed here at all and stays leaked as it is today. It is
released on the failures that happen while it exists and before the first
PI regulator does - setup_pi_matrix() and the supply check - which is
where the ordinary deferral now lands. It also clears pi[].pw_d, and says
what it does not cover: a power domain shared with another controller can
still be freed under it, which is pre-existing and wants the domain out
of devm.

Patch 4 is a small si3474 change for the same reason. si3474 logged
every controller registration failure with dev_err(), and its binding
puts vpwr-supply in the pse-pi nodes, which fw_devlink does not wait
for, so with patch 5 it would log an error on every deferred probe
retry until the supply appears. It now uses dev_err_probe(), as
pd692x0 and tps23881 already do.

The v6 review [8] caught two things in the phy patch, both fixed in v7.
The error paths of phy_device_register() could leak a handle:
device_add() puts the phy on the klist before its own later failure
points, so a PSE_REGISTERED walk can attach one that nothing releases. A
put at the out: label does not work, because by then device_add() has
unwound the phy off the bus and the PSE_UNREGISTERED walk would miss it
too. phydev->psec_detached now covers registration as well as removal,
so no handle is attached in that window. The write before device_add()
is the one place it is set without pse_phy_lock(), which is safe because
the phy is not on the klist yet and device_add()'s own locking orders
it.

That flag is a plain bool rather than another bit in the flags word,
since it is written under pse_phy_lock() while its neighbours are written
under phydev->lock and rtnl.

Patch 6 is a one-entry change in drivers/of/. fw_devlink treats "pses"
as a supplier binding, so a phy that references a PI has a device link to
the PSE controller and its driver probe waits for it. That was invisible
while the PSE lookup deferred the phy anyway, since fwnode_mdio removed
it again on every retry. Once patch 7 stops
deferring, the phy is registered while its own driver is still blocked,
and a MAC attaching in that window takes the generic driver through
phy_attach_direct() - device_bind_driver() then forces the bind past the
pending link, and nothing re-probes it later.

Marking the link FWLINK_FLAG_IGNORE drops it entirely: no device link, no
ordering, no cycle detection, and the fwnode link goes at the consumer's
device_add(). That is what post-init-providers already does.

Two things do ride on that link today, because it is managed - the fwnode
link sits on the phy's own node, so fw_devlink_create_devlink() asks for
fw_devlink_flags, which defaults to FW_DEVLINK_FLAGS_RPM and carries
neither DL_FLAG_STATELESS nor DL_FLAG_SYNC_STATE_ONLY, so
device_link_add() promotes it to DL_FLAG_MANAGED. So unbinding the PSE
controller releases the phy's driver, and DL_FLAG_AUTOPROBE_CONSUMER
probes the phy when the controller binds. Both are given up on purpose:
the notifier does the attach and detach now, without tearing the port's
phy driver down, and there is no deferral left for an autoprobe to wait
on. DL_FLAG_PM_RUNTIME is the one that really had no effect, since no PSE
driver implements PM ops, sync_state or runtime PM.

That also means patch 6 is not a no-op on its own. The MDIO path
registers the phy before it looks the PI up, so the link exists and goes
active as soon as the controller binds, which puts the cascade's removal
at patch 6 rather than at patch 7. It also stops holding the phy's driver
back during those retries, so at patch 6 alone the driver probes and is
removed again on each one until the controller binds; patch 7 removes
the retries. Its changelog says both.

Rob, Saravana: this one is yours, and it is why the phy patch is safe to
take.

No Fixes: tag. 5e82147de1cb ("net: mdiobus: search for PSE nodes by
parsing PHY nodes.") is the commit to blame, but this is a refactor
across net, phylib and drivers/of plus six new exports, and tagging it
would invite a stable backport of all that to cure a probe-retry loop.

How it works: pse_core gets a notifier chain (REGISTERED /
UNREGISTERED). phylib subscribes, owns phydev->psec, and attaches the
handle when the controller shows up instead of during probe. fwnode_mdio
loses its PSE awareness, so no PSE-originated -EPROBE_DEFER leaves it.
Patch 6 removes the other half, the fw_devlink link that deferred the
phy's driver. With both gone there is no probe-retry loop left.

On the tags: Jonas tested the v4 shape and Aleksander tested the v6
locking, which is unchanged here. Neither tested the fixes above. On the
folded phy patch the code they exercised is intact, so I have kept their
tags there. Jonas's tag also rides on patch 2, and that one did change
in v7 - pse_controller_unregister() is reordered around the event - so
it is the weakest of the tags I kept. Patch 1 only gained a kernel-doc
correction. Happy to drop any of them if either would prefer.

Tested on a Realtek rtl9303 PoE switch with an HS104 PSE controller on
i2c, with a PD drawing power on one port. That board carries the notifier
mechanism from patches 1, 2 and 7; the failure paths of patches 3 and 5
are exercised in QEMU only, below, patch 6 only by booting with it
applied, and patch 4 is build-tested only.

 - clean boot, no probe-retry loop, the controller registers once
 - rmmod is refused while a phy holds a handle
 - i2c unbind: the notifier walk drops the handle and the port powers
   down, and ethtool reports no PSE attached
 - i2c bind: the handle comes back and the PD is powered again
 - six unbind/bind cycles, power domain index stable

Also exercised under QEMU, on arm64 under KASAN, PROVE_LOCKING and
kmemleak. The device tree has five PSE controllers:

 - a working one, plus an MDIO bus with two phys, only one of which
   references a PI. Unbinding the controller detaches that phy's handle
   and rebinding re-attaches it; the other phy is never touched;
   unbinding the MDIO bus releases a live handle.
 - one whose vpwr provider never appears - must defer.
 - one with its vpwr-supply on the controller node instead of the PI
   nodes, which the core resolves one stage later - must defer.
 - one whose controller node carries a vpwr-supply that never appears,
   with the first PI naming its own working supply and the next none, so
   a check that lets of_get_child_regulator() answer the second from the
   first misses the controller's supply - must defer.
 - one shaped like pd692x0, whose PI names a regulator the driver only
   registers from setup_pi_matrix() - must register, not defer.

I confirmed the last three both ways, with and without the fix each is
there for. The WARN_ON in patch 7 stays silent across six unbind cycles
and kmemleak reports nothing.

The same setup stages an over-budget static-priority domain for the
patch 2 case above: releasing a handle in the walk sheds a lower
priority port through pse_disable_pi_pol(). No lockdep splat on that
path, which re-enters the regulator core from a notifier callback under
the chain's rwsem and pse_phy_mutex.

Build matrix, all linking a real vmlinux: PHYLIB=y, PHYLIB=m (the config
that failed to link in v5), PHYLIB=n, PSE_CONTROLLER=n, and CONFIG_OF=n.

Changes in v8:
 - Rewrite the changelogs: drop references to other patches by position
   and to the pending net series (its conflict note is now below the
   --- of patch 2), reflow, and cut review history that belongs here.
 - Patch 2: add Co-developed-by, since the reordering is mine.
 - New patch 4: switch si3474 to dev_err_probe() for the controller
   registration error, so the supply check's -EPROBE_DEFER is not
   logged as an error on every retry.
 - Patch 7: say that any phy registered with a DT node now gets its PI,
   including ones found by mdiobus_scan(), and that a persistent lookup
   error is reported again on each controller registration.
 - Patch 3: clear pi[].pw_d in pse_flush_pw_ds(). The domain is devm
   memory of whoever created it, and pse_pi_is_enabled() still reaches
   that pointer from the regulator "state" attribute.
 - Patch 5: take the PI-node supply lookup only once that PI's own
   vpwr-supply phandle resolves. of_regulator_get_optional() falls back
   to of_get_child_regulator() on the device node, so a PI naming no
   supply of its own was answered from a sibling PI's before the
   controller node was consulted, masking an unresolved vpwr-supply
   there.
 - Patch 5: treat -EPERM from the supply get as resolved. It only means
   another consumer holds the provider exclusively, which the core's own
   resolution never asks about.
 - Patch 5: say why the checks cannot move ahead of setup_pi_matrix(),
   which the review asked for - a pd692x0 PI names a regulator that
   function registers itself.
 - New patch 6: stop "pses" gating probe in fw_devlink, so the phy is not
   left bound to the generic driver once patch 7 removes the deferral.
   Its changelog spells out that the link is managed, so dropping it also
   drops the supplier-unbind cascade and DL_FLAG_AUTOPROBE_CONSUMER, that
   this happens at patch 6 rather than at patch 7, and that on its own it
   lets the phy driver probe and unbind on every MDIO retry.
 - Patch 5: ask the controller-device stage of the supply lookup once for
   the whole controller instead of once per PI.
 - Patch 3: drop the flush_pw_ds label, which had to jump over
   release_pis, in favour of flushing inline at the one failure that
   needs it.
 - Document what is not fixed: the worker drain is only final once the
   phy patch lands, devres already quiesces the irq for tps23881, the OF
   references leak with the PI array on the three paths that keep it, a
   PSE probe failing after registration powers the PI down, and a shared
   power domain can still be freed under a second controller. The last
   is pre-existing and wants the domain out of devm, so not fixed here.
 - Fix changelog claims the review caught: pse_release_pis() does run
   from of_load_pse_pis() too, and nothing retries a PI that resolves
   after the pre-check (v7 said it could "resolve late").
 - Fix the phy_try_attach_pse() comment: other errors are not
   necessarily a broken binding, and -EPROBE_DEFER from a registered
   controller whose vpwr provider is not yet bound is not retried.
 - Patch 3: the shared power domain problem is not a refcount race, as
   my reply to the v7 review put it - pse_register_pw_ds() takes its
   reference under pse_pw_d_mutex and kref_put_mutex() takes that mutex
   for the final put. It is only that the domain is devm memory of the
   controller that created it. Changelog corrected.
 - Correct changelog and comment claims found in a final audit: patch 5's
   comments no longer say nothing retries the consumer at that commit
   (fwnode_mdio still does) or describe a CONFIG_OF=n path that cannot
   be reached; patch 7 no longer says a hard lookup error used to be
   retried by deferred probe - it failed the whole MDIO bus registration.

Changes in v7:
 - Patch 2: reorder pse_controller_unregister() around the event - unlink
   and disable_irq() before it, cancel_work_sync() after it, the frees
   last. The pse_control_head WARN_ON goes with the phy patch instead
   (patch 7 here), with the walk that empties the list - in patch 2 an
   unbind would trip it, since the fwnode_mdio hook still hands out
   handles nothing releases.
 - New patch 3: unwind the kfifo, the PI array and the power domains when
   controller registration fails.
 - New patch 4: check every PI vpwr supply before registering the
   controller, so a consumer never meets a registered controller that can
   only answer -EPROBE_DEFER.
 - Fold old patches 4 and 5 into the phy patch, so no intermediate commit
   carries the rtnl recursion or the deferred release.
 - Hold phydev->psec_detached across registration too, make it a bool
   rather than a bitfield, and drop the unsafe release from
   phy_device_register()'s error path.
 - Drop netsec from the deadlock list; fix the module-unload rationale;
   document that a transient attach error is no longer retried by deferred
   probe.
 - Include <linux/notifier.h> in phy_device.c.
 - Rebased on net-next.

Changes in v6:
 - Fix a v5 build regression: the mutex moved into pse_core, since
   net/ethtool is always in vmlinux while PHYLIB is tristate.
 - Fold phy_device_register_locked() back into phy_device_register().

Changes in v5 (since v4 [4]):
 - Replace rtnl with a dedicated mutex in the PSE attach path; with rtnl
   it deadlocked lantiq_etop [5].
 - Put phydev->psec back in phy_device_remove(), closing the
   use-after-free Paolo forwarded [6].

Changes in v4:
 - Add Tested-by from Jonas Jelonek. No code changes.

Changes in v3:
 - Drop patch 1 (regulator handle fix); it went to net separately [2].

v1 was an RFC by Corey [3].

[1] https://lore.kernel.org/netdev/20260620112440.1734404-1-github@szelinsky.de/
[2] https://lore.kernel.org/netdev/20260624204017.2752934-1-github@szelinsky.de/
[3] https://lore.kernel.org/netdev/20260423-pse-notifier-decouple-v1-0-86ed750a9d62@leavitt.info/
[4] https://lore.kernel.org/netdev/20260630091125.3162481-1-github@szelinsky.de/
[5] https://lore.kernel.org/netdev/bac5e6e9-7358-4ccb-87fc-9c40baa33682@wp.pl/
[6] https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@redhat.com/
[7] https://lore.kernel.org/netdev/20260906153102.959217-1-github@szelinsky.de/
[8] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
[9] https://lore.kernel.org/netdev/20260813200653.980170-1-github@szelinsky.de/
[10] https://lore.kernel.org/netdev/20260927191850.1370515-1-github@szelinsky.de/
[11] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927191850.1370515-1-github%40szelinsky.de

Carlo Szelinsky (4):
  net: pse-pd: unwind allocations when controller registration fails
  net: pse-pd: si3474: use dev_err_probe() for controller registration
  net: pse-pd: check the PI vpwr supply before registering the
    controller
  of: property: do not let "pses" block a consumer's probe

Corey Leavitt (3):
  net: pse-pd: add notifier chain for controller lifecycle events
  net: pse-pd: fire lifecycle events on controller register/unregister
  net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio
    hook

 drivers/net/mdio/fwnode_mdio.c |  34 ----
 drivers/net/phy/phy_device.c   | 144 ++++++++++++++-
 drivers/net/pse-pd/pse_core.c  | 321 +++++++++++++++++++++++++++++++--
 drivers/net/pse-pd/si3474.c    |   7 +-
 drivers/of/property.c          |   5 +-
 include/linux/phy.h            |   7 +
 include/linux/pse-pd/pse.h     |  65 +++++++
 net/ethtool/pse-pd.c           |  16 +-
 8 files changed, 540 insertions(+), 59 deletions(-)


base-commit: e3bfd25626b44b6fa61a13c17178922171d519ce
-- 
2.43.0

[PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

[Carlo Szelinsky]

pse_controller_register() allocates the notification kfifo and, through
of_load_pse_pis(), the PI array plus OF references on each described PI
node and its pairset nodes. Every failure after that point simply
returns and none of it is freed. of_load_pse_pis() cleans up after
itself, but past that point kfifo_free() and pse_release_pis() only run
from pse_controller_unregister(), which a failed registration never
reaches, and devm_pse_controller_register() drops only its own devres
cookie. pcdev->pi is a plain allocation, so nothing else will ever free
it.

A partial pse_register_pw_ds() is worse than a leak. The power domains
it already created are devm-allocated but live in the global
pse_pw_d_map, so the failed probe frees them while that xarray still
points at them, and the next controller to register walks into freed
memory in pse_register_pw_ds(), reading pw_d->supply for
regulator_is_equal().

Unwind at two depths, because the PI array cannot always be freed here.
pse_pi_ops.is_enabled(), .enable() and .disable() all index pcdev->pi[],
and the PI regulators are devm-registered on pcdev->dev, so from the
first successful devm_pse_pi_regulator_register() until devres unwinds
the failed probe there are live regulators whose ops would follow a
freed pointer; regulator_late_cleanup() and the "state" class attribute
both reach them. So:

 - failures before of_load_pse_pis() succeeds free only the kfifo;
 - failures after it and before the PI regulator loop (today
   setup_pi_matrix()) also release the PI array and its OF references;
 - failures from the loop onwards free the kfifo, and the
   pse_register_pw_ds() one also flushes the power domains, but leave
   the PI array and its OF references leaked exactly as today rather
   than hand live regulators a dangling pointer.

A driver's setup_pi_matrix() may have registered devm regulators of its
own by the time it fails - pd692x0 registers its managers there - but
those do not index pcdev->pi[]. Freeing the array safely past the loop
would need the regulator ops to tolerate a NULL pcdev->pi, which they do
not today.

pcdev->pi is cleared at the release_pis label and deliberately not
inside pse_release_pis(): pse_controller_unregister() calls that helper
with every PI regulator still registered, and pse_pi_is_enabled()
indexes pcdev->pi[] unguarded behind the "state" attribute, so clearing
it there would turn that pre-existing read of freed memory into a NULL
dereference.

pse_flush_pw_ds() now also clears pi[].pw_d. The domain is devm memory
of whichever controller created it, so it can go as soon as that probe
unwinds, and pse_pi_is_enabled() still reaches pi[].pw_d from the
"state" attribute until the PI regulators are unregistered.

This does not fix a shared power domain. The reference counting is
sound - pse_register_pw_ds() takes its kref_get() under pse_pw_d_mutex
and kref_put_mutex() takes that mutex for the final put - but if
another controller on the same supply holds a reference, the flush only
goes 2->1 and devres frees the creator's pw_d underneath it anyway.
Unregistering a controller whose domain is shared has the same problem
today. Fixing it means moving the domain out of devm memory so that
only the last put frees it.

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 56 ++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index dc261beb6170..eeefbf25e671 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -937,11 +937,21 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
 			continue;
 
 		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
-		if (!pw_d)
+		if (!pw_d) {
+			pcdev->pi[i].pw_d = NULL;
 			continue;
+		}
 
 		kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release,
 			       &pse_pw_d_mutex);
+		/* The pw_d is devm memory of whichever controller created
+		 * it, so it can go away as soon as that probe unwinds.
+		 * Nothing may be left pointing at it: pse_pi_is_enabled()
+		 * reaches pi[].pw_d from the regulator "state" attribute,
+		 * which stays readable until the PI regulators are
+		 * unregistered after us.
+		 */
+		pcdev->pi[i].pw_d = NULL;
 	}
 }
 
@@ -1092,17 +1102,18 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	    !pcdev->ops->pi_get_pw_status) {
 		dev_err(pcdev->dev,
 			"Mandatory status report callbacks are missing");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto free_kfifo;
 	}
 
 	ret = of_load_pse_pis(pcdev);
 	if (ret)
-		return ret;
+		goto free_kfifo;
 
 	if (pcdev->ops->setup_pi_matrix) {
 		ret = pcdev->ops->setup_pi_matrix(pcdev);
 		if (ret)
-			return ret;
+			goto release_pis;
 	}
 
 	/* Each regulator name len is pcdev dev name + 7 char +
@@ -1110,7 +1121,12 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	 */
 	reg_name_len = strlen(dev_name(pcdev->dev)) + 18;
 
-	/* Register PI regulators */
+	/* Register PI regulators. Once one of these exists, pse_pi_ops index
+	 * pcdev->pi[] and nothing here can unregister it again, so the array
+	 * must outlive this function. Failures below therefore unwind to
+	 * free_kfifo and deliberately leak it, as they already do today,
+	 * rather than hand the live regulators a freed pointer.
+	 */
 	for (i = 0; i < pcdev->nr_lines; i++) {
 		char *reg_name;
 
@@ -1119,20 +1135,27 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 			continue;
 
 		reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL);
-		if (!reg_name)
-			return -ENOMEM;
+		if (!reg_name) {
+			ret = -ENOMEM;
+			goto free_kfifo;
+		}
 
 		snprintf(reg_name, reg_name_len, "pse-%s_pi%d",
 			 dev_name(pcdev->dev), i);
 
 		ret = devm_pse_pi_regulator_register(pcdev, reg_name, i);
 		if (ret)
-			return ret;
+			goto free_kfifo;
 	}
 
 	ret = pse_register_pw_ds(pcdev);
-	if (ret)
-		return ret;
+	if (ret) {
+		/* Deliberately not release_pis: the PI regulators registered
+		 * above index pcdev->pi[] and outlive this function.
+		 */
+		pse_flush_pw_ds(pcdev);
+		goto free_kfifo;
+	}
 
 	mutex_lock(&pse_list_mutex);
 	list_add(&pcdev->list, &pse_controller_list);
@@ -1142,6 +1165,19 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 				     PSE_REGISTERED, pcdev);
 
 	return 0;
+
+	/* Only for failures above the PI regulator loop, where no regulator
+	 * indexes pcdev->pi[] yet. Anything below it has to go straight to
+	 * free_kfifo instead.
+	 */
+release_pis:
+	pse_release_pis(pcdev);
+	pcdev->pi = NULL;
+
+free_kfifo:
+	kfifo_free(&pcdev->ntf_fifo);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
 
-- 
2.43.0

Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 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] The use-after-free the patch says it closes is still reachable
  when two controllers register concurrently.
- [Medium] The subject says the patch unwinds allocations when
  registration fails.
- [Medium] pse_flush_pw_ds() now stores NULL to pcdev->pi[i].pw_d (lines
  941 and 954) so that nothing is left pointing at the devm-allocated…
- [Low] The patch fixes a real memory-safety bug but carries no Fixes:
  tag and no stable Cc.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the podl-pse-
  regulator driver can never register a PSE controller.

--

net: pse-pd: unwind allocations when controller registration fails

This patch adds error unwinding to pse_controller_register(). The
notification kfifo is now freed on failure, and early failures also free
the PI array and its OF references. Power domains left by a partial
pse_register_pw_ds() are flushed, pse_flush_pw_ds() now clears pi[].pw_d,
and failures from the PI regulator loop onward deliberately leave the PI
array allocated.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index dc261beb61702..eeefbf25e6716 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -937,11 +937,21 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)
>  			continue;
>  
>  		pw_d = xa_load(&pse_pw_d_map, pcdev->pi[i].pw_d->id);
> -		if (!pw_d)
> +		if (!pw_d) {
> +			pcdev->pi[i].pw_d = NULL;
>  			continue;
> +		}
>  
>  		kref_put_mutex(&pw_d->refcnt, __pse_pw_d_release,
>  			       &pse_pw_d_mutex);
> +		/* The pw_d is devm memory of whichever controller created
> +		 * it, so it can go away as soon as that probe unwinds.
> +		 * Nothing may be left pointing at it: pse_pi_is_enabled()
> +		 * reaches pi[].pw_d from the regulator "state" attribute,
> +		 * which stays readable until the PI regulators are
> +		 * unregistered after us.
> +		 */
> +		pcdev->pi[i].pw_d = NULL;

[Severity: Medium]
Is this store enough to keep pse_pi_is_enabled() away from a freed pw_d?
The store is done without pcdev->lock, but the reader holds that lock:

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

On the new pse_register_pw_ds() failure path the PI regulators are still
live. That means the world-readable state attribute can reach this at any
time:

state_show()->_regulator_is_enabled()->pse_pi_is_enabled()

Say a reader has loaded pcdev->pi[id].pw_d and is preempted before
pse_pw_d_is_sw_pw_control() reads pw_d->budget_eval_strategy. In that gap,
pse_flush_pw_ds() can clear the field and return, and the probe fails.
devres then frees the pw_d before it unregisters the PI regulators,
because the pw_d was allocated after them. The reader then reads freed
memory.

Would it close this window to take pcdev->lock around the pointer clear
and do the kref_put_mutex() calls afterwards, outside pcdev->lock?

There is a second race in the intermediate state of the series.
pse_ethtool_get_status() checks pi->pw_d, calls the sleeping
pi_get_admin_state(), and then does:

	switch (pi->pw_d->budget_eval_strategy) {

If that races with pse_controller_unregister(), the new unlocked NULL
store turns this into a NULL dereference. Later in the series, "net: phy:
own phydev->psec via PSE notifier and remove fwnode_mdio hook" closes
that part: the ethtool PSE paths take pse_phy_lock(), and psec is
detached before the flush. The pse_pi_is_enabled() window is still there
at the end of the series.

[ ... ]

> @@ -1092,17 +1102,18 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	    !pcdev->ops->pi_get_pw_status) {
>  		dev_err(pcdev->dev,
>  			"Mandatory status report callbacks are missing");
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto free_kfifo;
>  	}

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
this patch reworks the kfifo handling here. Can the kfifo ever be
allocated for podl-pse-regulator? Earlier in pse_controller_register():

	ret = kfifo_alloc(&pcdev->ntf_fifo, pcdev->nr_lines, GFP_KERNEL);
	if (ret) {
		dev_err(pcdev->dev, "failed to allocate kfifo notifications\n");
		return ret;
	}
	INIT_WORK(&pcdev->ntf_work, pse_send_ntf_worker);

	if (!pcdev->nr_lines)
		pcdev->nr_lines = 1;

The kfifo size comes from nr_lines before the default is applied.
__kfifo_alloc_node() rounds the size up to a power of two and returns
-EINVAL if the result is below 2, and both 0 and 1 end up there.

pse_reg_probe() in drivers/net/pse-pd/pse_regulator.c never sets
nr_lines before it calls devm_pse_controller_register(). That probe
therefore seems to always fail with "failed to allocate kfifo
notifications". A controller with nr_lines == 1 fails the same way.

Would it be better to size the kfifo from the defaulted nr_lines with a
minimum of 2, for example max(nr_lines, 2)? This came in with
ffef61d6d273 ("net: pse-pd: Add support for budget evaluation
strategies"), and it is still the same at the end of the series.

[ ... ]

> @@ -1110,7 +1121,12 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	 */
>  	reg_name_len = strlen(dev_name(pcdev->dev)) + 18;
>  
> -	/* Register PI regulators */
> +	/* Register PI regulators. Once one of these exists, pse_pi_ops index
> +	 * pcdev->pi[] and nothing here can unregister it again, so the array
> +	 * must outlive this function. Failures below therefore unwind to
> +	 * free_kfifo and deliberately leak it, as they already do today,
> +	 * rather than hand the live regulators a freed pointer.
> +	 */
>  	for (i = 0; i < pcdev->nr_lines; i++) {
>  		char *reg_name;
>  
> @@ -1119,20 +1135,27 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  			continue;
>  
>  		reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL);
> -		if (!reg_name)
> -			return -ENOMEM;
> +		if (!reg_name) {
> +			ret = -ENOMEM;
> +			goto free_kfifo;
> +		}
>  
>  		snprintf(reg_name, reg_name_len, "pse-%s_pi%d",
>  			 dev_name(pcdev->dev), i);
>  
>  		ret = devm_pse_pi_regulator_register(pcdev, reg_name, i);
>  		if (ret)
> -			return ret;
> +			goto free_kfifo;
>  	}

[Severity: Medium]
Doesn't this still leak pcdev->pi and the OF references on the PI and
pairset nodes? That happens on every failure from here on: the reg_name
allocation, this one, and the pse_register_pw_ds() failure below. It even
happens on the first loop iteration, before any PI regulator exists.

On registration failure, devm_pse_controller_register() frees only its
devres cookie, so nothing else releases the array.

The commit message says:

    Freeing the array safely past the loop
    would need the regulator ops to tolerate a NULL pcdev->pi, which they do
    not today.

Is that really the only way to do it? devm_pse_pi_regulator_register()
registers the PI regulators on pcdev->dev with devm_regulator_register().
A devm action on pcdev->dev that releases the PIs could be added right
after of_load_pse_pis() succeeds. Devres releases in LIFO order, so that
action would run after every PI regulator has been unregistered. No
regulator op would ever see a freed or NULL pcdev->pi.

That would also cover pse_controller_unregister(), which the commit
message notes calls pse_release_pis() while the PI regulators are still
registered.

The later patch "net: pse-pd: check the PI vpwr supply before registering
the controller" moves the common -EPROBE_DEFER case ahead of the loop.
The leaks from the loop and from pse_register_pw_ds() are still there at
the end of the series.

>  
>  	ret = pse_register_pw_ds(pcdev);
> -	if (ret)
> -		return ret;
> +	if (ret) {
> +		/* Deliberately not release_pis: the PI regulators registered
> +		 * above index pcdev->pi[] and outlive this function.
> +		 */
> +		pse_flush_pw_ds(pcdev);
> +		goto free_kfifo;
> +	}

[Severity: High]
Can a controller probing at the same time still hit the use-after-free
the commit message describes? On its error path, pse_register_pw_ds()
drops pse_pw_d_mutex while the domains it just created are still in
pse_pw_d_map:

out:
	mutex_unlock(&pse_pw_d_mutex);
	return ret;

pse_flush_pw_ds() runs only after that, without the mutex. Here is one
possible sequence, where controller B shares the same vpwr supply:

controller A
pse_register_pw_ds()
  devm_pse_alloc_pw_d()       /* pw_d published in pse_pw_d_map */
  regulator_get() fails on a later PI
  mutex_unlock(&pse_pw_d_mutex)

controller B
pse_register_pw_ds()
  regulator_is_equal(pw_d->supply, rdev->supply) matches
  pcdev->pi[i].pw_d = pw_d
  kref_get(&pw_d->refcnt)     /* 1 -> 2 */

controller A
pse_flush_pw_ds()
  kref_put_mutex()            /* 2 -> 1, no xa_erase() */
devres frees pw_d

Now pse_pw_d_map and B's pi[].pw_d both point at freed memory. The next
pse_register_pw_ds() reads pw_d->supply, and B's own flush later does
kref_put on the freed object.

Every domain the failing call created stays private to it until
pse_pw_d_mutex is dropped. Could pse_register_pw_ds() undo those domains
itself before the out: label, doing kref_put/xa_erase/regulator_put while
it still holds pse_pw_d_mutex? That would close this window without
moving pw_d out of devm memory.

[Severity: Low]
Should this patch carry Fixes: tags and a stable Cc? A partial
pse_register_pw_ds() leaves freed power domains in pse_pw_d_map, so this
fixes a real use-after-free. The commit message has only a Signed-off-by.

According to blame, the bare returns this patch replaces came from these
commits:

- after pse_register_pw_ds(): 50f8b341d268 ("net: pse-pd: Add support
  for PSE power domains")
- kfifo_alloc(): ffef61d6d273 ("net: pse-pd: Add support for budget
  evaluation strategies")
- after of_load_pse_pis(): 9be9567a7c59 ("net: pse-pd: Add support for
  PSE PIs")
- after setup_pi_matrix(): 29e28d1d7a16 ("net: pse-pd: Add support for
  setup_pi_matrix callback")

Could this also go to net on its own, rather than sit inside the
notifier chain / phylib ownership series?

[ ... ]

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

Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

[Carlo Szelinsky]

On Mon, 05 Oct 2026 17:33:27 +0000, netdev-bot+sashiko@kernel.org wrote:
> [Severity: Medium]
> Is this store enough to keep pse_pi_is_enabled() away from a freed pw_d?
> The store is done without pcdev->lock, but the reader holds that lock:
...
> Would it close this window to take pcdev->lock around the pointer clear
> and do the kref_put_mutex() calls afterwards, outside pcdev->lock?

Yes, agreed. In v9 pi[].pw_d is cleared under pcdev->lock, and the
reference is dropped afterwards, outside that lock.

> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch, but
> this patch reworks the kfifo handling here. Can the kfifo ever be
> allocated for podl-pse-regulator?

No, it can't, so podl-pse-regulator never registers today. That is an
older bug; its fix goes to net.

> [Severity: Medium]
> Doesn't this still leak pcdev->pi and the OF references on the PI and
> pairset nodes?
...
> A devm action on pcdev->dev that releases the PIs could be added right
> after of_load_pse_pis() succeeds.

Yes, the PI array is still leaked once the PI regulators exist, and the
commit message says so. The devm action is a good idea, but it also
changes how unregister frees the array, so I'd rather do it as a
separate follow-up than in this series.

> [Severity: High]
> Can a controller probing at the same time still hit the use-after-free
> the commit message describes?
...
> Could pse_register_pw_ds() undo those domains
> itself before the out: label, doing kref_put/xa_erase/regulator_put while
> it still holds pse_pw_d_mutex?

Yes, agreed. v9 does exactly that: pse_register_pw_ds() undoes its own
domains while it still holds pse_pw_d_mutex.

> [Severity: Low]
> Should this patch carry Fixes: tags and a stable Cc?
...
> Could this also go to net on its own, rather than sit inside the
> notifier chain / phylib ownership series?

v9 moves this patch out of the notifier series, into the separate PSE
core fixes series. That series goes to net-next, because the notifier
series builds on it. The older bugs it touches that also hurt on their
own, the kfifo size and the shared power domain, go to net with Fixes:
tags.

Carlo

Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails

[Kory Maincent]

> pse_controller_register() allocates the notification kfifo and, through
> of_load_pse_pis(), the PI array plus OF references on each described PI
> node and its pairset nodes. Every failure after that point simply
> returns and none of it is freed. of_load_pse_pis() cleans up after
> itself, but past that point kfifo_free() and pse_release_pis() only run
> from pse_controller_unregister(), which a failed registration never
> reaches, and devm_pse_controller_register() drops only its own devres
> cookie. pcdev->pi is a plain allocation, so nothing else will ever free
> it.
> 
> A partial pse_register_pw_ds() is worse than a leak. The power domains
> it already created are devm-allocated but live in the global
> pse_pw_d_map, so the failed probe frees them while that xarray still
> points at them, and the next controller to register walks into freed
> memory in pse_register_pw_ds(), reading pw_d->supply for
> regulator_is_equal().
> 
> Unwind at two depths, because the PI array cannot always be freed here.
> pse_pi_ops.is_enabled(), .enable() and .disable() all index pcdev->pi[],
> and the PI regulators are devm-registered on pcdev->dev, so from the
> first successful devm_pse_pi_regulator_register() until devres unwinds
> the failed probe there are live regulators whose ops would follow a
> freed pointer; regulator_late_cleanup() and the "state" class attribute
> both reach them. So:
> 
>  - failures before of_load_pse_pis() succeeds free only the kfifo;
>  - failures after it and before the PI regulator loop (today
>    setup_pi_matrix()) also release the PI array and its OF references;
>  - failures from the loop onwards free the kfifo, and the
>    pse_register_pw_ds() one also flushes the power domains, but leave
>    the PI array and its OF references leaked exactly as today rather
>    than hand live regulators a dangling pointer.
> 
> A driver's setup_pi_matrix() may have registered devm regulators of its
> own by the time it fails - pd692x0 registers its managers there - but
> those do not index pcdev->pi[]. Freeing the array safely past the loop
> would need the regulator ops to tolerate a NULL pcdev->pi, which they do
> not today.
> 
> pcdev->pi is cleared at the release_pis label and deliberately not
> inside pse_release_pis(): pse_controller_unregister() calls that helper
> with every PI regulator still registered, and pse_pi_is_enabled()
> indexes pcdev->pi[] unguarded behind the "state" attribute, so clearing
> it there would turn that pre-existing read of freed memory into a NULL
> dereference.
> 
> pse_flush_pw_ds() now also clears pi[].pw_d. The domain is devm memory
> of whichever controller created it, so it can go as soon as that probe
> unwinds, and pse_pi_is_enabled() still reaches pi[].pw_d from the
> "state" attribute until the PI regulators are unregistered.
> 
> This does not fix a shared power domain. The reference counting is
> sound - pse_register_pw_ds() takes its kref_get() under pse_pw_d_mutex
> and kref_put_mutex() takes that mutex for the final put - but if
> another controller on the same supply holds a reference, the flush only
> goes 2->1 and devres frees the creator's pw_d underneath it anyway.
> Unregistering a controller whose domain is shared has the same problem
> today. Fixing it means moving the domain out of devm memory so that
> only the last put frees it.

Same, please write concise commit message and comments.
I have just saw that you have replied in the cover letter about a v9.
I will wait for the v9 to review the series.

-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com