[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

[sashiko-bot]

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

Pre-existing issues:
- [High] The main ENET interrupts (`fec_enet_interrupt`) are vulnerable to a use-after-free because they are devm-managed and outlive `free_netdev()`.
- [High] Teardown asymmetry allows tx_timeout_work to be rescheduled after cancellation.
- [High] Teardown asymmetry allows perout_timer to be re-armed after cancellation by a concurrent PTP ioctl.
--

commit e1540d76c9156ff39cff7d5d468d67d8d99e570a
Author: Shengzhuo Wei <me@cherr.cc>

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

This commit resolves a use-after-free issue by explicitly releasing the
PPS interrupt via devm_free_irq() in fec_ptp_stop(). This ensures the
interrupt handler is removed before the PHC and netdev are torn down.

> 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
[ ... ]
> @@ -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 issue, but does this teardown asymmetry allow the
perout_timer to be re-armed after cancellation?

In fec_ptp_enable(), the ptp_clk_mutex is unlocked before hrtimer_start()
is called:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_enable() {
    ...
    hrtimer_start(&fep->perout_timer, timeout, HRTIMER_MODE_REL);
}

fec_ptp_stop() cancels the timer here, but doesn't hold any locks preventing
fec_ptp_enable() from running, nor does it immediately block new ioctls. Could
a concurrent PTP ioctl pass the clock-enabled check, get preempted, and then
call hrtimer_start() after the hrtimer_cancel() call above, causing the timer
to fire on freed memory?

> +
> +	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);
>  }

While reviewing this patch, I noticed a couple of other pre-existing teardown
issues in the FEC driver that might be vulnerable to similar races.

[Severity: High]
This isn't a bug introduced by this patch, but are the main ENET interrupts
vulnerable to a use-after-free during device removal?

The main interrupts are requested as devm-managed resources in fec_probe():

drivers/net/ethernet/freescale/fec_main.c:fec_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt, 0,
                           pdev->name, ndev);
    ...
}

In fec_drv_remove(), the netdev is freed before these devm-managed
interrupts are released:

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

Since devm-managed resources are freed after the remove callback returns,
could an in-flight or spurious interrupt fire after free_netdev() and cause
fec_enet_interrupt() to dereference the freed ndev pointer?

[Severity: High]
This is also a pre-existing issue, but does the teardown sequence allow
tx_timeout_work to be rescheduled after it has been canceled?

In fec_drv_remove(), the work is canceled before the netdev watchdog is
stopped:

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
    ...
    cancel_work_sync(&fep->tx_timeout_work);
    unregister_netdev(ndev);
    ...
}

If the netdev watchdog fires in this window before unregister_netdev()
executes, it calls the ndo_tx_timeout handler which re-arms the work:

drivers/net/ethernet/freescale/fec_main.c:fec_timeout() {
    ...
    schedule_work(&fep->tx_timeout_work);
    ...
}

Could this cause the work item to be executed after the netdev has been
freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-fec-ptp-pps-event-uaf-v2-0-2a1d57121ade@cherr.cc?part=2

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

[sashiko-bot]

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

New issues:
- [High] Moving fec_ptp_stop() after unregister_netdev() introduces a race condition that can cause a hard CPU lockup in the PPS interrupt handler.

Pre-existing issues:
- [High] Calling cancel_work_sync(&fep->tx_timeout_work) before unregister_netdev() creates a race condition that allows the TX timeout work to be rescheduled, leading to a Use-After-Free.
- [High] Active devm-managed IRQ handlers are exposed to Use-After-Free because `ndev` is explicitly freed before the devres device teardown phase completes.
- [High] Hardware accesses in PTP operations are performed without runtime PM protection and without checking the PTP clock state, leading to a potential Synchronous External Abort.
--

commit 2868dfdaff74bb5f34e63d5c554ad690f2b576c1
Author: Shengzhuo Wei <me@cherr.cc>

net: fec: stop the PTP clock after the netdev is unregistered

This commit addresses a race condition where a freed PTP clock could be
accessed during ethtool operations by moving fec_ptp_stop() after
unregister_netdev().

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..a29c8ddbbf34d 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);

[Severity: High]
This is a pre-existing issue, but does calling cancel_work_sync() before
unregister_netdev() leave a window for the work to be rescheduled?

While unregister_netdev() is running, the network interface and its transmit
watchdog timer (dev_watchdog) are still active. If the watchdog timer fires
during this window, could fec_timeout() re-queue the work via
schedule_work(&fep->tx_timeout_work)?

Since unregister_netdev() shuts down the interface but does not cancel newly
scheduled work, the delayed work could run after free_netdev() destroys
ndev, leading to a Use-After-Free.

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling for
pm_runtime_get_sync() safely protect the hardware accesses?

If pm_runtime_get_sync() fails, the device remains suspended, but execution
continues into fec_ptp_stop(), which invokes fec_ptp_enable_pps() and
writes to FEC_TCSR.

Similarly, PTP ioctls or deferred timers might invoke functions like
fec_ptp_adjfine(), which unconditionally access hardware registers:

fec_ptp.c:fec_ptp_adjfine() {
    ...
    tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
    ...
}

Could accessing these registers while the device bus clocks are disabled
cause a Synchronous External Abort (bus fault)?

> +	unregister_netdev(ndev);

[Severity: High]
Does moving unregister_netdev() before fec_ptp_stop() introduce a race
condition that can cause a hard CPU lockup in the PPS interrupt handler?

Calling unregister_netdev() brings the device down and invokes
fec_enet_close(), which disables the PTP clock via
fec_enet_clk_enable(ndev, false).

During the window before fec_ptp_stop() is called, the hardware PPS
interrupt is still registered and unmasked. If the PPS interrupt fires
in this window, the CPU enters fec_pps_interrupt() and tries to clear
the interrupt flag by writing to FEC_TCSR in a do-while loop:

fec_ptp.c:fec_pps_interrupt() {
    ...
    do {
        writel(val, fep->hwp + FEC_TCSR(channel));
    } while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
    ...
}

Since the functional timer clock (clk_ptp) is disabled, the timer IP block
might not process the clear operation. Will the TF_MASK bit never clear,
causing the interrupt handler to spin indefinitely?

>  	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);

[Severity: High]
This is a pre-existing issue, but does explicitly freeing the netdev later
in this function expose devm-managed IRQ handlers to a Use-After-Free?

The driver registers network interrupts using devm_request_irq() in
fec_probe(), passing ndev as the dev_id token. These devm-managed
resources are only released by devres after fec_drv_remove() returns.

However, fec_drv_remove() manually calls free_netdev(ndev) at the end
of the function:

fec_main.c:fec_drv_remove() {
    ...
    fec_enet_deinit(ndev);
    free_netdev(ndev);
}

If a spurious or shared interrupt fires between the free_netdev() execution
and the devres teardown phase, could the active IRQ handler be invoked with
a freed ndev pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-fec-ptp-pps-event-uaf-v2-0-2a1d57121ade@cherr.cc?part=1

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