Re: [PATCH net v3 2/3] net: pse-pd: guard regulator ops against freed PI data during unregister

Thanks for running this through review, Jakub.

Most of the points fall into two buckets: the consumer-outlives-controller
race that I'm already leaving to the notifier series, and pre-existing bugs
that aren't really about this teardown fix. Let me go through them.

> [Severity: High]
> Now that pcdev->pi can legitimately be NULL, do the other readers of
> pcdev->pi in this file need the same guard?  Only the three regulator ops
> were updated, but the consumer-facing paths still index the array
> unconditionally:
[...]
> So after unbinding the PSE controller driver, an ethtool PSE request on
> the still registered interface, or a later PHY removal, would reach these
> reads with pcdev->pi == NULL.

Right. This is the case where a phy keeps its pse_control across driver
unbind - of_pse_control_get() only takes a module ref, no device link - so
ethtool and __pse_control_release() can still land here after pcdev->pi is
gone. Adding !pcdev->pi checks to all these paths doesn't really fix it, the
handle itself is stale by then.

So I'd leave these paths as they are here and not add guards. The notifier
series [1] fixes it properly: on PSE_UNREGISTERED the phy layer drops
phydev->psec before pse_release_pis() frees anything, and phy is the only
pse_control user in tree.

> [Severity: High]
> This isn't a bug introduced by this patch, but the ordering in
> pse_controller_unregister() looks like it leaves part of the window the
> commit message claims to close still open:
[...]
> pse_flush_pw_ds() drops the power-domain kref, and __pse_pw_d_release()
> calls regulator_put(pw_d->supply) [...] the new guards pass and the ops
> proceed to the released supply

Good catch, it's real. But it needs the same racing consumer to hit it -
something has to call into the regulator ops while unregister sits between
pse_flush_pw_ds() and the free. The notifier series closes this one too: it
fires PSE_UNREGISTERED at the top of unregister, so every consumer is gone
before pse_flush_pw_ds() drops the supply. Same bucket as above.

If you'd rather net not carry that window until [1] lands, I can add a small
stopgap - NULL pcdev->pi[i].pw_d in pse_flush_pw_ds() under pcdev->lock, so
pse_pi_deallocate_pw_budget()'s existing !pi->pw_d check catches it. Happy to
do either. Makes sense?

> [Severity: High]
> This isn't a bug introduced by this patch, but since these lines are being
> reworked anyway: should ret be returned here instead of 0?

Yes, that's a real one, but it's pre-existing and not about the UAF -
pse_pi_disable() has always swallowed the disable error. I'd send that as its
own patch with its own Fixes tag rather than bury it in here. Correct?

> [Severity: High]
> One more pre-existing item [...] pse_controller_register() [...] there is
> no error unwinding after the early allocations. [...] devres then frees the
> pw_d while the xarray still points at it

Also real, also pre-existing, and it's in the register path, not the teardown
one this series is about. The dangling xarray entry is the nasty part. I'd
recommend we handle that as its own patch rather than in this series - ok?

> [Severity: Medium]
> Also, pse_pi_enable() returns -ENODEV for the same condition while this one
> returns 0.  Is the asymmetry intended?

I understood it like this: a disable that races teardown has nothing left to
turn off, so returning "done" keeps the regulator core from warning, while
enable really can't proceed so -ENODEV made sense there. If that's right I'd
add a one-line comment so it doesn't read like an oversight. Makes sense to
you?

So, short version: I'd keep patches 1-3 as they are, leave the consumer
lifetime stuff and the supply window to the notifier series [1], and send the
two pre-existing bugs on their own.

Does that sound right to you, or would you do it differently? What would you
recommend as the next step?

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

Thanks,
Carlo