Re: [PATCH net 2/2] net: pse-pd: guard against freed PI data on regulator disable

[Carlo Szelinsky <github@szelinsky.de>]

Hi Simon,

Thanks for forwarding this... I'd missed it the first time round.
Both points are valid; answering inline.

> [High]
> Is the pcdev->pi = NULL store correctly synchronized with the readers?
> ...
> Would taking pcdev->lock around the kfree()+NULL store in
> pse_release_pis() (so the NULL observation under pcdev->lock is
> authoritative) close that window? Alternatively, can the regulator
> unregister be ordered to run before pse_release_pis() so no consumer
> can invoke a regulator op while the PI array is being torn down?

Agreed, the guard is not authoritative as it stands. For v2 I'd take
pcdev->lock around the kfree() + pcdev->pi = NULL in pse_release_pis(),
so the NULL observed under the lock is authoritative.

I leaned away from reordering the regulator unregister, because both
the PI regulators and their consumer are devm-bound to pcdev->dev
(devm_regulator_register() and devm_regulator_get_exclusive()), so
reordering means tearing the regulators down by hand in
pse_controller_unregister() instead of letting devres do it, heavier
than a net fix wants. Does the contained fix (lock around the free)
work for you, or would you rather see the reorder?

One thing I'd like your read on for the commit message: the consumer
get is exclusive on pcdev->dev, so I couldn't pin down a concrete
external consumer that calls a regulator op on another CPU during
unbind. Do you have a specific path in mind, or should I describe the
lock as closing a narrow theoretical window rather than a proven race?
I'll add it either way.. I just want to make sure we are aligned :-)

> [High]
> Should the same NULL guard be applied to pse_pi_is_enabled() and
> pse_pi_enable()?
> ...
> Should these two callbacks receive the same guard, or alternatively
> should the unwind order be changed so that no callback can fire after
> the PI array is freed?

Yes. Both follow the same rdev_get_drvdata() -> mutex_lock() ->
unconditional pcdev->pi[id] pattern as the disable path, so for v2 I'll
add the same !pcdev->pi guard to pse_pi_is_enabled() and
pse_pi_enable().

Thanks,
Carlo