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

[PATCH net v3 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.

The bugs were found by code inspection; no hardware testing was done.

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

---
Changes in v3:
- Use request_irq()/free_irq() for the PPS interrupt and drop
  unnecessary braces in patch 2, as suggested by Wei Fang.
- Patch 1 is unchanged.
- Link to v2: https://lore.kernel.org/all/20260908-fec-ptp-pps-event-uaf-v2-0-2a1d57121ade@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.
- Link to v1: https://lore.kernel.org/netdev/20260904-fec-ptp-pps-event-uaf-v1-0-9af446be4a11@cherr.cc

---
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, 12 insertions(+), 3 deletions(-)
---
base-commit: 548e7bcd0c5460ddcbca9600cea603ebeebf4da7
change-id: 20260901-fec-ptp-pps-event-uaf-dcc71b5e1db0

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

[PATCH net v3 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 v3 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.

Use request_irq() and record the IRQ after a successful request,
then release it with 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, 11 insertions(+), 2 deletions(-)

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..598cd8024123 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);
@@ -785,11 +786,12 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	 * only the PTP_CLOCK_PPS clock events should stop
 	 */
 	if (irq >= 0) {
-		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
-				       0, pdev->name, ndev);
+		ret = request_irq(irq, fec_pps_interrupt, 0, pdev->name, ndev);
 		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 +863,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) {
+		free_irq(fep->pps_irq, ndev);
+		fep->pps_irq = -1;
+	}
+
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
 }

-- 
2.47.3

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

[Wei Fang]

> 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

Thanks for fixing the issue.
And kindly reminder: do not repost a new version within 24 hours.
See https://elixir.bootlin.com/linux/v7.2.2/source/Documentation/process/maintainer-netdev.rst#L434

Reviewed-by: Wei Fang <wei.fang@nxp.com>

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

[Wei Fang]

> 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..598cd8024123 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);
> @@ -785,11 +786,12 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>  	 * only the PTP_CLOCK_PPS clock events should stop
>  	 */
>  	if (irq >= 0) {
> -		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
> -				       0, pdev->name, ndev);
> +		ret = request_irq(irq, fec_pps_interrupt, 0, pdev->name, ndev);
>  		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 +863,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) {
> +		free_irq(fep->pps_irq, ndev);
> +		fep->pps_irq = -1;
> +	}
> +
>  	if (fep->ptp_clock)
>  		ptp_clock_unregister(fep->ptp_clock);
>  }
> 
> --
> 2.47.3

Reviewed-by: Wei Fang <wei.fang@nxp.com>

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

[Bui Duc Phuc]

> +
> +       if (fep->pps_irq >= 0) {
> +               free_irq(fep->pps_irq, ndev);
> +               fep->pps_irq = -1;
> +       }
> +

I think fep->pps_irq > 0 would be more appropriate here,
since the IRQ returned by platform_get_irq_byname_optional() or
platform_get_irq_optional() cannot be 0.

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The network device and its private data are freed before devm-managed interrupts are unregistered, leading to a potential Use-After-Free if an interrupt fires during teardown.
- [High] The tx_timeout_work is cancelled before unregister_netdev, allowing the watchdog to reschedule it before the device is fully closed, leading to a Use-After-Free.
- [High] PTP clock teardown race allows userspace to re-enable hardware interrupts and timers after they are disabled.
- [High] The PTP perout timer can fire and access hardware registers while the PTP clock is disabled.
- [Medium] The global variables `mii_cnt` and `fec0_mii_bus` are mutated without locking during probe and remove.
--

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

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

This commit moves the PTP clock stop function in the driver's remove
path to execute after the netdev is unregistered. This prevents a race
condition where concurrent ethtool callbacks might access a freed PTP
clock.

> 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 by cancelling tx_timeout_work before
unregister_netdev(ndev), is there a window where the network watchdog can
still fire and reschedule the work?

unregister_netdev() stops the network device watchdog via dev_close(). In
the window between cancel_work_sync() and dev_close(), a transmit timeout
can still occur. If this happens, fec_timeout() can run and reschedule
the work:

    schedule_work(&fep->tx_timeout_work);

Could this lead to a use-after-free on fep and ndev if the scheduled work
executes after the device teardown is complete?

> +	unregister_netdev(ndev);
>  	if (fep->bufdesc_ex)
>  		fec_ptp_stop(pdev);

[Severity: High]
This is a pre-existing issue, but could the PTP perout timer fire and
access hardware registers while the PTP clock is disabled?

When unregister_netdev() is called, it brings down the interface, which
calls fec_enet_close(). This in turn disables clk_ptp.

Because the perout_timer is not cancelled until later in fec_ptp_stop(),
the timer might fire after the clock is disabled. If this happens,
fec_ptp_pps_perout() will access PTP registers:

    temp_val = readl(fep->hwp + FEC_ATIME_CTRL);
    temp_val |= FEC_T_CTRL_PINPER;
    writel(temp_val, fep->hwp + FEC_ATIME_CTRL);

Without checking if the clock is running, wouldn't accessing these registers
trigger an external synchronous abort (kernel panic) on ARM?

[Severity: High]
This is also a pre-existing issue, but does fec_ptp_stop() leave a race 
window that allows userspace to re-enable timers and interrupts?

Inside fec_ptp_stop(), timers and features are disabled before the PTP
clock is unregistered:

    hrtimer_cancel(&fep->perout_timer);

    if (fep->pps_irq >= 0) {
        free_irq(fep->pps_irq, ndev);
        fep->pps_irq = -1;
    }

    if (fep->ptp_clock)
        ptp_clock_unregister(fep->ptp_clock);

During this intermediate window, could concurrent userspace ioctls (such
as PTP_CLK_REQ_PPS) execute and re-enable the PPS hardware interrupt or
restart the hrtimer?

> -	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);

[Severity: Medium]
This is a pre-existing issue, but is it safe to mutate the shared mii_cnt 
variable without locking during probe and remove?

fec_enet_mii_init() increments mii_cnt, and fec_enet_mii_remove() decrements
it. The driver uses these static global variables to share a single MDIO
bus between multiple FEC MAC instances.

If instances probe or remove asynchronously, or via hotplug, could concurrent
modifications corrupt the reference counter? This might lead to a memory leak
or premature unregistration of an actively used MDIO bus.

>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
leave the primary interrupt vulnerable to a use-after-free?

In fec_probe(), the primary interrupt is requested using a devm-managed
function:

    ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt, 0,
                           pdev->name, ndev);

However, later in fec_drv_remove(), free_netdev(ndev) is called
synchronously. Since devres cleanup (which frees the IRQ) only happens after 
fec_drv_remove() completes, the ndev pointer is freed while the IRQ is still
active. If a spurious or delayed interrupt fires in this window, it would
result in a use-after-free on ndev.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-fec-ptp-pps-event-uaf-v3-0-bead12c29a16@cherr.cc?part=1

Re: [PATCH net v3 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 ethernet interrupts are managed via devres but `netdev` is freed manually, causing a use-after-free if an interrupt fires after driver removal.
- [High] Unregistering the PTP clock after canceling timers and freeing IRQs allows concurrent ioctls to re-arm them, leading to Use-After-Free.
- [High] Race condition causing tx_timeout_work to be queued after it is cancelled, leading to a use-after-free when the work executes.
--

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

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

The devm-managed PPS interrupt remains registered until after the remove
callback returns, outliving both the PHC and the netdev. This patch uses
request_irq() and free_irq() to explicitly manage the interrupt lifetime.

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -861,6 +863,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) {
> +		free_irq(fep->pps_irq, ndev);
> +		fep->pps_irq = -1;
> +	}
> +
>  	if (fep->ptp_clock)
>  		ptp_clock_unregister(fep->ptp_clock);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does unregistering the PTP clock after
canceling timers and freeing IRQs allow concurrent ioctls to re-arm them?

In fec_ptp_stop(), the driver cancels the perout_timer hrtimer before calling
ptp_clock_unregister(). Because the PTP char device is still registered and
active, a concurrent userspace thread can issue a PTP clock request ioctl
which calls fec_ptp_enable() and re-arms the timer via hrtimer_start().

fec_ptp_stop() finishes, and fec_drv_remove() frees the netdev. When the
re-armed timer fires, its callback fec_ptp_pps_perout_handler() accesses the
freed fep structure, leading to a use-after-free.

Should ptp_clock_unregister() be called before tearing down the timers and
interrupts to prevent them from being re-armed?


[Severity: High]
This is a pre-existing issue, but shares the same root cause as the PPS
interrupt bug this patch is fixing. Does the driver also have a use-after-free
risk with the main ethernet interrupts?

In fec_probe(), the main ethernet interrupts are requested using devres:

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

However, in fec_drv_remove(), the netdev is freed manually:

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

Because devm_request_irq() ties the interrupt lifetime to the device rather
than the netdev lifecycle, the interrupts are not freed until after
fec_drv_remove() returns. If an interrupt fires in this window,
fec_enet_interrupt() will execute and dereference the freed ndev pointer.

Should the main ethernet interrupts also be converted to request_irq() and
freed explicitly in fec_drv_remove() before free_netdev() is called?


[Severity: High]
This is a pre-existing issue, but is there a race condition causing
tx_timeout_work to be queued after it is cancelled?

In fec_drv_remove(), cancel_work_sync() is called before unregister_netdev():

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

While unregister_netdev() is executing, the netdev is still active, allowing
the networking core watchdog to detect a transmit timeout and invoke
ndo_tx_timeout (fec_timeout()). This asynchronously re-queues tx_timeout_work.

The driver continues teardown and frees the netdev. When the workqueue
executes fec_enet_timeout_work(), it accesses the freed fep pointer, causing a
use-after-free.

Should unregister_netdev() be called before cancel_work_sync() to ensure no
new work is queued?

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

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

[Shengzhuo Wei]

> > +
> > +       if (fep->pps_irq >= 0) {
> > +               free_irq(fep->pps_irq, ndev);
> > +               fep->pps_irq = -1;
> > +       }
> > +
> 
> I think fep->pps_irq > 0 would be more appropriate here,
> since the IRQ returned by platform_get_irq_byname_optional() or
> platform_get_irq_optional() cannot be 0.

Hi Phuc,

You're right. I hadn't checked whether these helpers could return
IRQ 0.

I saw the existing irq >= 0 check in the request path and wanted
the release path to match it. I used -1 to distinguish "no IRQ
requested" from a possible IRQ 0, recording the IRQ only after
request_irq() succeeded.

Since both helpers reject IRQ 0, that extra handling is unnecessary.
Keeping pps_irq at its initial value of 0 and checking > 0 before
free_irq() is sufficient.

Wei, I plan to drop the -1 initialization, use > 0 for the release
check, and reset pps_irq to 0 after freeing it. Do you have any
concerns with that change?

Thanks for pointing this out.

Best regards,
Shengzhuo

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

[Wei Fang]

> > > +
> > > +       if (fep->pps_irq >= 0) {
> > > +               free_irq(fep->pps_irq, ndev);
> > > +               fep->pps_irq = -1;
> > > +       }
> > > +
> >
> > I think fep->pps_irq > 0 would be more appropriate here,
> > since the IRQ returned by platform_get_irq_byname_optional() or
> > platform_get_irq_optional() cannot be 0.
> 
> Hi Phuc,
> 
> You're right. I hadn't checked whether these helpers could return
> IRQ 0.
> 
> I saw the existing irq >= 0 check in the request path and wanted
> the release path to match it. I used -1 to distinguish "no IRQ
> requested" from a possible IRQ 0, recording the IRQ only after
> request_irq() succeeded.
> 
> Since both helpers reject IRQ 0, that extra handling is unnecessary.
> Keeping pps_irq at its initial value of 0 and checking > 0 before
> free_irq() is sufficient.
> 
> Wei, I plan to drop the -1 initialization, use > 0 for the release
> check, and reset pps_irq to 0 after freeing it. Do you have any
> concerns with that change?
> 

The only concern is that your current series is conflicted with Phuc.
You'd better to rebase your patches after Phuc's patches are applied
to the net-next tree, and then post them for review. :)

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

[netdev-bot+sashiko]

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

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

- [Medium] Moving fec_ptp_stop() after unregister_netdev() in
  fec_drv_remove() (drivers/net/ethernet/freescale/fec_main.c:5503-5505)…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  fec_drv_remove() still cancels tx_timeout_work before the netdev is…
- [High] This problem wasn't introduced by this patch, but fec_ptp_stop()
  still cancels the perout hrtimer before unregistering the PHC, so a…

--

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

This moves the fec_ptp_stop() call in fec_drv_remove() to after
unregister_netdev() so that ethtool callbacks are drained before the PTP
clock is unregistered.  The probe error path is left unchanged.

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

[Severity: High]
This isn't a bug introduced by this patch, but can the tx timeout work be
re-queued after this cancel_work_sync() returns?

At this point the netdev is still registered and running, so the qdisc TX
watchdog can still call ndo_tx_timeout:

fec_main.c:fec_timeout() {
	...
	schedule_work(&fep->tx_timeout_work);
}

Nothing disarms the watchdog until unregister_netdev() -> dev_close() ->
dev_deactivate() runs, and there is no second cancel_work_sync() later in
fec_drv_remove().

fec_enet_timeout_work() also takes rtnl_lock(), which unregister_netdev()
holds, so it can be pushed past unregistration and still be pending when
fec_drv_remove() reaches:

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

Since the work_struct lives inside netdev_priv memory and the worker
dereferences fep->netdev, does this become a use-after-free of the netdev
and the work_struct?

Would moving cancel_work_sync() to after unregister_netdev() (or adding a
second cancel before free_netdev()) address that?

> +	unregister_netdev(ndev);
>  	if (fep->bufdesc_ex)
>  		fec_ptp_stop(pdev);

[Severity: Medium]
With this ordering, does fec_ptp_stop() now touch the 1588 registers while
clk_ptp is gated?

unregister_netdev() -> dev_close() -> fec_enet_close() runs first, and for
bufdesc_ex it goes through fec_stop():

fec_main.c:fec_stop() {
	...
		fec_ptp_start_cyclecounter(ndev);
		fec_ptp_restore_state(fep);
}

fec_ptp_restore_state() re-arms PPS via fec_ptp_enable_pps(fep, 1) when PPS
was enabled, so fep->pps_enable is 1 on return.  fec_enet_close() then
does:

fec_main.c:fec_enet_close() {
	...
	fec_enet_clk_enable(ndev, false);
}

which does clk_disable_unprepare(fep->clk_ptp) and clears fep->ptp_clk_on.

Control then returns to fec_drv_remove() and fec_ptp_stop() runs:

fec_ptp.c:fec_ptp_stop() {
	if (fep->pps_enable)
		fec_ptp_enable_pps(fep, 0);

	cancel_delayed_work_sync(&fep->time_keep);
	hrtimer_cancel(&fep->perout_timer);
	...
}

fec_ptp_enable_pps() reaches the disable branch with no ptp_clk_on or
ptp_clk_mutex guard:

fec_ptp.c:fec_ptp_enable_pps() {
	...
	} else {
		writel(0, fep->hwp + FEC_TCSR(fep->pps_channel));
	}
}

Every other 1588 register path in the driver (fec_ptp_gettime,
fec_ptp_settime, the PEROUT branch of fec_ptp_enable, fec_time_keep) bails
out when !fep->ptp_clk_on.  If this write is not latched with the clock
gated, does the output-compare channel and its TIE interrupt enable stay
armed while software records pps_enable = 0?

The same reorder also moves hrtimer_cancel(&fep->perout_timer) past the
clock gating, so can an already-armed perout hrtimer run
fec_ptp_pps_perout() from hardirq context and touch FEC_ATIME_CTRL /
FEC_TCCR / FEC_TCSR in the gated clock domain?

fec_ptp.c:fec_ptp_pps_perout() {
	...
	temp_val = readl(fep->hwp + FEC_ATIME_CTRL);
	temp_val |= FEC_T_CTRL_PINPER;
	writel(temp_val, fep->hwp + FEC_ATIME_CTRL);
	...
}

Would quiescing PPS/PEROUT and cancelling the hrtimer while clk_ptp is
still on, and deferring only ptp_clock_unregister() past
unregister_netdev(), keep the intended ethtool lifetime fix without this
window?

[Severity: High]
This is a pre-existing issue, but fec_ptp_stop() cancels the perout hrtimer
before unregistering the PHC, so can a concurrent PTP ioctl re-arm it?

fec_ptp.c:fec_ptp_stop() {
	...
	cancel_delayed_work_sync(&fep->time_keep);
	hrtimer_cancel(&fep->perout_timer);
	...
	if (fep->ptp_clock)
		ptp_clock_unregister(fep->ptp_clock);
}

In the PTP_CLK_REQ_PEROUT branch of fec_ptp_enable(), fep->ptp_clk_on is
validated under ptp_clk_mutex, but both ptp_clk_mutex and tmreg_lock are
dropped before the timer is armed:

fec_ptp.c:fec_ptp_enable() {
	...
			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);
	...
}

A thread preempted between the check and hrtimer_start() can arm the timer
at an arbitrary later point.  ptp_clock_unregister() only waits for the
ioctl to finish, which is after hrtimer_cancel() has already run.
fec_drv_remove() then proceeds to fec_enet_deinit(ndev) and
free_netdev(ndev), freeing fep together with the embedded hrtimer, so does
fec_ptp_pps_perout_handler() end up running against freed memory?

This patch narrows the window (ptp_clk_on is false once the netdev is
closed) but does not close it.  Would unregistering the PHC first, then
cancelling the hrtimer and the delayed work, be the right order here?

> -	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);
>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-fec-ptp-pps-event-uaf-v3-0-bead12c29a16%40cherr.cc

Re: [PATCH net v3 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: 1 (1) · Medium: 0 · Low: 0

- [High] Incomplete fix: only the PPS interrupt is converted to explicit
  request_irq()/free_irq().

Pre-existing issues:
- [High] Teardown ordering race in fec_ptp_stop()
  (drivers/net/ethernet/freescale/fec_ptp.c:856-874):…

--

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

This converts the PPS interrupt in fec_ptp_init() from devm_request_irq()
to request_irq(), records the irq number in fep->pps_irq, and releases it
with free_irq() in fec_ptp_stop() before ptp_clock_unregister().  The goal
is to make the handler go away before the PHC and the netdev are freed.

> 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..598cd8024123 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c

[ ... ]

> @@ -785,11 +786,12 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  	 * only the PTP_CLOCK_PPS clock events should stop
>  	 */
>  	if (irq >= 0) {
> -		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
> -				       0, pdev->name, ndev);
> +		ret = request_irq(irq, fec_pps_interrupt, 0, pdev->name, ndev);
>  		if (ret < 0)
>  			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>  				 ret);
> +		else
> +			fep->pps_irq = irq;
>  	}

[Severity: High]
The commit message says the change closes the case where "a later interrupt
can dereference the freed netdev".  Does that hold once the main FEC
interrupts are considered?

fec_probe() still registers them with devres, using the same ndev cookie:

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

devres_release_all() runs only after ->remove() returns, but
fec_drv_remove() already frees the netdev inside the callback:

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

If the line is asserted in that window, fec_enet_interrupt() dereferences
the cookie immediately:

drivers/net/ethernet/freescale/fec_main.c:fec_enet_interrupt() {
	struct net_device *ndev = dev_id;
	struct fec_enet_private *fep = netdev_priv(ndev);
	...
}

The same ordering applies to the probe unwind, where failed_irq,
failed_mii_init and failed_register all fall through to free_netdev(ndev)
with the handlers still installed.

Should fep->irq[] be released with free_irq() in fec_drv_remove() (or the
netdev allocation switched to a devm-managed one) so the lifetime rule is
the same for every irq registered with ndev as dev_id?  As it stands the
driver ends up with two different conventions for identically-cookied
interrupts, and the failure mode described in the changelog is still
reachable through the main irq lines.

> @@ -861,6 +863,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) {
> +		free_irq(fep->pps_irq, ndev);
> +		fep->pps_irq = -1;
> +	}
> +
>  	if (fep->ptp_clock)
>  		ptp_clock_unregister(fep->ptp_clock);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the ordering in
fec_ptp_stop() means both hrtimer_cancel() and the new free_irq() run while
the PHC character device is still registered, since ptp_clock_unregister()
comes last.

Can an in-flight ioctl re-arm perout_timer after the cancel?
fec_ptp_enable() drops ptp_clk_mutex and tmreg_lock before arming:

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

posix_clock_ioctl() holds only down_read(&clk->rwsem) across that callback,
and fec_ptp_stop() takes none of those locks.  ptp_clock_unregister() ->
posix_clock_unregister() then waits for the ioctl to finish, so the arm
completes and nothing cancels the timer again.

fec_drv_remove() afterwards calls free_netdev(ndev), which frees the
fec_enet_private embedding perout_timer, and the later expiry runs:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_pps_perout_handler() {
	struct fec_enet_private *fep = container_of(timer,
					struct fec_enet_private, perout_timer);

	fec_ptp_pps_perout(fep);
	...
}

The ptp_clk_on check in fec_ptp_enable() only rejects a fresh request; a
request already past that check is unaffected.  The window also exists on
the probe error path, fec_probe() failed_init -> fec_ptp_stop(), where
ptp_clk_on is true for the whole of probe.

Symmetrically, can a PTP_ENABLE_PPS ioctl reach fec_ptp_enable_pps(fep, 1)
after free_irq() and re-enable the hardware compare interrupt with no
handler installed?  fec_ptp_enable_pps() has no ptp_clk_on gate.

Would it be better to unregister (or defunct) the PHC first, or to re-cancel
the hrtimer after ptp_clock_unregister() returns?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-fec-ptp-pps-event-uaf-v3-0-bead12c29a16%40cherr.cc

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

[Wei Fang]

> > +	unregister_netdev(ndev);
> >  	if (fep->bufdesc_ex)
> >  		fec_ptp_stop(pdev);
> 
> [Severity: Medium]
> With this ordering, does fec_ptp_stop() now touch the 1588 registers while
> clk_ptp is gated?
> 
> unregister_netdev() -> dev_close() -> fec_enet_close() runs first, and for
> bufdesc_ex it goes through fec_stop():
> 
> fec_main.c:fec_stop() {
> 	...
> 		fec_ptp_start_cyclecounter(ndev);
> 		fec_ptp_restore_state(fep);
> }
> 
> fec_ptp_restore_state() re-arms PPS via fec_ptp_enable_pps(fep, 1) when PPS
> was enabled, so fep->pps_enable is 1 on return.  fec_enet_close() then
> does:
> 
> fec_main.c:fec_enet_close() {
> 	...
> 	fec_enet_clk_enable(ndev, false);
> }
> 
> which does clk_disable_unprepare(fep->clk_ptp) and clears fep->ptp_clk_on.
> 
> Control then returns to fec_drv_remove() and fec_ptp_stop() runs:
> 
> fec_ptp.c:fec_ptp_stop() {
> 	if (fep->pps_enable)
> 		fec_ptp_enable_pps(fep, 0);
> 
> 	cancel_delayed_work_sync(&fep->time_keep);
> 	hrtimer_cancel(&fep->perout_timer);
> 	...
> }
> 
> fec_ptp_enable_pps() reaches the disable branch with no ptp_clk_on or
> ptp_clk_mutex guard:
> 
> fec_ptp.c:fec_ptp_enable_pps() {
> 	...
> 	} else {
> 		writel(0, fep->hwp + FEC_TCSR(fep->pps_channel));
> 	}
> }
> 
> Every other 1588 register path in the driver (fec_ptp_gettime, fec_ptp_settime,
> the PEROUT branch of fec_ptp_enable, fec_time_keep) bails out
> when !fep->ptp_clk_on.  If this write is not latched with the clock gated, does
> the output-compare channel and its TIE interrupt enable stay armed while
> software records pps_enable = 0?
> 

I think this is a false positive, the register cannot be accessed when the ipg
clock is disabled rather than the ptp clock. And the ipg clock is enabled at
the beginning of fec_drv_remove(), it calls pm_runtime_get_sync() -->
fec_runtime_resume() --> clk_prepare_enable(fep->clk_ipg).

See commit 91c0d987a978 ("net: fec: ptp: avoid register access when ipg clock is disabled")
for more details.