[PATCH net v2 0/2] net: fec: fix PTP clock teardown races on device removal

[PATCH net v2 0/2] net: fec: fix PTP clock teardown races on device removal

[Shengzhuo Wei]

This series fixes two FEC teardown races: ethtool can access a freed
PTP clock, and the PPS interrupt can outlive the PHC and netdev.

I have kept the main Ethernet IRQ issue discussed with Phuc [1] out of
this revision. His PTP initialization error-propagation changes are
also separate; if they land first, the new probe failure path will
need PPS IRQ cleanup too.

Full x86_64 kernel and module builds passed with W=1, FEC=m and PTP
enabled and disabled. The bugs were found by code inspection; no
hardware testing was done.

[1] https://lore.kernel.org/r/CAABR9nHLcR103aADF1WMSEkw-_BjWB9vyWXENJCYfjX5Wz1nkw@mail.gmail.com/

---
Link to v1: https://lore.kernel.org/netdev/20260904-fec-ptp-pps-event-uaf-v1-0-9af446be4a11@cherr.cc

Changes in v2:
- Drop the ptp_clock_index_by_dev() lookup and unregister the netdev
  before fec_ptp_stop() instead, as suggested by Wei Fang, so the
  netdev callbacks are drained before the PHC teardown.
- Use -1 as the "no PPS interrupt" sentinel so a valid IRQ 0 is neither
  skipped on release nor freed when the request never happened.

---
Shengzhuo Wei (2):
      net: fec: stop the PTP clock after the netdev is unregistered
      net: fec: free the PPS interrupt before tearing down the PHC and netdev

 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c |  2 +-
 drivers/net/ethernet/freescale/fec_ptp.c  | 12 +++++++++++-
 3 files changed, 13 insertions(+), 2 deletions(-)
---
base-commit: 548e7bcd0c5460ddcbca9600cea603ebeebf4da7
change-id: 20260901-fec-ptp-pps-event-uaf-dcc71b5e1db0

Best regards,
--  
Shengzhuo Wei <me@cherr.cc>

[PATCH net v2 1/2] net: fec: stop the PTP clock after the netdev is unregistered

[Shengzhuo Wei]

fec_drv_remove() unregisters the PTP clock before unregistering the
netdev. A concurrent ethtool -T can therefore pass a freed PTP clock
to ptp_clock_index() through fec_enet_get_ts_info().

Move fec_ptp_stop() after unregister_netdev(), which drains the
ethtool callbacks before the PTP clock is torn down.

Leave the probe error path unchanged, since the netdev has not been
registered there.

Fixes: 32cba57ba74b ("net: fec: introduce fec_ptp_stop and use in probe fail path")
Cc: stable@vger.kernel.org
Suggested-by: Wei Fang <wei.fang@nxp.com>
Assisted-by: GLM:5.3
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 drivers/net/ethernet/freescale/fec_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 794ec427b0ee..a29c8ddbbf34 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5500,9 +5500,9 @@ fec_drv_remove(struct platform_device *pdev)
 			ERR_PTR(ret));
 
 	cancel_work_sync(&fep->tx_timeout_work);
+	unregister_netdev(ndev);
 	if (fep->bufdesc_ex)
 		fec_ptp_stop(pdev);
-	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
 	if (fep->reg_phy)
 		regulator_disable(fep->reg_phy);

-- 
2.47.3

[PATCH net v2 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[Shengzhuo Wei]

The devm-managed PPS interrupt remains registered until after the
remove callback returns, outliving both the PHC and the netdev.
An in-flight handler can call ptp_clock_event() during PHC teardown,
and a later interrupt can dereference the freed netdev.

Record the IRQ after a successful request and release it with
devm_free_irq() in fec_ptp_stop(), before ptp_clock_unregister().
This removes the handler and waits for any running instance to
finish before the PHC and netdev are torn down.

Fixes: 4ad1ceec05e4 ("net: fec: Let fec_ptp have its own interrupt routine")
Cc: stable@vger.kernel.org
Assisted-by: GLM:5.3
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 drivers/net/ethernet/freescale/fec.h     |  1 +
 drivers/net/ethernet/freescale/fec_ptp.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 7176803146f3..960b9f01c531 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -670,6 +670,7 @@ struct fec_enet_private {
 
 	/* pps  */
 	int pps_channel;
+	int pps_irq;
 	unsigned int reload_period;
 	int pps_enable;
 	unsigned int next_counter;
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 56801c2009d5..567ea2a46117 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -778,6 +778,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	hrtimer_setup(&fep->perout_timer, fec_ptp_pps_perout_handler, CLOCK_REALTIME,
 		      HRTIMER_MODE_REL);
 
+	fep->pps_irq = -1;
 	irq = platform_get_irq_byname_optional(pdev, "pps");
 	if (irq < 0)
 		irq = platform_get_irq_optional(pdev, irq_idx);
@@ -787,9 +788,12 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	if (irq >= 0) {
 		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
 				       0, pdev->name, ndev);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
 				 ret);
+		} else {
+			fep->pps_irq = irq;
+		}
 	}
 
 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
@@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&fep->time_keep);
 	hrtimer_cancel(&fep->perout_timer);
+
+	if (fep->pps_irq >= 0) {
+		devm_free_irq(&pdev->dev, fep->pps_irq, ndev);
+		fep->pps_irq = -1;
+	}
+
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
 }

-- 
2.47.3

RE: [PATCH net v2 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[Wei Fang]

> The devm-managed PPS interrupt remains registered until after the
> remove callback returns, outliving both the PHC and the netdev.
> An in-flight handler can call ptp_clock_event() during PHC teardown,
> and a later interrupt can dereference the freed netdev.
>
> Record the IRQ after a successful request and release it with
> devm_free_irq() in fec_ptp_stop(), before ptp_clock_unregister().
> This removes the handler and waits for any running instance to
> finish before the PHC and netdev are torn down.
>
> Fixes: 4ad1ceec05e4 ("net: fec: Let fec_ptp have its own interrupt routine")
> Cc: stable@vger.kernel.org
> Assisted-by: GLM:5.3
> Signed-off-by: Shengzhuo Wei <me@cherr.cc>
> ---
>  drivers/net/ethernet/freescale/fec.h     |  1 +
>  drivers/net/ethernet/freescale/fec_ptp.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 7176803146f3..960b9f01c531 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -670,6 +670,7 @@ struct fec_enet_private {
>
>       /* pps  */
>       int pps_channel;
> +     int pps_irq;
>       unsigned int reload_period;
>       int pps_enable;
>       unsigned int next_counter;
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
> b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d5..567ea2a46117 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -778,6 +778,7 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>       hrtimer_setup(&fep->perout_timer, fec_ptp_pps_perout_handler,
> CLOCK_REALTIME,
>                     HRTIMER_MODE_REL);
>
> +     fep->pps_irq = -1;
>       irq = platform_get_irq_byname_optional(pdev, "pps");
>       if (irq < 0)
>               irq = platform_get_irq_optional(pdev, irq_idx);
> @@ -787,9 +788,12 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>       if (irq >= 0) {
>               ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>                                      0, pdev->name, ndev);
> -             if (ret < 0)
> +             if (ret < 0) {
>                       dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>                                ret);
> +             } else {
> +                     fep->pps_irq = irq;
> +             }

braces {} are not necessary for single statement blocks

>       }
>
>       fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
> @@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
>
>       cancel_delayed_work_sync(&fep->time_keep);
>       hrtimer_cancel(&fep->perout_timer);
> +
> +     if (fep->pps_irq >= 0) {
> +             devm_free_irq(&pdev->dev, fep->pps_irq, ndev);

Since devm_*_irq APIs are useless for PPS irq, I think it is better to use
request_irq() and free_irq() instead.

> +             fep->pps_irq = -1;
> +     }
> +
>       if (fep->ptp_clock)
>               ptp_clock_unregister(fep->ptp_clock);
>  }
>
> --
> 2.47.3

NXP Confidential

Re: [PATCH net v2 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[Bui Duc Phuc]

> @@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
>
>         cancel_delayed_work_sync(&fep->time_keep);
>         hrtimer_cancel(&fep->perout_timer);
> +
> +       if (fep->pps_irq >= 0) {
> +               devm_free_irq(&pdev->dev, fep->pps_irq, ndev);
> +               fep->pps_irq = -1;
> +       }
> +

I noticed that fec_ptp_stop(pdev) is called only from the failed_init
error path in fec_probe():

failed_init:
fec_ptp_stop(pdev);
failed_reset:
...
failed_ioremap:
free_netdev(ndev);

There are several other error paths below failed_init that eventually
reach free_netdev(ndev) without calling fec_ptp_stop().

Since fec_ptp_stop() is currently responsible for calling
devm_free_irq(&pdev->dev, fep->pps_irq, ndev),
could this result in a use-after-free if the PPS IRQ has already been
registered and one of these lower error paths is taken?

Would it make more sense to handle the PPS IRQ cleanup separately on
these error paths, or,
use a devm-based approach so that the IRQ lifetime is managed automatically?

Re: [PATCH net v2 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[Bui Duc Phuc]

Sorry, I got this wrong.
Please disregard my previous comment.

On Tue, Sep 8, 2026 at 5:13 PM Bui Duc Phuc <phucduc.bui@gmail.com> wrote:
>
> > @@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
> >
> >         cancel_delayed_work_sync(&fep->time_keep);
> >         hrtimer_cancel(&fep->perout_timer);
> > +
> > +       if (fep->pps_irq >= 0) {
> > +               devm_free_irq(&pdev->dev, fep->pps_irq, ndev);
> > +               fep->pps_irq = -1;
> > +       }
> > +
>
> I noticed that fec_ptp_stop(pdev) is called only from the failed_init
> error path in fec_probe():
>
> failed_init:
> fec_ptp_stop(pdev);
> failed_reset:
> ...
> failed_ioremap:
> free_netdev(ndev);
>
> There are several other error paths below failed_init that eventually
> reach free_netdev(ndev) without calling fec_ptp_stop().
>
> Since fec_ptp_stop() is currently responsible for calling
> devm_free_irq(&pdev->dev, fep->pps_irq, ndev),
> could this result in a use-after-free if the PPS IRQ has already been
> registered and one of these lower error paths is taken?
>
> Would it make more sense to handle the PPS IRQ cleanup separately on
> these error paths, or,
> use a devm-based approach so that the IRQ lifetime is managed automatically?

Re: [PATCH net v2 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0 (1)

- [Medium] fec_ptp_stop() now removes the PPS interrupt handler
  (devm_free_irq(), drivers/net/ethernet/freescale/fec_ptp.c:870)…

Pre-existing issues:
- [High] Use-after-free of fec_enet_private via the perout hrtimer during
  driver removal.
- [Medium] fec_pps_interrupt()
  (drivers/net/ethernet/freescale/fec_ptp.c:696-728) performs a read-…
- [Low] Kernel-doc above fec_ptp_init()
  (drivers/net/ethernet/freescale/fec_ptp.c:735) states "If ptp support…

--

net: fec: free the PPS interrupt before tearing down the PHC and netdev

This records the PPS interrupt number in struct fec_enet_private when
devm_request_irq() succeeds, and releases it with devm_free_irq() in
fec_ptp_stop() before ptp_clock_unregister(), so the handler is gone
before the PHC and the netdev go away.

A few questions about the teardown ordering and about state that is not
covered by the new devm_free_irq() call.

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..567ea2a461173 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -778,6 +778,7 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  	hrtimer_setup(&fep->perout_timer, fec_ptp_pps_perout_handler, CLOCK_REALTIME,
>  		      HRTIMER_MODE_REL);
>  
> +	fep->pps_irq = -1;

[Severity: Low]
This is a pre-existing documentation issue and not something this patch
introduces, but the kernel-doc directly above fec_ptp_init() in
fec_ptp.c claims an early-exit path that the body does not have:

 * This function performs the required steps for enabling ptp
 * support. If ptp support has already been loaded it simply calls the
 * cyclecounter init routine and exits.

The body has no such test: every call re-populates fep->ptp_caps,
re-initialises tmreg_lock, the time_keep delayed work and perout_timer,
and now also resets fep->pps_irq before requesting the pps irq again.

Since fec_ptp_init() is only called once per probe from fec_probe(), this
has no runtime effect today. If a repeat initialisation ever became
possible though, would the unconditional fep->pps_irq = -1 erase the
recorded irq while the old non-shared handler is still installed, so that
the second devm_request_irq() fails and fec_ptp_stop() then skips
devm_free_irq() entirely?

Would it make sense to correct the comment while touching this function?

>  	irq = platform_get_irq_byname_optional(pdev, "pps");
>  	if (irq < 0)
>  		irq = platform_get_irq_optional(pdev, irq_idx);
> @@ -787,9 +788,12 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  	if (irq >= 0) {
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>  				       0, pdev->name, ndev);
> -		if (ret < 0)
> +		if (ret < 0) {
>  			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>  				 ret);
> +		} else {
> +			fep->pps_irq = irq;
> +		}
>  	}
>  
> @@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&fep->time_keep);
>  	hrtimer_cancel(&fep->perout_timer);

[Severity: High]
This is a pre-existing race and not introduced here (the patch only
inserts devm_free_irq() between this cancel and the unregister), but since
the teardown order is what the patch is about: can the perout hrtimer be
re-armed after this hrtimer_cancel() and outlive free_netdev()?

fec_ptp_enable() drops both locks before arming the timer:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_enable() {
	...
unlock:
			spin_unlock_irqrestore(&fep->tmreg_lock, flags);
			mutex_unlock(&fep->ptp_clk_mutex);
	...
			if (delta > FEC_PTP_MAX_NSEC_COUNTER) {
				timeout = ns_to_ktime(delta - NSEC_PER_SEC);
				hrtimer_start(&fep->perout_timer, timeout, HRTIMER_MODE_REL);
	...
}

So for a PTP_PEROUT_REQUEST whose start time is more than
FEC_PTP_MAX_NSEC_COUNTER away, a concurrent unbind can run
fec_drv_remove() -> unregister_netdev() -> fec_ptp_stop() and execute
hrtimer_cancel() while the timer is not yet armed, and the ioctl arms it
afterwards.

Does anything cancel it again? ptp_clock_unregister() waits for in-flight
posix-clock operations and then calls ptp_disable_all_events(), which only
touches EXTTS pins and PPS. This driver sets ptp_caps.n_pins = 0, and the
PPS-off request returns -EBUSY from fec_ptp_enable_pps() while
fep->perout_enable is set, so PEROUT is never disabled.

fec_drv_remove() then reaches:

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
	...
	fec_enet_deinit(ndev);
	free_netdev(ndev);
}

which frees the netdev private area containing both fep and the still
queued perout_timer. When it expires, fec_ptp_pps_perout_handler()
recovers the freed fec_enet_private with container_of() and
fec_ptp_pps_perout() writes fep->hwp registers that devres already
unmapped. Should fec_ptp_stop() do its hrtimer cancellation after the PHC
has been made inaccessible, rather than before?

> +
> +	if (fep->pps_irq >= 0) {
> +		devm_free_irq(&pdev->dev, fep->pps_irq, ndev);
> +		fep->pps_irq = -1;
> +	}

[Severity: Medium]
This is a pre-existing problem, not one added by the patch, but it affects
where the new devm_free_irq() should go: fec_pps_interrupt() does a
read-modify-write of the compare channel without taking fep->tmreg_lock:

drivers/net/ethernet/freescale/fec_ptp.c:fec_pps_interrupt() {
	val = readl(fep->hwp + FEC_TCSR(channel));
	if (val & FEC_T_TF_MASK) {
		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
		do {
			writel(val, fep->hwp + FEC_TCSR(channel));
		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
	...
}

Every other accessor (fec_ptp_enable_pps(), fec_ptp_pps_perout(),
fec_ptp_pps_disable()) holds spin_lock_irqsave(&fep->tmreg_lock), and the
irqsave variant suggests the handler was meant to take it too.

Because val is sampled before the disabling writel(0, FEC_TCSR(ch)) in
fec_ptp_enable_pps(fep, 0), can a handler running on another CPU write
TIE and TMODE back into TCSR after that disable, re-arming the channel and
clobbering fep->next_counter?

In fec_ptp_stop() the new devm_free_irq() sits after
fec_ptp_enable_pps(fep, 0), so the handler is only quiesced once the
state has already been torn down. Would freeing the irq first close that
window?

[Severity: Medium]
Removing the handler here does not seem to be paired with quiescing the
1588 compare channel in hardware for the PEROUT case. The channel is only
disabled when fep->pps_enable is set, via
fec_ptp_stop() -> fec_ptp_enable_pps(fep, 0), which writes 0 to FEC_TCSR.

When the channel was armed through PTP_CLK_REQ_PEROUT instead,
fec_ptp_pps_perout() leaves it enabled:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_pps_perout() {
	...
	temp_val |= (1 << FEC_T_TF_OFFSET | 1 << FEC_T_TIE_OFFSET);
	temp_val &= ~(FEC_T_TMODE_MASK);
	temp_val |= (FEC_TMODE_TOGGLE << FEC_T_TMODE_OFFSET);
	writel(temp_val, fep->hwp + FEC_TCSR(fep->pps_channel));
	...
}

and fec_ptp_stop() only cancels the software hrtimer; it never calls the
existing helper that clears the register:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_pps_disable() {
	hrtimer_cancel(&fep->perout_timer);
	spin_lock_irqsave(&fep->tmreg_lock, flags);
	fep->perout_enable = false;
	writel(0, fep->hwp + FEC_TCSR(channel));
	...
}

The later ptp_clock_unregister() -> ptp_disable_all_events() PPS-off
request cannot help either, since fec_ptp_enable_pps() returns -EBUSY
immediately while fep->perout_enable is true, without touching TCSR.

The close path does not reliably clear it either. On SoCs with
FEC_QUIRK_HAS_MULTI_QUEUES, or FEC_QUIRK_NO_HARD_RESET with the link up,
fec_ctrl_reset() only disables the MAC:

drivers/net/ethernet/freescale/fec_main.c:fec_ctrl_reset() {
	...
		if (fep->quirks & FEC_QUIRK_HAS_MULTI_QUEUES ||
		    ((fep->quirks & FEC_QUIRK_NO_HARD_RESET) && fep->link)) {
			writel(0, fep->hwp + FEC_ECNTRL);
	...
}

so TCSR and TCCR survive fec_stop().

fec_pps_interrupt() is the only code that acknowledges the write-1-to-clear
TF flag and reloads TCCR from fep->next_counter. After this devm_free_irq(),
does the channel stay enabled with TF latched and unacknowledged, and the
output pin still toggling?

And on a later re-bind, would the stale armed channel be serviced as soon
as fec_ptp_init() re-installs the handler with devm_request_irq() -- which
happens before ptp_clock_register() and before anything resets TCSR, as
fec_ptp_start_cyclecounter() only writes ATIME_INC, ATIME_EVT_PERIOD and
ATIME_CTRL -- reprogramming TCCR from a freshly zeroed fep->next_counter
on a channel the new instance considers idle?

Would it be better to clear FEC_TCSR for fep->pps_channel (acknowledging
TF) for both the pps and perout cases, for example by calling
fec_ptp_pps_disable(), and only then drop the handler?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-fec-ptp-pps-event-uaf-v2-0-2a1d57121ade%40cherr.cc