[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

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 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 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.

[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 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 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 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