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

[Carlo Szelinsky]

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

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

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

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

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

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

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

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

Signed-off-by: Carlo Szelinsky <github@xxxxxxxxxxxx>
---
 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