Skip to contentSkip to conversation

net: pse-pd: decouple controller lookup from MDIO probe

PATCH Superseded Carlo Szelinsky 3 participants 13 messages Sep 6 – Sep 27

Subsystems (6)

v6 of 8 Superseded Latest is v8 →

lore
Share & tools
lore
#0 lore raw

v6 cover net: pse-pd: decouple controller lookup from MDIO probe

This is v6 of Corey's series [1]. It takes the PSE controller lookup out of the MDIO probe path, so a modular PSE driver no longer makes the PHY/DSA probe spin on -EPROBE_DEFER until the PSE module loads.

Patches 1-3 are the same three notifier patches as v4 [4], unchanged, with Jonas's Tested-by. Patches 4 and 5 fix two problems the v4 review surfaced; v6 additionally fixes a build regression in v5 [7]'s patch 4.

Patch 4: Aleksander reported [5] that v4 deadlocks on probe for an MDIO bus registered from ndo_init (lantiq_etop, sni_ave, netsec): those already hold rtnl via register_netdevice(), and v4's phy attach took rtnl again underneath. Patch 4 swaps that rtnl for a dedicated mutex, so the register path no longer recurses. The ethtool PSE paths take the same mutex, so the use-after-free that rtnl used to close stays closed. The mutex lives in pse_core rather than phylib: net/ethtool is always built into vmlinux but PHYLIB is tristate, so with CONFIG_PHYLIB=m or =n a phylib export is unresolved (v5 failed to link there [8]); PSE_CONTROLLER is bool, so pse_core is always reachable.

Patch 5: Paolo's review [6] pointed out that patch 3 defers the pse_control_put() to phy_device_release(). A phy that is device_del()'d but still pinned (an attached netdev) is off the mdio_bus_type klist, so the PSE_UNREGISTERED notifier walk never clears its phydev->psec, and the deferred put later touches a pcdev->pi[] the controller has already freed. Patch 5 puts phydev->psec back in phy_device_remove(), which the mutex from patch 4 now makes safe (the rtnl recursion that motivated the deferral is gone), so the detach is synchronous and cannot outlive the controller.

How it works: pse_core gets a notifier chain (REGISTERED / UNREGISTERED). The phy layer subscribes, owns phydev->psec, and attaches the PSE handle when the controller shows up instead of during probe. fwnode_mdio loses its PSE awareness, so no -EPROBE_DEFER leaves it and the probe-retry loop is gone.

Tested on a Realtek rtl93xx PoE switch with two HS104 PSE controllers on i2c:

- clean boot, no probe-retry loop, no watchdog reset
- 10G SFP+ port: module hotplug works, no deadlock
- ethtool --set-pse enable/disable cuts and restores power to a PD
- i2c unbind -> rmmod -> modprobe: PSE detaches on unbind and re-attaches on reload with power restored, no reboot. No lockdep splats.

Jonas confirmed the RTL8214FC deadlock he reported is gone. Aleksander confirmed the lantiq_etop probe deadlock is gone at boot.

Changes in v6:
- Fix a build regression in v5: net/ethtool/pse-pd.c called the phylib export phy_pse_control_lock(), but net/ethtool is always in vmlinux while PHYLIB is tristate, so CONFIG_PHYLIB=m or =n failed to link. Move the mutex into pse_core (PSE_CONTROLLER is bool) and reach it through pse_phy_lock() / pse_phy_unlock(), with no-op stubs for !PSE_CONTROLLER. Folded into patch 4; no functional change. Reported by the kernel test robot [8] and by OpenWrt CI.
- Fold phy_device_register_locked() back into phy_device_register(): with the attach no longer taking rtnl the two are identical.
- Patches 1-3 unchanged.

Changes in v5:
- Add patch 4: replace rtnl with a dedicated mutex in the PSE attach path, fixing the ndo_init probe deadlock Aleksander reported [5].
- Add patch 5: put phydev->psec back in phy_device_remove(), closing the off-klist use-after-free Paolo raised [6].
- Patches 1-3 unchanged.

Changes in v4:
- Add Tested-by from Jonas Jelonek. No code changes.
- Repost now that net-next has reopened (v3 was deferred during the merge window).

Changes in v3:
- Drop patch 1 (regulator handle fix); it goes to net separately [2].
- Rebase on net-next. No code changes to the three patches.

v1 was an RFC by Corey [3].

[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
[3] https://lore.kernel.org/netdev/[email protected]/
[4] https://lore.kernel.org/netdev/[email protected]/
[5] https://lore.kernel.org/netdev/[email protected]/
[6] https://lore.kernel.org/netdev/[email protected]/
[7] https://lore.kernel.org/netdev/[email protected]/
[8] https://lore.kernel.org/oe-kbuild-all/[email protected]/

Carlo Szelinsky (2):
net: phy: use a dedicated mutex instead of rtnl for PSE control attach
net: phy: release phydev->psec from phy_device_remove() again

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   | 119 ++++++++++++++++++++++++++++++++-
 drivers/net/pse-pd/pse_core.c  | 100 +++++++++++++++++++++++++++
 include/linux/pse-pd/pse.h     |  64 ++++++++++++++++++
 net/ethtool/pse-pd.c           |  16 +++--
 5 files changed, 293 insertions(+), 40 deletions(-)

base-commit: cef9d6804030793cf8b8796fd6936197d065dd3e --
2.43.0

#1 lore raw

PATCH v6 1/5 net: pse-pd: add notifier chain for controller lifecycle events

From: Corey Leavitt <[email protected]>

Introduce a blocking notifier chain that allows other subsystems to be informed when a PSE controller is registered or unregistered, and provide pse_register_notifier() / pse_unregister_notifier() as the subscriber interface.

Subsequent patches will use this to let the phy subsystem own the phydev->psec lifecycle directly, decoupling PSE lookup from fwnode_mdiobus_register_phy() and removing the probe-time -EPROBE_DEFER coupling that currently exists between mdio, phy and pse-pd when the PSE controller driver is modular.

A blocking chain (rather than atomic) is used because callbacks will take rtnl_lock and call back into pse_core via of_pse_control_get().

The enum pse_controller_event is placed outside the IS_ENABLED(CONFIG_PSE_CONTROLLER) guard so that subscribers compiled into a kernel without PSE support can still reference the event values in dead-code paths without breaking the build.

This patch is pure infrastructure: nothing fires events yet, and nothing subscribes. No observable behavior change.

---
 drivers/net/pse-pd/pse_core.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/pse-pd/pse.h    | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)
drivers/net/pse-pd/pse_core.c +34 -0
index 69dbdbde9d71..3c4d09f1d6e4 100644
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/ethtool.h>
 #include <linux/ethtool_netlink.h>
+#include <linux/notifier.h>
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/pse-pd/pse.h>
@@ -23,6 +24,39 @@ static LIST_HEAD(pse_controller_list);
 static DEFINE_XARRAY_ALLOC(pse_pw_d_map);
 static DEFINE_MUTEX(pse_pw_d_mutex);
 
+static BLOCKING_NOTIFIER_HEAD(pse_controller_notifier);
+
+/**
+ * pse_register_notifier - register a callback for PSE controller events
+ * @nb: notifier block to register
+ *
+ * See enum pse_controller_event for events fired and their subscriber
+ * contract. Callbacks run in process context; they may sleep, take
+ * rtnl, and call of_pse_control_get(). The chain fires synchronously,
+ * so a PSE controller driver's probe/unbind path must not hold any
+ * such lock when calling pse_controller_register() or
+ * pse_controller_unregister().
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int pse_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&pse_controller_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(pse_register_notifier);
+
+/**
+ * pse_unregister_notifier - unregister a previously registered callback
+ * @nb: notifier block previously passed to pse_register_notifier()
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int pse_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&pse_controller_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(pse_unregister_notifier);
+
 /**
  * struct pse_control - a PSE control
  * @pcdev: a pointer to the PSE controller device
include/linux/pse-pd/pse.h +32 -0
index 4e5696cfade7..78fe3a2b1ea8 100644
@@ -21,6 +21,7 @@ struct net_device;
 struct phy_device;
 struct pse_controller_dev;
 struct netlink_ext_ack;
+struct notifier_block;
 
 /* C33 PSE extended state and substate. */
 struct ethtool_c33_pse_ext_state_info {
@@ -337,6 +338,24 @@ enum pse_budget_eval_strategies {
 	PSE_BUDGET_EVAL_STRAT_DYNAMIC	= 1 << 2,
 };
 
+/**
+ * enum pse_controller_event - PSE controller lifecycle events
+ *
+ * Event data in callbacks is always a pointer to the struct
+ * pse_controller_dev firing the event.
+ *
+ * @PSE_REGISTERED: controller added to pse_controller_list and
+ *	resolvable by of_pse_control_get().
+ * @PSE_UNREGISTERED: controller about to be removed from
+ *	pse_controller_list. Subscribers holding pse_control references
+ *	targeting it must drop them before returning and must not
+ *	acquire new references for it.
+ */
+enum pse_controller_event {
+	PSE_REGISTERED,
+	PSE_UNREGISTERED,
+};
+
 #if IS_ENABLED(CONFIG_PSE_CONTROLLER)
 int pse_controller_register(struct pse_controller_dev *pcdev);
 void pse_controller_unregister(struct pse_controller_dev *pcdev);
@@ -366,6 +385,9 @@ int pse_ethtool_set_prio(struct pse_control *psec,
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
 
+int pse_register_notifier(struct notifier_block *nb);
+int pse_unregister_notifier(struct notifier_block *nb);
+
 #else
 
 static inline struct pse_control *of_pse_control_get(struct device_node *node,
@@ -416,6 +438,16 @@ static inline bool pse_has_c33(struct pse_control *psec)
 	return false;
 }
 
+static inline int pse_register_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int pse_unregister_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
 #endif
 
 #endif
Signature · 1 line
2.43.0
#2 lore raw

Re: [PATCH net-next v6 1/5] net: pse-pd: add notifier chain for controller lifecycle events

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

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

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

- [Low] Claim: the patch adds a blocking notifier chain and two EXPORT_SYMBOL_GPL() entry points…

--

net: pse-pd: add notifier chain for controller lifecycle events

This adds a blocking notifier chain to pse_core along with pse_register_notifier() and pse_unregister_notifier() as the subscriber interface, plus enum pse_controller_event describing PSE_REGISTERED and PSE_UNREGISTERED. Nothing fires the chain and nothing subscribes to it in this patch.

No Link: tags present in the commit header.

Quoted 4 lines
drivers/net/pse-pd/pse_core.c +0 -0
index 69dbdbde9d71a..3c4d09f1d6e4f 100644

[ ... ]

Quoted 26 lines
drivers/net/pse-pd/pse_core.c +22 -0
@@ -23,6 +24,39 @@ static LIST_HEAD(pse_controller_list);
 static DEFINE_XARRAY_ALLOC(pse_pw_d_map);
 static DEFINE_MUTEX(pse_pw_d_mutex);
 
+static BLOCKING_NOTIFIER_HEAD(pse_controller_notifier);
+
+/**
+ * pse_register_notifier - register a callback for PSE controller events
+ * @nb: notifier block to register
+ *
+ * See enum pse_controller_event for events fired and their subscriber
+ * contract. Callbacks run in process context; they may sleep, take
+ * rtnl, and call of_pse_control_get(). The chain fires synchronously,
+ * so a PSE controller driver's probe/unbind path must not hold any
+ * such lock when calling pse_controller_register() or
+ * pse_controller_unregister().
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int pse_register_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&pse_controller_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(pse_register_notifier);
+
+/**

drivers/net/pse-pd/pse_core.c static LIST_HEAD(pse_controller_list); · line 58

Quoted 10 lines
drivers/net/pse-pd/pse_core.c +10 -0
+ * pse_unregister_notifier - unregister a previously registered callback
+ * @nb: notifier block previously passed to pse_register_notifier()
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int pse_unregister_notifier(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&pse_controller_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(pse_unregister_notifier);

[Severity: Low]
This isn't a bug, just a note on something that stood out while reading the patch on its own.

At this commit, pse_register_notifier() and pse_unregister_notifier() in drivers/net/pse-pd/pse_core.c are exported with no in-tree caller, and the event-data contract documented for enum pse_controller_event has no producer yet, so the documented semantics cannot be checked from this patch alone.

Following the series forward, the producer side shows up in the next commit, with pse_controller_register() and pse_controller_unregister() doing:

	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_REGISTERED, pcdev);

	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);

and the first subscriber lands in phy_init() in drivers/net/phy/phy_device.c:

	rc = pse_register_notifier(&phy_pse_notifier);

Both call sites pass pcdev, which matches what the kerneldoc for enum pse_controller_event promises, and the commit message states outright that nothing fires or subscribes yet.

So nothing to fix here, this is the usual "add the API, then use it" split.

[ ... ]

Quoted 4 lines
include/linux/pse-pd/pse.h +0 -0
index 4e5696cfade7c..78fe3a2b1ea84 100644

[ ... ]

include/linux/pse-pd/pse.h int pse_ethtool_set_prio(struct pse_control *psec, · line 391

Quoted 8 lines
include/linux/pse-pd/pse.h +3 -0
@@ -366,6 +385,9 @@ int pse_ethtool_set_prio(struct pse_control *psec,
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
 
+int pse_register_notifier(struct notifier_block *nb);
+int pse_unregister_notifier(struct notifier_block *nb);
+
 #else

[ ... ]

No other regressions found. The CONFIG_PSE_CONTROLLER=n stubs returning 0 match the existing stub style in include/linux/pse-pd/pse.h, and placing enum pse_controller_event outside the IS_ENABLED() guard is consistent with the stated goal of letting subscribers reference the values in dead-code paths.

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
#3 lore raw

PATCH v6 2/5 net: pse-pd: fire lifecycle events on controller register/unregister

In reply to Carlo Szelinsky

From: Corey Leavitt <[email protected]>

Hook the newly-introduced pse_controller_notifier chain so that pse_controller_register() fires PSE_REGISTERED after the controller has been added to pse_controller_list (i.e. is now resolvable by of_pse_control_get()), and pse_controller_unregister() fires PSE_UNREGISTERED before the controller is removed from the list (while it is still valid to dereference from a subscriber's pse_control pointer targeting it).

With no subscribers yet, this is observably a no-op. A later change wires the phy subsystem in as the first subscriber.

---
 drivers/net/pse-pd/pse_core.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/pse-pd/pse_core.c +6 -0
index 3c4d09f1d6e4..0190fac33c78 100644
@@ -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;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
@@ -1148,6 +1151,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
+	blocking_notifier_call_chain(&pse_controller_notifier,
+				     PSE_UNREGISTERED, pcdev);
+
 	pse_flush_pw_ds(pcdev);
 	pse_release_pis(pcdev);
 	if (pcdev->irq)
Signature · 1 line
2.43.0
#4 lore raw

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

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

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

- [High] pse_controller_unregister() fires PSE_UNREGISTERED while @pcdev is still linked in pse_controller_list and only calls list_del() at…
- [Low] Reported concern (assessed as not a bug today): both new call sites use blocking_notifier_call_chain() and discard the return value;…

Pre-existing issues:
- [High] pse_controller_unregister() calls pse_release_pis() (which kfree()s pcdev->pi) at pse_core.c:1158, but only drains the…

--

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

This hooks the new pse_controller_notifier chain into the PSE core, firing PSE_REGISTERED after the controller is added to pse_controller_list and PSE_UNREGISTERED before it is removed. A later patch in the series wires the phy subsystem in as the first subscriber.

drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1143

Quoted 11 lines
drivers/net/pse-pd/pse_core.c +3 -0
index 3c4d09f1d6e4f..0190fac33c78a 100644
@@ -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);
+

[Severity: Low]
This isn't a bug today, but both new call sites discard the return value of blocking_notifier_call_chain(). notifier_call_chain() stops walking as soon as a callback returns a value containing NOTIFY_STOP_MASK, so a subscriber returning NOTIFY_STOP or NOTIFY_BAD would hide PSE_UNREGISTERED from subscribers registered later in the chain, and their pse_control objects would then outlive pse_release_pis().

The only in-tree subscriber added by this series, phy_pse_notifier_event(), returns NOTIFY_OK or NOTIFY_DONE, so the chain cannot currently be truncated.

Would it be worth checking notifier_to_errno() (or at least warning) here, and/or warning in pse_controller_unregister() when pcdev->pse_control_head is not empty after the broadcast?

drivers/net/pse-pd/pse_core.c EXPORT_SYMBOL_GPL(pse_controller_register); · line 1156

Quoted 10 lines
drivers/net/pse-pd/pse_core.c +3 -0
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pse_controller_register);
@@ -1148,6 +1151,9 @@ EXPORT_SYMBOL_GPL(pse_controller_register);
  */
 void pse_controller_unregister(struct pse_controller_dev *pcdev)
 {
+	blocking_notifier_call_chain(&pse_controller_notifier,
+				     PSE_UNREGISTERED, pcdev);
+

[Severity: High]
Can a new pse_control still be handed out for this controller after the event has been broadcast?

PSE_UNREGISTERED is emitted while pcdev is still linked in pse_controller_list, and list_del() only happens at the very end of pse_controller_unregister(), after pse_flush_pw_ds(), pse_release_pis(), disable_irq() and kfifo_free().

of_pse_control_get() gates resolution purely on list membership plus try_module_get(pcdev->owner):

	list_for_each_entry(r, &pse_controller_list, list) {
		if (!r->no_of_pse_pi) {
			ret = of_pse_match_pi(r, args.np);

There is no "dying" state that of_pse_control_get() rejects, and try_module_get() succeeds in the common case since unregister runs from the devm release on device unbind while the provider module is still loaded.

The subscriber added later in the series does not close the window either:
phy_pse_notifier_event() takes and then releases pse_phy_mutex, while the concurrent acquirer path is:

phy_device_register()
pse_phy_lock()
phy_try_attach_pse()
of_pse_control_get()
pse_phy_unlock()

so the lock does not cover the rest of the teardown. Two outcomes look possible:

If the attach wins before pse_release_pis(), the phy stores a pse_control whose psec->pcdev->pi[] array is freed moments later, and the final put reads 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);
	devm_regulator_put(psec->ps);

	module_put(psec->pcdev->owner);

If the lookup runs after pse_release_pis(), of_pse_match_pi() dereferences pcdev->pi[i].np directly, and pse_release_pis() does not clear the pointer:

drivers/net/pse-pd/pse_core.c:pse_release_pis() {
	...
	kfree(pcdev->pi);
}

The header text added by the series says subscribers "must not acquire new references for it" for PSE_UNREGISTERED, but non-subscriber lookup paths can still resolve the controller.

Would it be more robust to make the controller unresolvable first (list_del() or a dying flag checked by of_pse_control_get()) and only then broadcast PSE_UNREGISTERED?

drivers/net/pse-pd/pse_core.c EXPORT_SYMBOL_GPL(pse_controller_register); · line 1159

Quoted 3 lines
drivers/net/pse-pd/pse_core.c +0 -0
 	pse_flush_pw_ds(pcdev);
 	pse_release_pis(pcdev);
 	if (pcdev->irq)

[Severity: High]
This isn't a bug introduced by this patch, but the ordering here becomes easier to hit with the new broadcast, so it seems worth raising.

pse_release_pis() frees pcdev->pi, while the notification worker is only drained afterwards:

	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	if (pcdev->irq)
		disable_irq(pcdev->irq);
	cancel_work_sync(&pcdev->ntf_work);

pse_send_ntf_worker() takes a real reference via pse_control_find_by_id():

		if (psec->id == id) {
			kref_get(&psec->refcnt);

then drops pse_list_mutex and sleeps across rtnl_lock() / ethnl_pse_send_ntf() / rtnl_unlock() before calling pse_control_put().

With PSE_UNREGISTERED now telling the phy subscriber to drop its reference first, can the in-flight worker end up being the last holder, so that its pse_control_put() runs __pse_control_release() against the already freed pcdev->pi:

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

Should disable_irq() and cancel_work_sync() run before pse_release_pis(), so the IRQ producer and the worker are stopped and drained before controller state is released?

Quoted 2 lines
		disable_irq(pcdev->irq);

cancel_work_sync(&pcdev->ntf_work);

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
#5 lore raw

PATCH v6 3/5 net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

In reply to Carlo Szelinsky

From: Corey Leavitt <[email protected]>

Transfer ownership of phydev->psec from fwnode_mdio to the phy subsystem itself. The phy subsystem now subscribes to the pse-pd notifier chain and manages psec attach/detach in response to PSE controller lifecycle events, while fwnode_mdio loses its PSE awareness entirely.

phydev->psec is attached after device_add() has made the phy visible on mdio_bus_type, under a narrow rtnl_lock() that covers only phy_try_attach_pse(). Ordering the attach after registration closes the race that would otherwise leave a phy unattached: a PSE_REGISTERED event firing during registration walks mdio_bus_type and either finds the phy already added (and attaches it) or runs before device_add(), in which case the post-add attach resolves it. The phydev->psec check in phy_try_attach_pse() makes the two paths idempotent. Holding rtnl across of_pse_control_get() is safe because pse_list_mutex is never taken in the opposite order.

device_add() is deliberately left outside rtnl. Binding a phy that itself provides an SFP cage reaches sfp_bus_add_upstream() through phy_probe() -> phy_setup_ports() -> phy_sfp_probe(), and sfp_bus_add_upstream() takes rtnl_lock(); holding rtnl across device_add() would deadlock such phys (reported on RTL8214FC).

phy_device_register() is split into the public form, which takes the narrow rtnl_lock() around the attach, and a phy_device_register_locked() form for callers that already hold rtnl (the SFP module state machine via __sfp_sm_event). This pair mirrors the register_netdevice() / register_netdev() split convention already established in the core networking stack. The _locked form runs device_add() under the caller's rtnl, which is safe because a phy resident on an SFP module does not itself provide a downstream cage, so phy_sfp_probe() is a no-op there.

- On PSE_REGISTERED: an rtnl-guarded bus walk retries the attach for every registered phy whose psec is still NULL. This is the "phy was enumerated before the PSE controller loaded" case, the root cause of the boot-time probe-retry storm on systems with a modular PSE controller driver.

- On PSE_UNREGISTERED: an rtnl-guarded bus walk releases every

    phydev->psec that targets the departing controller before
    pse_release_pis() frees pcdev->pi. Without this, a phy still
    holding a pse_control reference would cause a use-after-free in
    __pse_control_release()'s pcdev->pi[psec->id] access, and the PSE
    driver module could not finish unloading while any phy still held a
    reference.

A bad pses binding -- an error from of_pse_control_get() other than -ENOENT (no phandle) or -EPROBE_DEFER (controller not yet registered) -- is reported with phydev_warn() rather than silently dropped, preserving the diagnostic that the removed fwnode_mdio lookup used to provide.

The final pse_control_put() of phydev->psec moves from phy_device_remove() to phy_device_release(), so it runs only after every reference on the device -- including the bus-iterator references taken by bus_for_each_dev() in the notifier walk -- has been dropped.

Finally, delete fwnode_find_pse_control() and its call site in fwnode_mdiobus_register_phy(), and drop the PSE header from fwnode_mdio.c. The MDIO/DSA probe no longer sees any PSE-originated -EPROBE_DEFER, so the probe-retry storm is gone and fwnode_mdio is now PSE-agnostic.

---
 drivers/net/mdio/fwnode_mdio.c |  34 -------
 drivers/net/phy/phy_device.c   | 168 +++++++++++++++++++++++++++++++--
 drivers/net/phy/sfp.c          |   2 +-
 drivers/net/pse-pd/pse_core.c  |  14 +++
 include/linux/phy.h            |   2 +
 include/linux/pse-pd/pse.h     |   9 ++
 6 files changed, 186 insertions(+), 43 deletions(-)
drivers/net/mdio/fwnode_mdio.c +0 -34
index ba7091518265..7bd979b59f49 100644
@@ -11,33 +11,11 @@
 #include <linux/fwnode_mdio.h>
 #include <linux/of.h>
 #include <linux/phy.h>
-#include <linux/pse-pd/pse.h>
 
 MODULE_AUTHOR("Calvin Johnson <[email protected]>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("FWNODE MDIO bus (Ethernet PHY) accessors");
 
-static struct pse_control *
-fwnode_find_pse_control(struct fwnode_handle *fwnode,
-			struct phy_device *phydev)
-{
-	struct pse_control *psec;
-	struct device_node *np;
-
-	if (!IS_ENABLED(CONFIG_PSE_CONTROLLER))
-		return NULL;
-
-	np = to_of_node(fwnode);
-	if (!np)
-		return NULL;
-
-	psec = of_pse_control_get(np, phydev);
-	if (PTR_ERR(psec) == -ENOENT)
-		return NULL;
-
-	return psec;
-}
-
 static struct mii_timestamper *
 fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
 {
@@ -118,7 +96,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 				struct fwnode_handle *child, u32 addr)
 {
 	struct mii_timestamper *mii_ts = NULL;
-	struct pse_control *psec = NULL;
 	struct phy_device *phy;
 	bool is_c45;
 	u32 phy_id;
@@ -159,14 +136,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 			goto clean_phy;
 	}
 
-	psec = fwnode_find_pse_control(child, phy);
-	if (IS_ERR(psec)) {
-		rc = PTR_ERR(psec);
-		goto unregister_phy;
-	}
-
-	phy->psec = psec;
-
 	/* phy->mii_ts may already be defined by the PHY driver. A
 	 * mii_timestamper probed via the device tree will still have
 	 * precedence.
@@ -176,9 +145,6 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
 
 	return 0;
 
-unregister_phy:
-	if (is_acpi_node(child) || is_of_node(child))
-		phy_device_remove(phy);
 clean_phy:
 	phy_device_free(phy);
 clean_mii_ts:
drivers/net/phy/phy_device.c +160 -8

211 lines not on this page · raw message

drivers/net/phy/sfp.c +1 -1
index 03bfd8640db9..18868bdd6485 100644
@@ -2083,7 +2083,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
 	/* Mark this PHY as being on a SFP module */
 	phy->is_on_sfp_module = true;
 
-	err = phy_device_register(phy);
+	err = phy_device_register_locked(phy);
 	if (err) {
 		phy_device_free(phy);
 		dev_err(sfp->dev, "phy_device_register failed: %pe\n",
drivers/net/pse-pd/pse_core.c +14 -0
index 0190fac33c78..611540df43bb 100644
@@ -2021,3 +2021,17 @@ bool pse_has_c33(struct pse_control *psec)
 	return psec->pcdev->types & ETHTOOL_PSE_C33;
 }
 EXPORT_SYMBOL_GPL(pse_has_c33);
+
+/**
+ * pse_control_matches_pcdev - Test whether a pse_control targets a controller
+ * @psec: pse_control obtained from of_pse_control_get()
+ * @pcdev: PSE controller to compare against
+ *
+ * Return: %true if @psec was obtained from @pcdev, %false otherwise.
+ */
+bool pse_control_matches_pcdev(struct pse_control *psec,
+			       struct pse_controller_dev *pcdev)
+{
+	return psec->pcdev == pcdev;
+}
+EXPORT_SYMBOL_GPL(pse_control_matches_pcdev);
include/linux/phy.h +2 -0
index 199a7aaa341b..865b9baddb85 100644
@@ -2158,6 +2158,8 @@ struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
+/* Caller must hold rtnl_lock(); see phy_device_register() for the public form. */
+int phy_device_register_locked(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 void phy_device_remove(struct phy_device *phydev);
 int phy_get_c45_ids(struct phy_device *phydev);
include/linux/pse-pd/pse.h +9 -0
index 78fe3a2b1ea8..d4310ca71a3e 100644
@@ -385,6 +385,9 @@ int pse_ethtool_set_prio(struct pse_control *psec,
 bool pse_has_podl(struct pse_control *psec);
 bool pse_has_c33(struct pse_control *psec);
 
+bool pse_control_matches_pcdev(struct pse_control *psec,
+			       struct pse_controller_dev *pcdev);
+
 int pse_register_notifier(struct notifier_block *nb);
 int pse_unregister_notifier(struct notifier_block *nb);
 
@@ -438,6 +441,12 @@ static inline bool pse_has_c33(struct pse_control *psec)
 	return false;
 }
 
+static inline bool pse_control_matches_pcdev(struct pse_control *psec,
+					     struct pse_controller_dev *pcdev)
+{
+	return false;
+}
+
 static inline int pse_register_notifier(struct notifier_block *nb)
 {
 	return 0;
Signature · 1 line
2.43.0
#6 lore raw

Re: [PATCH net-next v6 3/5] net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

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

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

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

- [High] Deferred psec put vs. bus-walk-based detach: moving the final pse_control_put(phydev->psec) from phy_device_remove() to…
- [High] The PSE_UNREGISTERED detach walk does not actually close the use-after-free class the commit claims to fix: the notifier callback…
- [Medium] pse_controller_unregister() frees the PI storage before quiescing the notification worker, and the new PSE_UNREGISTERED detach…
- [Medium] phy_try_attach_pse() treats every of_pse_control_get() error other than -ENOENT/-EPROBE_DEFER as permanent and warn-only, but that…
- [Low] The commit message claims that without the new PSE_UNREGISTERED detach walk "the PSE driver module could not finish unloading while…
- [Low] The patch fixes a reported, user-visible regression (boot-time probe-retry storm / PHYs failing to register when the PSE controller…

--

net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook

This moves ownership of phydev->psec from fwnode_mdio into phylib, which now subscribes to the pse-pd notifier chain and attaches/detaches psec on PSE controller register/unregister events. phy_device_register() is split into a public form that takes rtnl around the attach and a phy_device_register_locked() form for callers already holding rtnl, and the final pse_control_put() moves from phy_device_remove() to phy_device_release().

A couple of questions on the changelog first.

[Severity: Low]

Quoted 7 lines

- On PSE_UNREGISTERED: an rtnl-guarded bus walk releases every phydev->psec that targets the departing controller before pse_release_pis() frees pcdev->pi. Without this, a phy still holding a pse_control reference would cause a use-after-free in __pse_control_release()'s pcdev->pi[psec->id] access, and the PSE driver module could not finish unloading while any phy still held a reference.

Is the module unload part of this rationale the right way round?

pse_control_get_internal() pins the controller driver for every psec:

	if (!try_module_get(pcdev->owner)) {
		ret = -ENODEV;
		goto free_psec;
	}

and the matching module_put() only happens in __pse_control_release(). While a phy holds a psec the module refcount is non-zero, so delete_module() fails with -EWOULDBLOCK before the module exit path (and therefore pse_controller_unregister(), the only emitter of PSE_UNREGISTERED) is ever entered.

The detach walk does help for driver unbind and device removal, where pse_controller_unregister() is reached with the module still loaded, but it cannot make an rmmod that was already refused succeed. Could that sentence be reworded to talk about unbind/removal instead?

[Severity: Low]

Quoted 2 lines

This carries Reported-by:, Closes: and Tested-by: for a user-visible regression (PHYs failing to register / probe-retry storm with a modular PSE controller driver), but there is no Fixes: tag and no indication of the target tree.

git blame on the removed lookup in fwnode_mdiobus_register_phy() points at 5e82147de1cbd7 for the original PSE lookup, later touched by fa2f0454174c2f, so the affected releases can be named. Could a Fixes: tag be added, and could the intended tree (net vs net-next) be stated given the size of the refactor and the new exported symbol?

Quoted 13 lines
drivers/net/phy/phy_device.c +5 -0
index 0615228459ef4..f5febff4b00b8 100644
@@ -223,8 +223,19 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
 
 static void phy_device_release(struct device *dev)
 {
+	struct phy_device *phydev = to_phy_device(dev);
+
+	/* bus_for_each_dev() holds get_device() across each iteration
+	 * step, deferring this release callback until any in-flight PSE
+	 * notifier walk has advanced past this phy. pse_control_put()

drivers/net/phy/phy_device.c static void phy_mdio_device_free(struct mdio_device *mdiodev) · line 239

Quoted 10 lines
drivers/net/phy/phy_device.c +7 -1
+	 * takes pse_list_mutex, so this path must run in sleepable
+	 * context.
+	 */
+	might_sleep();
+	pse_control_put(phydev->psec);
+
 	fwnode_handle_put(dev->fwnode);
-	kfree(to_phy_device(dev));
+	kfree(phydev);
 }

[Severity: High]

Can a psec survive here past the point where the detach walk can find it?

Once phy_device_remove() has run, device_del() -> bus_remove_device() -> klist_del() takes the phy off the mdio_bus_type klist, but the phy itself can still be pinned by a get_device() from phy_attach_direct() or of_phy_find_device(). The detach is driven only by the klist walk:

	case PSE_UNREGISTERED:
		rtnl_lock();
		bus_for_each_dev(&mdio_bus_type, NULL, data,
				 phy_pse_detach_one);

so an off-bus phy keeps its psec, while pse_controller_unregister() continues straight on:

	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);
	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);		/* kfree(pcdev->pi) */

When the last device reference finally drops and this release callback runs, __pse_control_release() does:

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

which reads the freed pi array and may act on it.

The last patch of this series ("net: phy: release phydev->psec from phy_device_remove() again") restores the put plus phydev->psec = NULL under pse_phy_lock() in phy_device_remove() before device_del(), which is the ordering that avoids this. Would it be better to keep the put in phy_device_remove() from this patch onwards so the intermediate tree is not left with the window open?

Quoted 28 lines
drivers/net/phy/phy_device.c +19 -4
@@ -1102,11 +1113,103 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 }
 EXPORT_SYMBOL(get_phy_device);
 
-/**
- * phy_device_register - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
+/* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
+ * Caller must hold rtnl. A missing phandle (-ENOENT) or a not-yet-registered
+ * controller (-EPROBE_DEFER) is silent; the notifier retries the latter at
+ * PSE_REGISTERED time. Any other error means a broken binding and is warned
+ * about, but left non-fatal so the phy still registers.
  */
-int phy_device_register(struct phy_device *phydev)
+static void phy_try_attach_pse(struct phy_device *phydev)
+{
+	struct pse_control *psec;
+	struct device_node *np;
+
+	ASSERT_RTNL();
+
+	np = phydev->mdio.dev.of_node;
+	if (!np)
+		return;
+
+	if (phydev->psec)
+		return;
+

drivers/net/phy/phy_device.c struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) · line 1145

Quoted 10 lines
drivers/net/phy/phy_device.c +10 -0
+	psec = of_pse_control_get(np, phydev);
+	if (IS_ERR(psec)) {
+		if (PTR_ERR(psec) != -EPROBE_DEFER && PTR_ERR(psec) != -ENOENT)
+			phydev_warn(phydev, "failed to get PSE control: %pe\n",
+				    psec);
+		return;
+	}
+
+	phydev->psec = psec;
+}

[Severity: Medium]

Is every error other than -ENOENT and -EPROBE_DEFER really a broken binding? of_pse_control_get() does hardware traffic on this path:

drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
	...
	ret = pse_pi_is_hw_enabled(pcdev, index);
	if (ret < 0)
		goto free_psec;
	pcdev->pi[index].admin_state_enabled = ret;
	...
	psec->ps = devm_regulator_get_exclusive(...);
}

pse_pi_is_hw_enabled() calls pcdev->ops->pi_get_admin_state(), which is an I2C/SMBus read on tps23881, si3474 and pd692x0, so a transient -EIO, -EBUSY or -ENOMEM is possible. Here that is only warned about, phydev->psec stays NULL, and the only retry left is a fresh PSE_REGISTERED event.

That is aggravated by PSE_REGISTERED firing synchronously from the tail of pse_controller_register(), i.e. mid-probe of the controller driver:

drivers/net/pse-pd/tps23881.c:tps23881_probe() {
	...
	ret = devm_pse_controller_register(dev, &priv->pcdev);
	...
	ret = tps23881_setup_irq(priv, client->irq);
}

Previously the same failure propagated out of fwnode_mdiobus_register_phy() and deferred probe retried it. With this change, does a single transient bus error permanently lose the PSE binding for that port, leaving the PI without a consumer and unreachable from ethtool even though admin_state_enabled may have been seeded from the chip?

Quoted 22 lines
drivers/net/phy/phy_device.c +22 -0
+
+static int phy_pse_attach_one(struct device *dev, void *data __maybe_unused)
+{
+	ASSERT_RTNL();
+
+	if (dev->type != &mdio_bus_phy_type)
+		return 0;
+
+	phy_try_attach_pse(to_phy_device(dev));
+	return 0;
+}
+
+static int phy_pse_detach_one(struct device *dev, void *data)
+{
+	struct pse_controller_dev *pcdev = data;
+	struct phy_device *phydev;
+	struct pse_control *psec;
+
+	ASSERT_RTNL();
+
+	if (dev->type != &mdio_bus_phy_type)
+		return 0;

drivers/net/phy/phy_device.c struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) · line 1177

Quoted 10 lines
drivers/net/phy/phy_device.c +10 -0
+
+	phydev = to_phy_device(dev);
+	psec = phydev->psec;
+	if (!psec || !pse_control_matches_pcdev(psec, pcdev))
+		return 0;
+
+	phydev->psec = NULL;
+	pse_control_put(psec);
+	return 0;
+}

[Severity: Medium]

Once this walk drops the phy's reference, can the notification worker end up being the last holder of the psec?

pse_send_ntf_worker() takes its own reference and then blocks on rtnl:

	psec = pse_control_find_by_id(pcdev, ntf.id);
	rtnl_lock();
	netdev = pse_control_get_netdev(psec);
	...
	rtnl_unlock();
	pse_control_put(psec);

The detach walk runs under rtnl, so it can complete in that window and the psec then survives only on the worker's reference. Meanwhile pse_controller_unregister() frees the PI storage before quiescing the worker:

	pse_release_pis(pcdev);		/* kfree(pcdev->pi) */
	if (pcdev->irq)
		disable_irq(pcdev->irq);
	cancel_work_sync(&pcdev->ntf_work);

so the worker's pse_control_put() reaches __pse_control_release() and reads psec->pcdev->pi[psec->id].admin_state_enabled after the array is gone. The still-enabled IRQ can also queue more notification work before cancel_work_sync(). Should disable_irq() and cancel_work_sync() move ahead of pse_flush_pw_ds()/pse_release_pis()?

Quoted 11 lines
drivers/net/phy/phy_device.c +11 -0
+
+static int phy_pse_notifier_event(struct notifier_block *nb,
+				  unsigned long event, void *data)
+{
+	switch (event) {
+	case PSE_REGISTERED:
+		rtnl_lock();
+		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
+				 phy_pse_attach_one);
+		rtnl_unlock();
+		return NOTIFY_OK;

drivers/net/phy/phy_device.c struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45) · line 1198

Quoted 10 lines
drivers/net/phy/phy_device.c +10 -0
+	case PSE_UNREGISTERED:
+		rtnl_lock();
+		bus_for_each_dev(&mdio_bus_type, NULL, data,
+				 phy_pse_detach_one);
+		rtnl_unlock();
+		return NOTIFY_OK;
+	default:
+		return NOTIFY_DONE;
+	}
+}

[Severity: High]

Does the detach walk actually close the use-after-free the changelog describes, given that rtnl is dropped before the notifier returns?

pse_controller_unregister() unlinks the controller last:

	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);
	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);		/* kfree(pcdev->pi) */
	...
	mutex_lock(&pse_list_mutex);
	list_del(&pcdev->list);

In the window after the notifier returns and before that list_del(), a phy registering on another CPU runs phy_device_register() -> phy_try_attach_pse() -> of_pse_control_get(), still finds the departing pcdev on pse_controller_list, and creates a fresh psec on it. That path touches the already freed pi array: of_pse_match_pi() reads pcdev->pi[i].np, and pse_control_get_internal() reads pcdev->pi[index].rdev and writes pcdev->pi[index].admin_state_enabled = ret.

Since the attach decision is taken under rtnl but the rest of the teardown is not, this looks like a check-then-attach race against controller teardown. Would unlinking pcdev from pse_controller_list before the notifier fires, or publishing a "dying" state under the same lock the attach uses, be needed here?

Quoted 9 lines
drivers/net/phy/phy_device.c +0 -2
@@ -1152,8 +1299,6 @@ EXPORT_SYMBOL(phy_device_register);
 void phy_device_remove(struct phy_device *phydev)
 {
 	unregister_mii_timestamper(phydev->mii_ts);
-	pse_control_put(phydev->psec);
-
 	device_del(&phydev->mdio.dev);
 
 	/* Assert the reset signal */

[ ... ]

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
#7 lore raw

PATCH v6 4/5 net: phy: use a dedicated mutex instead of rtnl for PSE control attach

In reply to Carlo Szelinsky

phy_device_register() took rtnl_lock() around phy_try_attach_pse() to serialise phydev->psec against the PSE controller notifier walk. But an MDIO bus registered from ndo_init() runs with rtnl already held:

register_netdevice() # holds rtnl

    ndo_init() == ltq_etop_init()
      ltq_etop_mdio_init()
        mdiobus_register()
          mdiobus_scan()
            phy_device_register()
              rtnl_lock()		# deadlock

so any such driver (lantiq_etop, sni_ave, netsec) deadlocks on probe.

Replace rtnl with a dedicated mutex for the attach, for the notifier attach/detach walks, and for the ethtool PSE paths that dereference phydev->psec. A private lock cannot recurse against the caller's rtnl, so the register path no longer deadlocks, while attach vs notifier and detach vs ethtool stay mutually excluded.

The mutex lives in pse_core, not phylib, and is reached through pse_phy_lock() / pse_phy_unlock(). net/ethtool/pse-pd.c is built into vmlinux unconditionally while PHYLIB is tristate, so ethtool cannot call a phylib export: with CONFIG_PHYLIB=m or =n the link fails. PSE_CONTROLLER is bool, so pse_core is always in vmlinux (or absent, with no-op stubs for !PSE_CONTROLLER), and both built-in and modular callers reach the lock. It does not protect a phylib field but the phy<->controller binding against PSE controller teardown, and it sits directly above pse_list_mutex, which pse_core already owns.

rtnl also kept the ethtool PSE reads from racing the PSE_UNREGISTERED detach that frees phydev->psec, so net/ethtool/pse-pd.c takes the same lock across its phydev->psec accesses; guarding only the phy side would reopen a use-after-free there. The lock order is
rtnl -> pse_phy_mutex -> pse_list_mutex -> pcdev->lock, and the notifier walks enter at pse_phy_lock() and never take rtnl.

Because the attach no longer takes rtnl, phy_device_register() and the phy_device_register_locked() variant that was added for the rtnl-holding sfp caller became identical, so fold them back into phy_device_register().

---
 drivers/net/phy/phy_device.c  | 94 +++++++++++------------------------
 drivers/net/phy/sfp.c         |  2 +-
 drivers/net/pse-pd/pse_core.c | 46 +++++++++++++++++
 include/linux/phy.h           |  2 -
 include/linux/pse-pd/pse.h    | 23 +++++++++
 net/ethtool/pse-pd.c          | 16 ++++--
 6 files changed, 111 insertions(+), 72 deletions(-)
drivers/net/phy/phy_device.c +30 -64
index f5febff4b00b..e8d894bbfa7a 100644
@@ -1114,17 +1114,17 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 EXPORT_SYMBOL(get_phy_device);
 
 /* Best-effort attach of phydev->psec from a DT `pses = <&...>` phandle.
- * Caller must hold rtnl. A missing phandle (-ENOENT) or a not-yet-registered
- * controller (-EPROBE_DEFER) is silent; the notifier retries the latter at
- * PSE_REGISTERED time. Any other error means a broken binding and is warned
- * about, but left non-fatal so the phy still registers.
+ * Caller must hold pse_phy_lock(). A missing phandle (-ENOENT) or a
+ * not-yet-registered controller (-EPROBE_DEFER) is silent; the notifier
+ * retries the latter at PSE_REGISTERED time. Any other error means a broken
+ * binding and is warned about, but left non-fatal so the phy still registers.
  */
 static void phy_try_attach_pse(struct phy_device *phydev)
 {
 	struct pse_control *psec;
 	struct device_node *np;
 
-	ASSERT_RTNL();
+	pse_phy_lock_assert_held();
 
 	np = phydev->mdio.dev.of_node;
 	if (!np)
@@ -1146,7 +1146,7 @@ static void phy_try_attach_pse(struct phy_device *phydev)
 
 static int phy_pse_attach_one(struct device *dev, void *data __maybe_unused)
 {
-	ASSERT_RTNL();
+	pse_phy_lock_assert_held();
 
 	if (dev->type != &mdio_bus_phy_type)
 		return 0;
@@ -1161,7 +1161,7 @@ static int phy_pse_detach_one(struct device *dev, void *data)
 	struct phy_device *phydev;
 	struct pse_control *psec;
 
-	ASSERT_RTNL();
+	pse_phy_lock_assert_held();
 
 	if (dev->type != &mdio_bus_phy_type)
 		return 0;
@@ -1181,16 +1181,16 @@ static int phy_pse_notifier_event(struct notifier_block *nb,
 {
 	switch (event) {
 	case PSE_REGISTERED:
-		rtnl_lock();
+		pse_phy_lock();
 		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
 				 phy_pse_attach_one);
-		rtnl_unlock();
+		pse_phy_unlock();
 		return NOTIFY_OK;
 	case PSE_UNREGISTERED:
-		rtnl_lock();
+		pse_phy_lock();
 		bus_for_each_dev(&mdio_bus_type, NULL, data,
 				 phy_pse_detach_one);
-		rtnl_unlock();
+		pse_phy_unlock();
 		return NOTIFY_OK;
 	default:
 		return NOTIFY_DONE;
@@ -1201,15 +1201,22 @@ static struct notifier_block phy_pse_notifier __read_mostly = {
 	.notifier_call = phy_pse_notifier_event,
 };
 
-/* Core registration: add the phy to the MDIO bus. Does not touch rtnl or
- * PSE. phydev->psec is attached by the callers below, after device_add()
- * has made the phy visible on mdio_bus_type, so that a concurrent PSE
- * notifier walk and the attach can never leave the phy unattached. Keeping
- * device_add() out of rtnl also avoids deadlocking when binding a phy that
- * itself provides an SFP cage (phy_probe() -> phy_sfp_probe() ->
- * sfp_bus_add_upstream() takes rtnl).
+/**
+ * phy_device_register - Register the phy device on the MDIO bus
+ * @phydev: phy_device structure to be added to the MDIO bus
+ *
+ * phydev->psec is attached after device_add() has made the phy visible on
+ * mdio_bus_type, so that a concurrent PSE notifier walk and the attach can
+ * never leave the phy unattached. Neither step takes rtnl: keeping
+ * device_add() out of rtnl avoids deadlocking when binding a phy that itself
+ * provides an SFP cage (phy_probe() -> phy_sfp_probe() ->
+ * sfp_bus_add_upstream() takes rtnl), and pse_phy_lock() rather than rtnl
+ * guards the attach so a bus registered from ndo_init (which already holds
+ * rtnl) does not recurse on it.
+ *
+ * Return: 0 on success, negative error code on failure.
  */
-static int __phy_device_register(struct phy_device *phydev)
+int phy_device_register(struct phy_device *phydev)
 {
 	int err;
 
@@ -1233,6 +1240,10 @@ static int __phy_device_register(struct phy_device *phydev)
 		goto out;
 	}
 
+	pse_phy_lock();
+	phy_try_attach_pse(phydev);
+	pse_phy_unlock();
+
 	return 0;
 
  out:
@@ -1241,51 +1252,6 @@ static int __phy_device_register(struct phy_device *phydev)
 	mdiobus_unregister_device(&phydev->mdio);
 	return err;
 }
-
-/**
- * phy_device_register_locked - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
- *
- * Same as phy_device_register() but caller must already hold rtnl_lock().
- *
- * Return: 0 on success, negative error code on failure.
- */
-int phy_device_register_locked(struct phy_device *phydev)
-{
-	int err;
-
-	ASSERT_RTNL();
-
-	err = __phy_device_register(phydev);
-	if (err)
-		return err;
-
-	phy_try_attach_pse(phydev);
-
-	return 0;
-}
-EXPORT_SYMBOL(phy_device_register_locked);
-
-/**
- * phy_device_register - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
- *
- * Return: 0 on success, negative error code on failure.
- */
-int phy_device_register(struct phy_device *phydev)
-{
-	int err;
-
-	err = __phy_device_register(phydev);
-	if (err)
-		return err;
-
-	rtnl_lock();
-	phy_try_attach_pse(phydev);
-	rtnl_unlock();
-
-	return 0;
-}
 EXPORT_SYMBOL(phy_device_register);
 
 /**
drivers/net/phy/sfp.c +1 -1
index 18868bdd6485..03bfd8640db9 100644
@@ -2083,7 +2083,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, int addr, bool is_c45)
 	/* Mark this PHY as being on a SFP module */
 	phy->is_on_sfp_module = true;
 
-	err = phy_device_register_locked(phy);
+	err = phy_device_register(phy);
 	if (err) {
 		phy_device_free(phy);
 		dev_err(sfp->dev, "phy_device_register failed: %pe\n",
drivers/net/pse-pd/pse_core.c +46 -0

54 lines not on this page · raw message

include/linux/phy.h +0 -2
index 865b9baddb85..199a7aaa341b 100644
@@ -2158,8 +2158,6 @@ struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct fwnode_handle *fwnode_get_phy_node(const struct fwnode_handle *fwnode);
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
-/* Caller must hold rtnl_lock(); see phy_device_register() for the public form. */
-int phy_device_register_locked(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 void phy_device_remove(struct phy_device *phydev);
 int phy_get_c45_ids(struct phy_device *phydev);
include/linux/pse-pd/pse.h +23 -0
index d4310ca71a3e..8c10613d9c51 100644
@@ -391,6 +391,17 @@ bool pse_control_matches_pcdev(struct pse_control *psec,
 int pse_register_notifier(struct notifier_block *nb);
 int pse_unregister_notifier(struct notifier_block *nb);
 
+void pse_phy_lock(void);
+void pse_phy_unlock(void);
+
+#ifdef CONFIG_LOCKDEP
+void pse_phy_lock_assert_held(void);
+#else
+static inline void pse_phy_lock_assert_held(void)
+{
+}
+#endif
+
 #else
 
 static inline struct pse_control *of_pse_control_get(struct device_node *node,
@@ -457,6 +468,18 @@ static inline int pse_unregister_notifier(struct notifier_block *nb)
 	return 0;
 }
 
+static inline void pse_phy_lock(void)
+{
+}
+
+static inline void pse_phy_unlock(void)
+{
+}
+
+static inline void pse_phy_lock_assert_held(void)
+{
+}
+
 #endif
 
 #endif
net/ethtool/pse-pd.c +11 -5
index 757c9e0cc856..654325946aaa 100644
@@ -71,7 +71,9 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
 	if (ret < 0)
 		return ret;
 
+	pse_phy_lock();
 	ret = pse_get_pse_attributes(phydev, info->extack, data);
+	pse_phy_unlock();
 
 	ethnl_ops_complete(dev);
 
@@ -281,9 +283,12 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PSE_HEADER,
 				      info->extack);
+
+	pse_phy_lock();
+
 	ret = ethnl_set_pse_validate(phydev, info);
 	if (ret)
-		return ret;
+		goto out;
 
 	if (tb[ETHTOOL_A_PSE_PRIO]) {
 		unsigned int prio;
@@ -291,7 +296,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		prio = nla_get_u32(tb[ETHTOOL_A_PSE_PRIO]);
 		ret = pse_ethtool_set_prio(phydev->psec, info->extack, prio);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	if (tb[ETHTOOL_A_C33_PSE_AVAIL_PW_LIMIT]) {
@@ -301,7 +306,7 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		ret = pse_ethtool_set_pw_limit(phydev->psec, info->extack,
 					       pw_limit);
 		if (ret)
-			return ret;
+			goto out;
 	}
 
 	/* These values are already validated by the ethnl_pse_set_policy */
@@ -319,10 +324,11 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 		 */
 		ret = pse_ethtool_set_config(phydev->psec, info->extack,
 					     &config);
-		if (ret)
-			return ret;
 	}
 
+out:
+	pse_phy_unlock();
+
 	/* Return errno or zero - PSE has no notification */
 	return ret;
 }
Signature · 1 line
2.43.0
#8 lore raw

Re: [PATCH net-next v6 4/5] net: phy: use a dedicated mutex instead of rtnl for PSE control attach

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

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

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

- [High] At this commit, phydev->psec is released asynchronously and without the new lock: phy_device_remove()…
- [High] pse_phy_lock() only serialises the attach against the notifier callback itself, not against the controller teardown that follows it.
- [Medium] Commit message hides that the deadlock being fixed was introduced two patches earlier in the same series.
- [Low] The commit message wrongly lists netsec among the drivers that deadlock ("so any such driver (lantiq_etop, sni_ave, netsec) deadlocks…

--

net: phy: use a dedicated mutex instead of rtnl for PSE control attach

This replaces the rtnl_lock() that phy_device_register() took around phy_try_attach_pse() with a new mutex living in pse_core, reached via pse_phy_lock() / pse_phy_unlock(), and uses it in the PSE notifier walks and in the ethtool PSE paths. phy_device_register_locked() becomes identical to phy_device_register() and is folded back in.

A few questions on the changelog first.

Quoted 2 lines

phy_device_register() took rtnl_lock() around phy_try_attach_pse() to serialise phydev->psec against the PSE controller notifier walk.

[Severity: Medium]
Should this say where that rtnl_lock() came from? At the baseline commit cef9d6804030, drivers/net/phy/phy_device.c has no rtnl in phy_device_register() and no phy_device_register_locked() at all; both were added by the earlier patch in this same series ("net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook").

As written the message reads as if the rtnl_lock() were pre-existing upstream behaviour, and there is no Fixes: tag or note that the deadlock is introduced inside this submission.

That also leaves the series non-bisectable: with only the earlier patches applied, register_netdevice() holds rtnl and calls ltq_etop_init() -> ltq_etop_mdio_init() -> mdiobus_register() -> mdiobus_scan() -> phy_device_register() -> rtnl_lock(), and ave_init() -> of_mdiobus_register() reaches the same recursion, so those boards hang on probe at the intermediate commit.

Would it make sense to squash this patch (and the never-needed exported phy_device_register_locked() API that it removes again) into the patch that introduced the rtnl_lock()?

Quoted 1 line

so any such driver (lantiq_etop, sni_ave, netsec) deadlocks on probe.

[Severity: Low]
Does netsec belong in that list? netsec_netdev_init(), its .ndo_init, only allocates the TX/RX descriptor rings and resets the hardware; the MDIO bus is registered from netsec_probe() via netsec_register_mdio(), before register_netdev() is called, so phy_device_register() runs without rtnl held there. The lantiq_etop and sni_ave paths do match the described recursion.

Quoted 4 lines
drivers/net/phy/phy_device.c +0 -0
index f5febff4b00b8..e8d894bbfa7aa 100644

[ ... ]

Quoted 9 lines
drivers/net/phy/phy_device.c +1 -2
@@ -1181,16 +1181,16 @@ static int phy_pse_notifier_event(struct notifier_block *nb,
 {
 	switch (event) {
 	case PSE_REGISTERED:
-		rtnl_lock();
+		pse_phy_lock();
 		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
 				 phy_pse_attach_one);
-		rtnl_unlock();

drivers/net/phy/phy_device.c static int phy_pse_notifier_event(struct notifier_block *nb, · line 1194

Quoted 10 lines
drivers/net/phy/phy_device.c +3 -2
+		pse_phy_unlock();
 		return NOTIFY_OK;
 	case PSE_UNREGISTERED:
-		rtnl_lock();
+		pse_phy_lock();
 		bus_for_each_dev(&mdio_bus_type, NULL, data,
 				 phy_pse_detach_one);
-		rtnl_unlock();
+		pse_phy_unlock();
 		return NOTIFY_OK;

[Severity: High]
Can a phy that has already left mdio_bus_type keep its pse_control past the controller teardown?

The detach walk only reaches devices still on the bus, but at this commit phy_device_remove() does not touch phydev->psec:

drivers/net/phy/phy_device.c:phy_device_remove() {
	unregister_mii_timestamper(phydev->mii_ts);
	device_del(&phydev->mdio.dev);
	...
}

and the put is deferred to the release callback, without the new lock:

drivers/net/phy/phy_device.c:phy_device_release() {
	...
	might_sleep();
	pse_control_put(phydev->psec);
	...
}

So a phy that has been device_del()'d but is still pinned (an attached netdev holding a reference, or an SFP-module phy waiting for phy_device_free()) is invisible to the PSE_UNREGISTERED walk and keeps its psec across pse_controller_unregister(). That function then frees the pi array:

drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);
	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);		/* kfree(pcdev->pi) */
	...
}

and the later put reaches __pse_control_release(), which does
"if (psec->pcdev->pi[psec->id].admin_state_enabled) regulator_disable(psec->ps);" on that freed memory. This also runs outside pse_phy_lock(), which the new kernel-doc says every phydev->psec access must hold.

The last patch of the series ("net: phy: release phydev->psec from phy_device_remove() again") does move the put into phy_device_remove() under pse_phy_lock() and strips it from phy_device_release(), so the end state is fine; the concern is only about this intermediate commit, which is also the one that documents the locking contract.

Quoted 1 line
drivers/net/phy/phy_device.c +0 -0
@@ -1201,15 +1201,22 @@ static struct notifier_block phy_pse_notifier __read_mostly = {

[ ... ]

Quoted 43 lines
drivers/net/phy/phy_device.c +5 -24
+int phy_device_register(struct phy_device *phydev)
 {
 	int err;
@@ -1233,6 +1240,10 @@ static int __phy_device_register(struct phy_device *phydev)
 		goto out;
 	}
 
+	pse_phy_lock();
+	phy_try_attach_pse(phydev);
+	pse_phy_unlock();
+
 	return 0;
 
  out:
@@ -1241,51 +1252,6 @@ static int __phy_device_register(struct phy_device *phydev)
 	mdiobus_unregister_device(&phydev->mdio);
 	return err;
 }
-
-/**
- * phy_device_register_locked - Register the phy device on the MDIO bus
- * @phydev: phy_device structure to be added to the MDIO bus
- *
- * Same as phy_device_register() but caller must already hold rtnl_lock().
- *
- * Return: 0 on success, negative error code on failure.
- */
-int phy_device_register_locked(struct phy_device *phydev)
-{
-	int err;
-
-	ASSERT_RTNL();
-
-	err = __phy_device_register(phydev);
-	if (err)
-		return err;
-
-	phy_try_attach_pse(phydev);
-
-	return 0;
-}
-EXPORT_SYMBOL(phy_device_register_locked);

[ ... ]

Quoted 1 line

EXPORT_SYMBOL(phy_device_register);

[ ... ]

Quoted 10 lines
drivers/net/pse-pd/pse_core.c +2 -0
index 611540df43bb1..b771f5a2a83fc 100644
@@ -24,8 +24,54 @@ static LIST_HEAD(pse_controller_list);
 static DEFINE_XARRAY_ALLOC(pse_pw_d_map);
 static DEFINE_MUTEX(pse_pw_d_mutex);
 
+/* Serialises phydev->psec against the PSE controller lifecycle notifier and
+ * the ethtool PSE paths, in place of rtnl. The attach must not take rtnl: an

[ ... ]

Quoted 7 lines
drivers/net/pse-pd/pse_core.c +5 -0
+static DEFINE_MUTEX(pse_phy_mutex);
+
 static BLOCKING_NOTIFIER_HEAD(pse_controller_notifier);
 
+/**
+ * pse_phy_lock - hold phydev->psec stable against PSE controller teardown
+ *

drivers/net/pse-pd/pse_core.c static LIST_HEAD(pse_controller_list); · line 53

Quoted 10 lines
drivers/net/pse-pd/pse_core.c +10 -0
+ * The PSE_UNREGISTERED notifier clears phydev->psec and drops the last
+ * reference on the pse_control before the controller frees its state. Callers
+ * that attach, detach or dereference phydev->psec must hold this lock across
+ * the whole access so the detach cannot run underneath them.
+ */
+void pse_phy_lock(void)
+{
+	mutex_lock(&pse_phy_mutex);
+}
+EXPORT_SYMBOL_GPL(pse_phy_lock);

[Severity: High]
Does this lock actually cover the teardown it claims to protect against?

phy_pse_notifier_event() drops pse_phy_mutex as soon as the detach walk returns, and pse_controller_unregister() does the rest of the teardown with the mutex no longer held:

drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);
	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);		/* kfree(pcdev->pi) */
	if (pcdev->irq)
		disable_irq(pcdev->irq);
	cancel_work_sync(&pcdev->ntf_work);
	kfifo_free(&pcdev->ntf_fifo);
	mutex_lock(&pse_list_mutex);
	list_del(&pcdev->list);
	mutex_unlock(&pse_list_mutex);
}

The controller stays on pse_controller_list until the very end, and the window spans cancel_work_sync(), which can block for a queued notification work item.

Can a concurrent phy_device_register() -> pse_phy_lock() -> phy_try_attach_pse() -> of_pse_control_get() land in that window? of_pse_control_get() walks pse_controller_list under pse_list_mutex only, with no unregistering-state check, so of_pse_match_pi() reads the freed pcdev->pi[i].np and pse_control_get_internal() then writes pcdev->pi[index].admin_state_enabled and dereferences pcdev->pi[index].rdev.

The pse_control created that way can also never be detached, because the PSE_UNREGISTERED walk has already finished, so its eventual put re-enters __pse_control_release() on the freed pi array.

Would moving the list_del() ahead of pse_release_pis(), or holding pse_phy_mutex across the whole of pse_controller_unregister(), close this? The list_del-last ordering predates the series, but the changelog states the new mutex protects "the phy<->controller binding against PSE controller teardown", and the series adds both new attach sites, so the case seems worth addressing here. This is still present at the tip of the series.

[ ... ]

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
#9 lore raw

PATCH v6 5/5 net: phy: release phydev->psec from phy_device_remove() again

In reply to Carlo Szelinsky

"net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook" deferred the final pse_control_put() of phydev->psec from phy_device_remove() to phy_device_release(), so it would run only after the PSE_UNREGISTERED notifier walk had dropped its bus-iterator reference on the phy. But bus_for_each_dev() only reaches phys still on the mdio_bus_type klist: a phy that has been device_del()'d yet is still pinned (e.g. by an attached netdev) is invisible to the walk, so phy_pse_detach_one() never clears its phydev->psec. Its deferred put then runs after pse_controller_unregister() -> pse_release_pis() has freed pcdev->pi[], and __pse_control_release() dereferences the freed array:
use-after-free.

Put phydev->psec back in phy_device_remove(), before device_del(), so the detach is synchronous and ordered ahead of the phy leaving the bus; it can no longer outlive the PSE controller. "net: phy: use a dedicated mutex instead of rtnl for PSE control attach" replaced rtnl with pse_phy_lock() for the attach/detach, so this put can take that same lock without the rtnl recursion that originally motivated the deferral, and it serialises against the notifier walk: whichever runs first clears phydev->psec, the other sees NULL.

---
 drivers/net/phy/phy_device.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)
drivers/net/phy/phy_device.c +11 -12
index e8d894bbfa7a..bb120045c406 100644
@@ -223,19 +223,8 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
 
 static void phy_device_release(struct device *dev)
 {
-	struct phy_device *phydev = to_phy_device(dev);
-
-	/* bus_for_each_dev() holds get_device() across each iteration
-	 * step, deferring this release callback until any in-flight PSE
-	 * notifier walk has advanced past this phy. pse_control_put()
-	 * takes pse_list_mutex, so this path must run in sleepable
-	 * context.
-	 */
-	might_sleep();
-	pse_control_put(phydev->psec);
-
 	fwnode_handle_put(dev->fwnode);
-	kfree(phydev);
+	kfree(to_phy_device(dev));
 }
 
 static void phy_mdio_device_remove(struct mdio_device *mdiodev)
@@ -1265,6 +1254,16 @@ EXPORT_SYMBOL(phy_device_register);
 void phy_device_remove(struct phy_device *phydev)
 {
 	unregister_mii_timestamper(phydev->mii_ts);
+
+	/* Detach synchronously, before the phy leaves the bus, so the put cannot
+	 * outlive the PSE controller (an off-bus but still-pinned phy is missed by
+	 * the PSE_UNREGISTERED walk). pse_phy_lock() serialises against that walk.
+	 */
+	pse_phy_lock();
+	pse_control_put(phydev->psec);
+	phydev->psec = NULL;
+	pse_phy_unlock();
+
 	device_del(&phydev->mdio.dev);
 
 	/* Assert the reset signal */
Signature · 1 line
2.43.0
#10 lore raw

Re: [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again

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

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

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

- [High] The commit claims that after this change phydev->psec "can no longer outlive the PSE controller", but the same freed-array sink…
- [High] phy_device_remove() now drops phydev->psec and releases pse_phy_lock() *before* device_del(), i.e. before the phy leaves the…
- [Medium] In-series regression fixed by a later patch of the same series, with no attribution.
- [Medium] Memory leak of pse_control on the error path of phy_device_register() if device_add() fails.

Pre-existing issues:
- [High] phylib clears only the forward link on detach (phydev->psec = NULL in phy_pse_detach_one() and in the newly added block in…

--

net: phy: release phydev->psec from phy_device_remove() again

This moves the final pse_control_put() of phydev->psec out of phy_device_release() and back into phy_device_remove(), where it now runs under pse_phy_lock() just before device_del(). The stated reason is that a device_del()'d but still pinned phy is invisible to the PSE_UNREGISTERED bus walk, so a deferred put could outlive the PSE controller.

A few questions below.

Quoted 13 lines
drivers/net/phy/phy_device.c +0 -5
index e8d894bbfa7aa..bb120045c406e 100644
@@ -223,19 +223,8 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
 
 static void phy_device_release(struct device *dev)
 {
-	struct phy_device *phydev = to_phy_device(dev);
-
-	/* bus_for_each_dev() holds get_device() across each iteration
-	 * step, deferring this release callback until any in-flight PSE
-	 * notifier walk has advanced past this phy. pse_control_put()

drivers/net/phy/phy_device.c static void phy_mdio_device_free(struct mdio_device *mdiodev) · line 228

Quoted 10 lines
drivers/net/phy/phy_device.c +1 -7
-	 * takes pse_list_mutex, so this path must run in sleepable
-	 * context.
-	 */
-	might_sleep();
-	pse_control_put(phydev->psec);
-
 	fwnode_handle_put(dev->fwnode);
-	kfree(phydev);
+	kfree(to_phy_device(dev));
 }

[Severity: Medium]
With the put gone from the release callback, phy_device_remove() is the only place phylib ever drops phydev->psec. Do the error paths of phy_device_register() still reach it?

As far as I can tell they do not: if device_add() fails, phy_device_register() logs "failed to add", jumps to out:, asserts the reset and calls mdiobus_unregister_device(), then returns the error to the caller, which typically just phy_device_free()s the phy. phy_device_remove() is never invoked, and phy_device_release() is now only fwnode_handle_put() plus kfree().

The window matters because device_add() puts the device on the mdio_bus_type klist in bus_add_device() before dpm_sysfs_add() and the remaining steps, any of which can still fail and roll back. While the phy sits on that klist a concurrent PSE_REGISTERED walk (bus_for_each_dev(&mdio_bus_type, ..., phy_pse_attach_one) under pse_phy_lock()) can see it, and phy_try_attach_pse() will set phydev->psec via of_pse_control_get() because psec is still NULL. On the failure return nothing puts that reference: the struct pse_control stays on pcdev->pse_control_head with attached_phydev pointing at memory that is about to be kfree()d, and the try_module_get(pcdev->owner) taken in pse_control_get_internal() is never dropped, pinning the PSE driver module for the lifetime of the system. Keeping the put in phy_device_release() happened to cover this case for free.

This is a cold path, but could the same locked put be factored into a small helper (say phy_pse_detach(phydev)) and called from both phy_device_remove() and the out: label of phy_device_register()? Or do you consider a put in the release callback still warranted as a backstop for phys that never reach phy_device_remove() at all?

Related question: if phy_device_register() itself attaches the psec before any of its remaining failure points, the leak would not even need a concurrent notifier walk -- can you confirm the ordering there?

[Severity: Medium]
Is the use-after-free described in the changelog reachable in any released tree? In the baseline this series applies to, phy_device_remove() already contained:

	unregister_mii_timestamper(phydev->mii_ts);
	pse_control_put(phydev->psec);

and phy_device_release() had no PSE put at all. The deferral being undone here was added earlier in this same submission by "net: phy: own phydev->psec via PSE notifier and remove fwnode_mdio hook", whose own changelog says the put "moves from phy_device_remove() to phy_device_release()".

Should this hunk simply be folded into that patch instead of appearing as a separate commit? As posted, the two intermediate commits of the series carry the live use-after-free on pcdev->pi[] that this commit removes, so bisecting an unrelated phy or PSE problem into that window hits it.

The two sibling patches are referenced by quoted title only, with no SHA, no Fixes: tag and no statement that they belong to the same series. Could that be made explicit either way, so a maintainer or stable backporter can tell whether a Fixes:/backport is needed here? If the commits do stay separate, the introducing patch's changelog is left permanently documenting a deferral rationale that no longer matches the tree.

Quoted 5 lines
drivers/net/phy/phy_device.c +1 -0
@@ -1265,6 +1254,16 @@ EXPORT_SYMBOL(phy_device_register);
 void phy_device_remove(struct phy_device *phydev)
 {
 	unregister_mii_timestamper(phydev->mii_ts);
+

drivers/net/phy/phy_device.c EXPORT_SYMBOL(phy_device_register); · line 1267

Quoted 10 lines
drivers/net/phy/phy_device.c +9 -0
+	/* Detach synchronously, before the phy leaves the bus, so the put cannot
+	 * outlive the PSE controller (an off-bus but still-pinned phy is missed by
+	 * the PSE_UNREGISTERED walk). pse_phy_lock() serialises against that walk.
+	 */
+	pse_phy_lock();
+	pse_control_put(phydev->psec);
+	phydev->psec = NULL;
+	pse_phy_unlock();
+
 	device_del(&phydev->mdio.dev);

[Severity: High]
Can a concurrent PSE_REGISTERED walk re-attach a psec to this phy after the put has already run?

The lock is dropped here, and the phy only leaves the mdio_bus_type klist much later, inside device_del() -> bus_remove_device():

drivers/base/bus.c:bus_remove_device() {
	...
	if (klist_node_attached(&dev->p->knode_bus))
		klist_del(&dev->p->knode_bus);
	...
}

bus_for_each_dev() does not skip devices whose device_del() is in progress, so throughout kill_device(), BUS_NOTIFY_DEL_DEVICE, dpm_sysfs_remove() and the sysfs teardown the phy is still reachable from:

phy_pse_notifier_event() {
	case PSE_REGISTERED:
		pse_phy_lock();
		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
				 phy_pse_attach_one);
		pse_phy_unlock();
}

phy_pse_attach_one() -> phy_try_attach_pse() then sees dev->type == &mdio_bus_phy_type, of_node still set, and psec == NULL because this hunk just cleared it:

	if (phydev->psec)
		return;

	psec = of_pse_control_get(np, phydev);
	...
	phydev->psec = psec;

Which path releases that new reference? phy_pse_detach_one() can no longer see the phy once it is off the klist, the put in phy_device_remove() has already run, and phy_device_release() is now just fwnode_handle_put() plus kfree(). That looks like a leaked struct pse_control left on pcdev->pse_control_head with attached_phydev pointing at freed memory, plus the try_module_get(pcdev->owner) reference taken in pse_control_get_internal(), which would pin the PSE driver module for good.

The changelog says "whichever runs first clears phydev->psec, the other sees NULL". Does that hold for PSE_REGISTERED, which creates a reference rather than clearing one?

Would placing the locked put after device_del(), still synchronously in phy_device_remove(), close this? Then either the put wins and the walk cannot see the off-klist phy, or the walk wins and the put releases whatever it attached.

[Severity: High]
The changelog states the put "can no longer outlive the PSE controller". Is that accurate given the attach side?

pse_controller_unregister() fires the notifier, frees pcdev->pi[], and only then removes the controller from the list:

drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);

	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	...
	mutex_lock(&pse_list_mutex);
	list_del(&pcdev->list);
	mutex_unlock(&pse_list_mutex);
}

pse_release_pis() does kfree(pcdev->pi) without clearing pcdev->pi or pcdev->nr_lines. pse_phy_lock() is held only for the duration of the notifier walk inside phy_pse_notifier_event(), which ends before pse_release_pis() runs, so between the end of the walk and that list_del the controller is still resolvable:

phy_device_register() -> phy_try_attach_pse() -> of_pse_control_get()

of_pse_match_pi() then reads pcdev->pi[i].np out of the freed array, and pse_control_get_internal() reads and writes it:

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

The resulting psec lands in phydev->psec, and its eventual put reaches the same freed-array dereference the changelog says is eliminated:

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

The documented contract in include/linux/pse-pd/pse.h says subscribers "must not acquire new references for it" after PSE_UNREGISTERED, but nothing on the subscriber side can enforce that while the controller stays on pse_controller_list.

Would making the controller unresolvable before pse_release_pis() (list_del, or an unregistering flag tested under pse_list_mutex), and having pse_release_pis() NULL out pcdev->pi and zero nr_lines, be needed for the claim to hold?

[Severity: High]
This isn't a bug introduced by this patch, but only the forward link is cleared here; nothing in phylib ever invalidates the reverse link psec->attached_phydev. pse_core sets it once, and only when it allocates a fresh psec:

drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
	psec->attached_phydev = phydev;
}

An existing psec at the same PI index is merely kref_get()'d and the new phydev pointer discarded, so the back-pointer stays bound to whichever phy attached first and survives that phy's detach. It is dereferenced here:

drivers/net/pse-pd/pse_core.c:pse_control_get_netdev() {
	ASSERT_RTNL();

	if (!psec || !psec->attached_phydev)
		return NULL;

	return psec->attached_phydev->attached_dev;
}

The NULL check cannot detect a freed phy. pse_send_ntf_worker() takes a transient reference and then blocks:

	psec = pse_control_find_by_id(pcdev, ntf.id);
	rtnl_lock();
	netdev = pse_control_get_netdev(psec);

If the phy removal completes in that window (this put drops phylib's reference while the psec stays alive on the worker's kref, then phy_mdio_device_free() -> put_device() -> phy_device_release() -> kfree()), does the worker dereference the freed phy_device and hand a garbage net_device pointer to ethnl_pse_send_ntf()?

Same window existed with the put in phy_device_release() and in the baseline, so this is not a new problem, but should phylib also clear psec->attached_phydev on detach?

Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de
maintainer
#11 lore raw
In reply to Carlo Szelinsky
Quoted 44 lines · Carlo Szelinsky

This is v6 of Corey's series [1]. It takes the PSE controller lookup out of the MDIO probe path, so a modular PSE driver no longer makes the PHY/DSA probe spin on -EPROBE_DEFER until the PSE module loads.

Patches 1-3 are the same three notifier patches as v4 [4], unchanged, with Jonas's Tested-by. Patches 4 and 5 fix two problems the v4 review surfaced; v6 additionally fixes a build regression in v5 [7]'s patch 4.

Patch 4: Aleksander reported [5] that v4 deadlocks on probe for an MDIO bus registered from ndo_init (lantiq_etop, sni_ave, netsec): those already hold rtnl via register_netdevice(), and v4's phy attach took rtnl again underneath. Patch 4 swaps that rtnl for a dedicated mutex, so the register path no longer recurses. The ethtool PSE paths take the same mutex, so the use-after-free that rtnl used to close stays closed. The mutex lives in pse_core rather than phylib: net/ethtool is always built into vmlinux but PHYLIB is tristate, so with CONFIG_PHYLIB=m or =n a phylib export is unresolved (v5 failed to link there [8]); PSE_CONTROLLER is bool, so pse_core is always reachable.

Patch 5: Paolo's review [6] pointed out that patch 3 defers the pse_control_put() to phy_device_release(). A phy that is device_del()'d but still pinned (an attached netdev) is off the mdio_bus_type klist, so the PSE_UNREGISTERED notifier walk never clears its phydev->psec, and the deferred put later touches a pcdev->pi[] the controller has already freed. Patch 5 puts phydev->psec back in phy_device_remove(), which the mutex from patch 4 now makes safe (the rtnl recursion that motivated the deferral is gone), so the detach is synchronous and cannot outlive the controller.

How it works: pse_core gets a notifier chain (REGISTERED / UNREGISTERED). The phy layer subscribes, owns phydev->psec, and attaches the PSE handle when the controller shows up instead of during probe. fwnode_mdio loses its PSE awareness, so no -EPROBE_DEFER leaves it and the probe-retry loop is gone.

Tested on a Realtek rtl93xx PoE switch with two HS104 PSE controllers on i2c:

- clean boot, no probe-retry loop, no watchdog reset
- 10G SFP+ port: module hotplug works, no deadlock
- ethtool --set-pse enable/disable cuts and restores power to a PD
- i2c unbind -> rmmod -> modprobe: PSE detaches on unbind and re-attaches on reload with power restored, no reboot. No lockdep splats.

Quoted 4 lines

Jonas confirmed the RTL8214FC deadlock he reported is gone. Aleksander confirmed the lantiq_etop probe deadlock is gone at boot.

A bunch of 'high prio' sashiko reported issues are actually fixed by patch 5/5, so IMHO not very relevant. Still I think there are a few points to act upon.

@Carlo: please have a look at commit c82ff94592fb68f529afe63ca7f5ddb7dae4ba83: you are supposed to reply to LLM's comments.

/P

#12 lore raw

Hi Paolo,

Spoiler: this is heavily assisted by AI agents - without them a change this size would get too complicated for me to hold together. Everything in it is verified by me personally.

Thanks for the feedback, and for the pointer to c82ff94592fb - I had read the sashiko mails but hadn't replied, which is exactly what that commit says not to do. Won't happen again.

I ran v7 through https://sashiko.dev/ myself before posting, as that commit asks, and it was worth doing. It found a regression I had missed, and later passes found more. Those are covered below. Doing that first should also keep the volume down on your side.

You're right that most of the [High] ones are just the state of the tree between v6 patches 3 and 5. Instead of saying that five times, v7 folds v6 patches 3, 4 and 5 into one. The rtnl detour and the deferred release are then gone from every commit, so those findings no longer apply.

v7 follows this mail. It is still five patches, but not the same five - the review before posting found a regression that needed two more pse_core fixes. I go through everything below anyway, one block per patch. The numbering is v6's, to match the review mails.

1/5 - notifier chain
https://lore.kernel.org/netdev/[email protected]/

Quoted 1 line

[Low] exported with no in-tree caller at this commit

The review says itself this is the usual "add the API, then use it" split. Nothing to do.

2/5 - fire lifecycle events
https://lore.kernel.org/netdev/[email protected]/

Quoted 1 line

[Low] the call sites discard the return value

The first half is right, but not worth doing. There is one subscriber in tree, phy_pse_notifier_event(), and it returns NOTIFY_OK or NOTIFY_DONE, so nothing can stop the chain. A notifier_to_errno() check would test for something that cannot happen today.

The second half is worth doing. In v7, by the time cancel_work_sync() returns, every handle should be gone: the walk drops the phy's and the drain drops the worker's. So if pcdev->pse_control_head is not empty there, someone still holds one I did not account for. That is the bug I describe further down. v7 patch 2 adds that warning.

Quoted 1 line

[High] PSE_UNREGISTERED fires while pcdev is still on pse_controller_list

Real, and v7 closes it here rather than leaving it to my net series [1].

My first answer was going to be that the race isn't new. The old fwnode lookup ran from the same registration path, so it could wait for [1], whose patch 3 moves list_del() ahead of pse_release_pis().

That is true but it isn't the whole picture. After the phy patch, a second controller registering runs a PSE_REGISTERED walk that calls of_pse_control_get() for every phy on mdio_bus_type. So this series adds a second trigger, and it is the one a two-controller board hits. Mine is one.

So v7 patch 2 reorders pse_controller_unregister() around the event. disable_irq() and the unlink both move above it. of_pse_control_get() is the only thing that walks pse_controller_list, and subscribers get pcdev as the event data, so nothing needs the controller listed. A lookup can then no longer reach a controller whose pi[] is about to go.

cancel_work_sync() moves the other way, below the event. That is the next answer.

[1] still stands on its own for net and stable.

Quoted 1 line

[High, pre-existing] pi freed before the worker is drained

I had this filed as [1]'s problem. That was wrong.

Before this series the phy's reference outlived the whole teardown. The worker takes a transient reference through pse_control_find_by_id(), but it could never be the last one, so __pse_control_release() did not run during unregister at all.

The PSE_UNREGISTERED walk changes that. It drops the phy's reference, so the worker's put can be the last one. Left as it was, that put would land after pse_release_pis() had freed pcdev->pi. The window is much smaller than the one it replaces, but it is new, so it belongs in this series. The reorder below is what closes it.

The obvious fix is cancel_work_sync() ahead of the frees, which is what
[1] does for net. On its own that is not enough here, because the walk does more than drop references.

A subscriber dropping the last reference reaches __pse_control_release(), which calls regulator_disable() if the PI is still on. With the static budget strategy that retries any port on the same power domain waiting for power, and if the domain is still over budget it sheds a lower priority port through pse_disable_pi_pol(), which queues a notification and calls schedule_work(). So draining the worker before the walk leaves new work queued behind it, racing the kfifo_free() below.

Narrow - it needs tps23881 or another PSE_BUDGET_EVAL_STRAT_STATIC controller, over budget, with a port pending - but reachable.

So in v7 patch 2 cancel_work_sync() sits after the event, with the frees below both. The order is not the same as [1]'s. There the unlink sits below cancel_work_sync() and pse_flush_pw_ds(), which is fine because
[1] has no event to place. Here the event has to be after the unlink and before the frees, and cancel_work_sync() after the event, so the unlink moves to the top.

The two will conflict on the back-merge - I tried the merge, and pse_controller_unregister() comes out with two conflict hunks. The merged function wants v7's order, which contains [1]'s fix. The cover spells it out.

Kory reviewed [1]. This one turns on pse_pw_d_retry_power_delivery() re-entering from a release, so I would like him to check the placement here too.

[1] has been sitting at Changes Requested since August, I'll respin it.

3/5 - own phydev->psec via the notifier
https://lore.kernel.org/netdev/[email protected]/

Quoted 1 line

[Low] is the module unload part of this rationale the right way round?

No, it is the wrong way round. try_module_get() pins the provider for as long as a phy holds a handle, so rmmod is refused before the module exit path runs and the walk never runs at all. What the walk covers is driver unbind and device removal, and v7 says that instead.

Quoted 1 line

[Low] no Fixes: tag, no target tree

The target tree is in the subject line. On the Fixes: tag - 5e82147de1cb ("net: mdiobus: search for PSE nodes by parsing PHY nodes") is the commit to blame, but this is a refactor across two subsystems plus a new export, and tagging it invites a stable backport of all that to cure a probe-retry loop. Say the word if you want it anyway.

Quoted 2 lines

[Medium] is every error other than -ENOENT/-EPROBE_DEFER really a broken binding?

Fair point. pse_pi_is_hw_enabled() talks to the chip over i2c on tps23881, si3474, pd692x0 and realtek-pse-mcu, so -EIO is possible. Before this series that error came back out of fwnode_mdiobus_register_phy() and deferred probe retried it. Now the port ends up without PSE, and phy_try_attach_pse() logs it with phydev_warn() rather than dropping it silently. Patch 5 documents that.

For a genuinely transient i2c error I am not convinced a retry is worth it. That needs a work item and a give-up policy, in a series whose whole point is taking work out of the probe path.

Running v7 through sashiko.dev before posting turned up a worse case of the same thing, which I had missed and so had the review on the list. -EPROBE_DEFER here does not only mean "controller not registered yet".

Every PI regulator is registered with a "vpwr" supply, and the regulator core treats an unresolved supply at registration as non-fatal. So pse_controller_register() completes, PSE_REGISTERED fires, and regulator_get_exclusive() inside pse_control_get_internal() keeps returning -EPROBE_DEFER until the vpwr provider shows up. phylib has no event left to retry on, so the port loses PSE for good, and silently. Deferred probe on the MDIO bus used to recover exactly that.

So my "the notifier retries the latter at PSE_REGISTERED time" was wrong for that case. v7 patch 4 checks the PI's vpwr supply before registering anything, so the PSE driver's own probe defers and the ordering goes back where deferred probe can handle it.

That in turn makes -EPROBE_DEFER a routine return from pse_controller_register(), which has no error unwind at all. The notification kfifo, the PI array and an OF reference per described PI are leaked on every failure, and a partial pse_register_pw_ds() leaves devm-allocated power domains in the global xarray for the next registration to trip over.

Today those failures are terminal, so each leaks once. With the supply check the leak would repeat on every deferred-probe retry. So v7 also adds the unwind, as patch 3, ahead of the supply check.

It unwinds at two depths, which is worth flagging. 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 registration until devres unwinds the failed probe there are live regulators whose ops would follow a freed pointer. regulator_late_cleanup() and the "state" attribute both reach them.

The array is released on the failures that happen while it exists and before the first regulator does: setup_pi_matrix() and the supply check. Earlier than that there is nothing to release - pcdev->pi is still NULL, or of_load_pse_pis() has already freed it - and from the registration loop on it has to stay. An allocation failure in the loop's first iteration therefore leaks it too, where releasing would have been safe, but the rule stays simple. The supply check runs before the loop, so the ordinary deferral releases the array.

On an arm64 boot with a PI whose supply never appears, kmemleak is clean with that patch in place.

Unrelated thing I noticed while checking pse_control_get_internal():
two exits after a successful try_module_get() jump to free_psec instead of put_module - the missing pi_get_admin_state callback, and a failing pse_pi_is_hw_enabled(). Both leak a module reference. Pre-existing, and it has to go to net separately.

Quoted 3 lines

[High] deferred put vs the bus walk [High] does the detach walk close the UAF [Medium] worker as last holder

The first is the intermediate state that v6 patch 5 removes, and v7 folds away entirely. The second is the list_del ordering above. The third is the worker answer under 2/5 - it turned out to be this series' problem, not [1]'s.

4/5 - dedicated mutex instead of rtnl
https://lore.kernel.org/netdev/[email protected]/

Quoted 1 line

[Low] does netsec belong in that list?

No, netsec does not belong there. It brings its MDIO bus up in probe, before register_netdev(). ndo_init allocates the rings, power-cycles the phy over that bus and resets the hardware - which only works because the bus is already there. lantiq_etop and sni_ave do match. v7 drops netsec.

Quoted 2 lines

[Medium] the deadlock was introduced two patches earlier, series isn't bisectable

Folded for v7. I agree the UAF windows aren't worth restructuring for, but at v6 patch 3 alone lantiq_etop and sni_ave hang on probe, and that is a boot failure rather than a theoretical window. So v6 patches 4 and 5 go into 3, and the intermediate states are gone. Each of the five commits builds on its own.

Jonas's and Aleksander's tags are on the folded patch. Jonas's tag is also on patch 2, which did change in v7, and the cover says so.

Quoted 2 lines

[High] off-bus phy keeps its pse_control [High] does the mutex cover the teardown

Intermediate state again, and the list_del ordering again. The kernel-doc is fixed either way: the mutex serialises phydev->psec against the notifier walk, the phy attach and detach paths and the ethtool paths. It does not protect the controller teardown and no longer says it does.

5/5 - put phydev->psec back in phy_device_remove()
https://lore.kernel.org/netdev/[email protected]/

Quoted 2 lines

[Medium] in-series regression fixed by a later patch, with no attribution

Same answer as under 4/5. v7 folds v6 patches 3, 4 and 5 into one, so there is no intermediate regression left, and nothing to attribute.

Quoted 1 line

[Medium] do the error paths of phy_device_register() still reach it?

They don't. That is a real leak, and v7 fixes it.

device_add() puts the phy on the klist in bus_add_device() and can still fail after that, so a concurrent PSE_REGISTERED walk can attach a handle that nobody ever puts. The obvious fix is a locked put at the out:
label, since the unwind calls bus_remove_device() for us. That is exactly what makes it wrong: by then the phy is off the bus, so the PSE_UNREGISTERED walk misses it too, and a concurrent unregister can free pcdev->pi before the put reads it.

v7 instead sets phydev->psec_detached before device_add() and clears it only once registration has succeeded. No handle can be attached in that window, so there is nothing to release on the error path.

Quoted 1 line

[High] can PSE_REGISTERED re-attach after the put has run?

Yes, that one's real. The phy only comes off the mdio_bus_type klist in bus_remove_device(), which is a good way into device_del(), so between our unlock and that point the walk still sees it, psec is NULL again, and phy_try_attach_pse() re-binds it.

The suggested fix doesn't work though. With the put after device_del(), the phy is off the klist first, so an unregistering controller can walk past it, free pcdev->pi, and then our put hits the freed array in __pse_control_release() - which is the UAF this patch exists to fix. The put has to stay in front of device_del(): while the phy is still on the klist the walk and the put take the same mutex, so whoever is second sees NULL.

So v7 marks the phy as detached under the same lock and has phy_try_attach_pse() skip it. I did look at just holding pse_phy_lock() across device_del(), but that puts the PSE mutex above driver removal and the whole sysfs teardown, which seems like a lot for a window this small.

The flag is a plain bool, not another bit in the flags word. It would otherwise share a storage unit with suspended and sysfs_links, which are written under phydev->lock and rtnl while this one is written under pse_phy_lock().

Quoted 1 line

[High] "can no longer outlive the PSE controller" - accurate?

Not as flatly as v6 put it. v7 says what the patch actually gives you:
the phy's own reference goes away synchronously while it is still on the bus. With patch 2 unlinking the controller first, the controller side is closed too, so the claim holds for a different reason than the one v6 gave.

Quoted 1 line

[High, pre-existing] nothing clears psec->attached_phydev

Agreed. attached_phydev is only set when a fresh handle is allocated, an existing one at the same index is just kref_get()'d, and nothing ever clears it - so pse_send_ntf_worker() can end up dereferencing a freed phy_device.

Not something this series introduces, and it needs a pse_core helper to clear it under pse_list_mutex. It has to go to net separately, after this series, with:

Fixes fa2f0454174c ("net: pse-pd: Introduce attached_phydev to pse control")

So v7, which follows shortly, is five patches again, but not the same five:

- v6 patches 3, 4 and 5 folded into one, no intermediate rtnl or deferred release
- patch 2 now reorders pse_controller_unregister() around the event - unlink and disable_irq() before it, cancel_work_sync() after it, the frees last - closing both the lookup-versus-teardown race and the worker-as-last-holder one inside this series
- patch 2 also warns if pcdev->pse_control_head is not empty once the worker is drained
- new patch 3: unwind the kfifo and PI allocations when controller registration fails
- new patch 4: check the PI vpwr supply before registering the controller, so a consumer never meets a registered controller that can only answer -EPROBE_DEFER
- psec_detached is held across registration as well as removal, so the PSE_REGISTERED walk can't attach a handle nothing will free
- psec_detached 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
- netsec dropped, module-unload wording fixed, the probe-defer behaviour change documented
- kernel-doc no longer claims the mutex protects controller teardown
- <linux/notifier.h> included where struct notifier_block is used

One more thing turned up while testing patch 3 on a device tree where the vpwr provider never appears. pse_controller_register() calls kfifo_alloc() with pcdev->nr_lines before the "if (!nr_lines) nr_lines = 1" default below it, and __kfifo_alloc() rejects a size under two. So a controller with nr_lines of 0 or 1 cannot register at all, and that default is dead code. The three drivers with a compile-time constant cannot hit it; realtek-pse-mcu-core takes nr_lines from the device at runtime, so it is the one that could. Pre-existing, one line to fix, and it has to go to net separately.

So, separately against net: the leaked module ref, the attached_phydev back-pointer, that kfifo ordering, and a respin of [1].

[1] https://lore.kernel.org/netdev/[email protected]/

Thanks,
Carlo