[PATCH net-next v2 0/2] ptp: safely cleanup when unregistering a PTP clock

[PATCH net-next v2 0/2] ptp: safely cleanup when unregistering a PTP clock

[Russell King (Oracle)]

(I'm assuming drivers/ptp/ patches go via net-next as there is no
git tree against the "PTP HARDWARE CLOCK SUPPORT" maintainers entry.)

The standard rule in the kernel for unregistering user visible devices
is to unpublish the userspace API before doing any shutdown of the
resources necessary for the operation of the device.

PTP has several issues in this area:

1. ptp_clock_unregister() cancells and destroys work while the PTP
   chardev is still published, which gives the opportunity for a
   precisely timed user API call to cause a driver to attempt to
   queue the aux work.

2. PTP pins are not cleaned up - if userspace has enabled PTP pins,
   e.g. for extts, drivers are forced to do cleanup before calling
   ptp_clock_unregister() to stop events being forwarded into the
   PTP layer. E.g mv88e6xxx cancells its internal tai_event_work
   to avoid calling into the PTP clock code with a stale ptp_clock
   pointer, but a badly timed userspace EXTTS enable will re-schedule
   the tai_event_work.

Simplify the process by ensuring that:

1. we take a referene on the PTP struct device to stop the
   ptp_clock structure going away underneath us when we call
   posix_clock_unregister().

2. call posix_clock_unregister() to remove the /dev/ptp* device.

3. add additional functionality to disable any PTP EXTTS pins and
   PPS event generation that have been configured on this device.
   This should shutdown all events coming from PTP clock drivers.

4. cancel the delayed aux_work and destroy the kthread.

5. remove the PPS source.

6. drop the reference on the PTP struct device to allow the
   ptp_clock structure to be released.

This is difficult for me to test beyond build testing - on the
Clearfog platform with Marvell PHY PTP, the ethernet PHY is the
primary connectivity, so removing the PHY driver for an in-use
network interface isn't possible.

On the ZII rev B platform, where the DSA switches have the TAI
hardware and where root NFS is used, removal of the DSA switch
module somehow forces the FEC interface _not_ connected to the DSA
switch to lose link, causing the machine to become unresponsive
as its root filesystem vanishes.

---
v2:
- add r-b's to patch 1
- only disable EXTTS pins
- disable PPS event generation

 drivers/ptp/ptp_chardev.c | 28 +++++++++++++++++++++++++++-
 drivers/ptp/ptp_clock.c   | 15 ++++++++++++++-
 drivers/ptp/ptp_private.h |  2 ++
 3 files changed, 43 insertions(+), 2 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next v2 1/2] ptp: describe the two disables in ptp_set_pinfunc()

[Russell King (Oracle)]

Accurately describe what each call to ptp_disable_pinfunc() is doing,
rather than the misleading comment above the first disable. This helps
to make the code more readable.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/ptp/ptp_chardev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index e9719f365aab..eb4f6d1b1460 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -91,12 +91,18 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		return -EOPNOTSUPP;
 	}
 
-	/* Disable whatever function was previously assigned. */
+	/* Disable whichever pin was previously assigned to this function and
+	 * channel.
+	 */
 	if (pin1) {
 		ptp_disable_pinfunc(info, func, chan);
 		pin1->func = PTP_PF_NONE;
 		pin1->chan = 0;
 	}
+
+	/* Disable whatever function was previously assigned to the requested
+	 * pin.
+	 */
 	ptp_disable_pinfunc(info, pin2->func, pin2->chan);
 	pin2->func = func;
 	pin2->chan = chan;
-- 
2.47.3

[PATCH net-next v2 2/2] ptp: rework ptp_clock_unregister() to disable events

[Russell King (Oracle)]

The ordering of ptp_clock_unregister() is not ideal, as the chardev
remains published while state is being torn down, which means userspace
can race with the kernel teardown. There is also no cleanup of enabled
pin settings nor of the internal PPS event, which means enabled events
can still forward into the core, dereferencing a free'd pointer.

Rework the ordering of cleanup in ptp_clock_unregister() so that we
unpublish the posix clock (and user chardev), disable any pins that
have EXTTS events enabled, disable the PPS event, and then clean up
the aux work and PPS source.

This avoids potential use-after-free and races in PTP clock driver
teardown.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/ptp/ptp_chardev.c | 20 ++++++++++++++++++++
 drivers/ptp/ptp_clock.c   | 15 ++++++++++++++-
 drivers/ptp/ptp_private.h |  2 ++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c
index eb4f6d1b1460..8106eb617c8c 100644
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -47,6 +47,26 @@ static int ptp_disable_pinfunc(struct ptp_clock_info *ops,
 	return err;
 }
 
+void ptp_disable_all_events(struct ptp_clock *ptp)
+{
+	struct ptp_clock_info *info = ptp->info;
+	unsigned int i;
+
+	mutex_lock(&ptp->pincfg_mux);
+	/* Disable any pins that may raise EXTTS events */
+	for (i = 0; i < info->n_pins; i++)
+		if (info->pin_config[i].func == PTP_PF_EXTTS)
+			ptp_disable_pinfunc(info, info->pin_config[i].func,
+					    info->pin_config[i].chan);
+
+	/* Disable the PPS event if the driver has PPS support */
+	if (info->pps) {
+		struct ptp_clock_request req = { .type = PTP_CLK_REQ_PPS };
+		info->enable(info, &req, 0);
+	}
+	mutex_unlock(&ptp->pincfg_mux);
+}
+
 int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		    enum ptp_pin_function func, unsigned int chan)
 {
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 1d920f8e20a8..ef020599b771 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -498,9 +498,21 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
 	}
 
+	/* Get the device to stop posix_clock_unregister() doing the last put
+	 * and freeing the structure(s)
+	 */
+	get_device(&ptp->dev);
+
+	/* Wake up any userspace waiting for an event. */
 	ptp->defunct = 1;
 	wake_up_interruptible(&ptp->tsev_wq);
 
+	/* Tear down the POSIX clock, which removes the user interface. */
+	posix_clock_unregister(&ptp->clock);
+
+	/* Disable all sources of event generation. */
+	ptp_disable_all_events(ptp);
+
 	if (ptp->kworker) {
 		kthread_cancel_delayed_work_sync(&ptp->aux_work);
 		kthread_destroy_worker(ptp->kworker);
@@ -510,7 +522,8 @@ int ptp_clock_unregister(struct ptp_clock *ptp)
 	if (ptp->pps_source)
 		pps_unregister_source(ptp->pps_source);
 
-	posix_clock_unregister(&ptp->clock);
+	/* The final put, normally here, will invoke ptp_clock_release(). */
+	put_device(&ptp->dev);
 
 	return 0;
 }
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index b352df4cd3f9..76ab9276b588 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -141,6 +141,8 @@ extern const struct class ptp_class;
  * see ptp_chardev.c
  */
 
+void ptp_disable_all_events(struct ptp_clock *ptp);
+
 /* caller must hold pincfg_mux */
 int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin,
 		    enum ptp_pin_function func, unsigned int chan);
-- 
2.47.3

Re: [PATCH net-next v2 2/2] ptp: rework ptp_clock_unregister() to disable events

[Vladimir Oltean]

On Tue, Sep 16, 2025 at 10:36:03PM +0100, Russell King (Oracle) wrote:
> The ordering of ptp_clock_unregister() is not ideal, as the chardev
> remains published while state is being torn down, which means userspace
> can race with the kernel teardown. There is also no cleanup of enabled
> pin settings nor of the internal PPS event, which means enabled events
> can still forward into the core, dereferencing a free'd pointer.
> 
> Rework the ordering of cleanup in ptp_clock_unregister() so that we
> unpublish the posix clock (and user chardev), disable any pins that
> have EXTTS events enabled, disable the PPS event, and then clean up
> the aux work and PPS source.
> 
> This avoids potential use-after-free and races in PTP clock driver
> teardown.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> # ocelot, sja1105, netdevsim, vclocks

Re: [PATCH net-next v2 2/2] ptp: rework ptp_clock_unregister() to disable events

[Vadim Fedorenko]

On 16/09/2025 22:36, Russell King (Oracle) wrote:
> The ordering of ptp_clock_unregister() is not ideal, as the chardev
> remains published while state is being torn down, which means userspace
> can race with the kernel teardown. There is also no cleanup of enabled
> pin settings nor of the internal PPS event, which means enabled events
> can still forward into the core, dereferencing a free'd pointer.
> 
> Rework the ordering of cleanup in ptp_clock_unregister() so that we
> unpublish the posix clock (and user chardev), disable any pins that
> have EXTTS events enabled, disable the PPS event, and then clean up
> the aux work and PPS source.
> 
> This avoids potential use-after-free and races in PTP clock driver
> teardown.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Now LGTM, shouldn't break our use-case, thanks!

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

Re: [PATCH net-next v2 0/2] ptp: safely cleanup when unregistering a PTP clock

[Richard Cochran]

On Tue, Sep 16, 2025 at 10:35:30PM +0100, Russell King (Oracle) wrote:
> (I'm assuming drivers/ptp/ patches go via net-next as there is no
> git tree against the "PTP HARDWARE CLOCK SUPPORT" maintainers entry.)
> 
> The standard rule in the kernel for unregistering user visible devices
> is to unpublish the userspace API before doing any shutdown of the
> resources necessary for the operation of the device.
> 
> PTP has several issues in this area:
> 
> 1. ptp_clock_unregister() cancells and destroys work while the PTP
>    chardev is still published, which gives the opportunity for a
>    precisely timed user API call to cause a driver to attempt to
>    queue the aux work.
> 
> 2. PTP pins are not cleaned up - if userspace has enabled PTP pins,
>    e.g. for extts, drivers are forced to do cleanup before calling
>    ptp_clock_unregister() to stop events being forwarded into the
>    PTP layer. E.g mv88e6xxx cancells its internal tai_event_work
>    to avoid calling into the PTP clock code with a stale ptp_clock
>    pointer, but a badly timed userspace EXTTS enable will re-schedule
>    the tai_event_work.

Thanks, Russell!

For the series:

Acked-by: Richard Cochran <richardcochran@gmail.com>

Re: [PATCH net-next v2 0/2] ptp: safely cleanup when unregistering a PTP clock

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 16 Sep 2025 22:35:30 +0100 you wrote:
> (I'm assuming drivers/ptp/ patches go via net-next as there is no
> git tree against the "PTP HARDWARE CLOCK SUPPORT" maintainers entry.)
> 
> The standard rule in the kernel for unregistering user visible devices
> is to unpublish the userspace API before doing any shutdown of the
> resources necessary for the operation of the device.
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/2] ptp: describe the two disables in ptp_set_pinfunc()
    https://git.kernel.org/netdev/net-next/c/0fcb1dc3e804
  - [net-next,v2,2/2] ptp: rework ptp_clock_unregister() to disable events
    https://git.kernel.org/netdev/net-next/c/a60fc3294a37

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html