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

[Kory Maincent]

On Wed, 12 Aug 2026 16:59:36 +0300
Carlo Szelinsky <github@xxxxxxxxxxxx> wrote:

> On Mon, 10 Aug 2026 13:52:33 -0700 Jakub Kicinski wrote:
> > It'd be great if we can figure out the safe lifetime rules for pse-pd
> > from all entry points. Specifically the sysfs complaint on patch 1
> > (assuming it's real) makes the series seem like a bit of of a see-saw.
> >
> > Let's get it reposted and take it from there?  
> 
> Thanks for the reviews, Kory.
> 
> Makes sense. Let me go through everything that still touches pcdev->pi
> once pse_controller_unregister() has started freeing it, so it's clear
> what the series covers and what it doesn't.
> 
> The regulator ops (pse_pi_enable/disable/is_enabled) are the sysfs case.
> It's real: the PI regulators get registered before the controller, so on
> unbind the controller goes down first and pse_release_pis() frees
> pcdev->pi while the regulators are still live. is_enabled() is
> reachable over sysfs. Patch 2 is what handles this - it checks !pcdev->pi
> in those three ops and frees under the same lock. Patch 1 just does the
> reorder, so the two really need to be read together.
> 
> The other one is of_pse_match_pi() from of_pse_control_get(). A consumer
> probing at the same time walks pse_controller_list and reads
> pcdev->pi[i].np. Problem is we free pi before list_del(), so the
> controller is still on the list with pi already gone. The series doesn't
> touch this yet. It's a one-liner really - move list_del() before the
> free. Then a lookup either sees a valid pi or doesn't find the controller
> at all, no NULL checks anywhere. I'd add that as a patch 3.
> 
> Last two are pse_ethtool_get_status() and __pse_control_release(), from a
> consumer still holding a pse_control. They read pcdev->pi[psec->id] too,
> but here the real issue isn't pi, it's that the consumer can outlive the
> whole controller - of_pse_control_get() only takes a module ref, no
> device link - so a NULL check buys you nothing. That's pre-existing and
> not really something to fix in net. The notifier series I have for
> net-next [1] is what deals with it: the phy layer drops phydev->psec on
> unregister before pi is freed, and phy is the only pse_control user we
> have in tree.
> 
> So for v3: patches 1 and 2 as they are with Kory's Reviewed-by, plus the
> small patch 3 for the list_del reorder, and I'll write all this up in the
> cover letter so the ethtool side is clearly left to [1].
> 
> Sound ok before I send it out?

This sound good to me.

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