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

[Kory Maincent]

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

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

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