Netdev List
 help / color / mirror / Atom feed
From: Carlo Szelinsky <github@szelinsky.de>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Kory Maincent <kory.maincent@bootlin.com>,
	Simon Horman <horms@kernel.org>,
	Oleksij Rempel <o.rempel@pengutronix.de>,
	Andrew Lunn <andrew+netdev@lunn.ch>,
	"David S . Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	Carlo Szelinsky <github@szelinsky.de>
Subject: Re: [PATCH net v2 2/2] net: pse-pd: guard regulator ops against freed PI data during unregister
Date: Wed, 12 Aug 2026 16:59:36 +0300	[thread overview]
Message-ID: <20260812135936.889119-1-github@szelinsky.de> (raw)
In-Reply-To: <20260810135233.4be3dc5d@kernel.org>

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?

[1] https://lore.kernel.org/netdev/20260630091125.3162481-1-github@szelinsky.de/

Thanks,
Carlo

      reply	other threads:[~2026-08-12 13:59 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-11 12:16 [PATCH net v2 0/2] net: pse-pd: fix use-after-free of PI array on controller unregister Carlo Szelinsky
2026-07-11 12:16 ` [PATCH net v2 1/2] net: pse-pd: stop async event sources before freeing PI data in unregister Carlo Szelinsky
2026-07-17 11:07   ` Simon Horman
2026-08-10 13:19     ` Kory Maincent
2026-07-11 12:16 ` [PATCH net v2 2/2] net: pse-pd: guard regulator ops against freed PI data during unregister Carlo Szelinsky
2026-07-17 11:10   ` Simon Horman
2026-08-10 13:43     ` Kory Maincent
2026-08-10 20:52       ` Jakub Kicinski
2026-08-12 13:59         ` Carlo Szelinsky [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260812135936.889119-1-github@szelinsky.de \
    --to=github@szelinsky.de \
    --cc=andrew+netdev@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=horms@kernel.org \
    --cc=kory.maincent@bootlin.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=o.rempel@pengutronix.de \
    --cc=pabeni@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox