Netdev List
 help / color / mirror / Atom feed
* [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails
  2026-10-04 16:42 [PATCH net-next v8 0/7] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
@ 2026-10-04 16:42 ` Carlo Szelinsky
  2026-10-05 17:33   ` netdev-bot+sashiko
  2026-10-13  9:39   ` Kory Maincent
  0 siblings, 2 replies; 5+ messages in thread
From: Carlo Szelinsky @ 2026-10-04 16:42 UTC (permalink / raw)
  To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
	Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Saravana Kannan
  Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
	Aleksander Jan Bajkowski, Mark Brown, Liam Girdwood,
	netdev-bot+sashiko, devicetree, netdev, linux-kernel,
	Carlo Szelinsky

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

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

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

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

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

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

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

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

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

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails
  2026-10-04 16:42 ` [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails Carlo Szelinsky
@ 2026-10-05 17:33   ` netdev-bot+sashiko
  2026-10-17 22:46     ` Carlo Szelinsky
  2026-10-13  9:39   ` Kory Maincent
  1 sibling, 1 reply; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-10-05 17:33 UTC (permalink / raw)
  To: github
  Cc: o.rempel, kory.maincent, andrew+netdev, hkallweit1, linux, davem,
	edumazet, kuba, pabeni, robh, saravanak, corey, jelonek.jonas,
	horms, olek2, broonie, lgirdwood, netdev-bot+sashiko, devicetree,
	netdev, linux-kernel

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

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

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

--

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

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

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

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

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

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

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

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

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

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

	switch (pi->pw_d->budget_eval_strategy) {

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

[ ... ]

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

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

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

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

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

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

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

[ ... ]

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

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

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

The commit message says:

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

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

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

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

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

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

out:
	mutex_unlock(&pse_pw_d_mutex);
	return ret;

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

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

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

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

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

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

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

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

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

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

[ ... ]

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails
  2026-10-04 16:42 ` [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails Carlo Szelinsky
  2026-10-05 17:33   ` netdev-bot+sashiko
@ 2026-10-13  9:39   ` Kory Maincent
  1 sibling, 0 replies; 5+ messages in thread
From: Kory Maincent @ 2026-10-13  9:39 UTC (permalink / raw)
  To: Carlo Szelinsky
  Cc: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
	Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Saravana Kannan, Corey Leavitt,
	Jonas Jelonek, Simon Horman, Aleksander Jan Bajkowski, Mark Brown,
	Liam Girdwood, netdev-bot+sashiko, devicetree, netdev,
	linux-kernel

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

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

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails
  2026-10-05 17:33   ` netdev-bot+sashiko
@ 2026-10-17 22:46     ` Carlo Szelinsky
  0 siblings, 0 replies; 5+ messages in thread
From: Carlo Szelinsky @ 2026-10-17 22:46 UTC (permalink / raw)
  To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
	Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Saravana Kannan
  Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
	Aleksander Jan Bajkowski, Mark Brown, Liam Girdwood,
	netdev-bot+sashiko, sashiko-bot, sashiko-reviews, devicetree,
	Kyle Swenson, netdev, linux-kernel

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

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

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

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

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

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

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

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

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

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

Carlo

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails
       [not found] <20261005164306.0BDF21F000FF@smtp.kernel.org>
@ 2026-10-17 22:47 ` Carlo Szelinsky
  0 siblings, 0 replies; 5+ messages in thread
From: Carlo Szelinsky @ 2026-10-17 22:47 UTC (permalink / raw)
  To: Oleksij Rempel, Kory Maincent, Andrew Lunn, Heiner Kallweit,
	Russell King, David S . Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Rob Herring, Saravana Kannan
  Cc: Corey Leavitt, Jonas Jelonek, Simon Horman,
	Aleksander Jan Bajkowski, Mark Brown, Liam Girdwood,
	netdev-bot+sashiko, sashiko-bot, sashiko-reviews, devicetree,
	Kyle Swenson, netdev, linux-kernel

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

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

Carlo

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-10-17 22:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20261005164306.0BDF21F000FF@smtp.kernel.org>
2026-10-17 22:47 ` [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails Carlo Szelinsky
2026-10-04 16:42 [PATCH net-next v8 0/7] net: pse-pd: decouple controller lookup from MDIO probe Carlo Szelinsky
2026-10-04 16:42 ` [PATCH net-next v8 3/7] net: pse-pd: unwind allocations when controller registration fails Carlo Szelinsky
2026-10-05 17:33   ` netdev-bot+sashiko
2026-10-17 22:46     ` Carlo Szelinsky
2026-10-13  9:39   ` Kory Maincent

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox