* Re: [PATCH net v2] net: fec: only stop PTP if it was initialized
2026-08-26 10:34 [PATCH net v2] net: fec: only stop PTP if it was initialized phucduc.bui
@ 2026-08-27 10:35 ` sashiko-bot
2026-08-27 19:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-27 10:35 UTC (permalink / raw)
To: phucduc.bui; +Cc: imx, Frank.Li
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Hardware access with disabled clocks in `fec_drv_remove()` causing kernel panic
- [Critical] PTP clock ioctls access hardware directly without verifying clock status, leading to kernel panic
- [High] Race condition causing Use-After-Free during `fec_drv_remove()` watchdog teardown
- [High] Initialization order race in `fec_probe()` causing workqueue corruption
- [High] Race condition causing Use-After-Free during PTP timer teardown in `fec_ptp_stop()`
- [High] Use-After-Free in probe error path due to premature explicit freeing of network device
- [High] Concurrent `fec_pps_interrupt` execution and `fec_ptp_stop` causes Use-After-Free
--
commit e0b7dcf388492f09f947c8b3bfed37b87cb8fc59
Author: bui duc phuc <phucduc.bui@gmail.com>
net: fec: only stop PTP if it was initialized
fec_ptp_init() is only called when fep->bufdesc_ex is available.
However, fec_probe() unconditionally calls fec_ptp_stop() on the
failed_init path, and fec_drv_remove() unconditionally calls
fec_ptp_stop() during device removal.
Check fep->bufdesc_ex before calling fec_ptp_stop() in both paths
to avoid stopping PTP when it was not initialized.
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ced4dbf8cd90f..794ec427b0ee6 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5457,7 +5457,8 @@ fec_probe(struct platform_device *pdev)
> failed_irq:
> fec_enet_deinit(ndev);
> failed_init:
> - fec_ptp_stop(pdev);
> + if (fep->bufdesc_ex)
> + fec_ptp_stop(pdev);
[Severity: High]
This is a pre-existing issue, but does the initialization order in
fec_probe() create a race condition that corrupts the workqueue?
Currently, the device is registered and exposed to the system before the TX
timeout work is initialized:
fec_probe() {
...
ret = register_netdev(ndev);
if (ret)
goto failed_register;
...
INIT_WORK(&fep->tx_timeout_work, fec_enet_timeout_work);
...
}
If a fast userspace actor brings the interface up and triggers a TX timeout
after register_netdev() but before INIT_WORK(), .ndo_tx_timeout (fec_timeout)
could call schedule_work() on an uninitialized work structure, potentially
corrupting the workqueue. Should INIT_WORK() run before register_netdev()?
[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk in the probe
error path?
At the end of fec_probe():
failed_ioremap:
free_netdev(ndev);
return ret;
Since devm_request_irq() was used for the interrupt handler, the IRQ is not
freed until after fec_probe() returns. Because fec_enet_init() writes to the
MAC interrupt mask register to enable TX interrupts, a pending interrupt
could fire during this window.
If fec_enet_interrupt() executes, it would dereference the already-freed ndev
pointer. Would it be safer to free the netdev via devres, or manually disable
interrupts/free the IRQ before freeing ndev?
[Severity: High]
This is a pre-existing issue, but does the timer teardown sequence
in fec_ptp_stop() properly protect against concurrent ioctls?
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);
}
The PTP timers are cancelled before the PTP clock character device is
unregistered. Could a concurrent ioctl (like PTP_PEROUT_REQUEST) invoke
fec_ptp_enable() and re-arm the timers during unregistration?
If so, could the re-armed timers fire after the driver is removed and access
freed memory? Should ptp_clock_unregister() be called first to block new
ioctl requests?
[Severity: High]
This is a pre-existing issue, but does fec_ptp_stop() synchronize with a
currently running PPS interrupt handler?
fec_ptp_stop() disables the PPS interrupt at the hardware level but does not
call synchronize_irq(). It then unregisters the PTP clock:
if (fep->ptp_clock)
ptp_clock_unregister(fep->ptp_clock);
If a concurrent fec_pps_interrupt() is running on another CPU, could it
dereference fep->ptp_clock via ptp_clock_event(fep->ptp_clock, &event) after
the clock instance has been unregistered and freed?
[Severity: Critical]
This is a pre-existing issue, but do the PTP clock ioctls safely access
hardware when the device is runtime-suspended?
Functions like fec_ptp_adjfine() perform direct register writes:
fec_ptp_adjfine() {
...
tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
tmp |= corr_ns << FEC_T_INC_CORR_OFFSET;
writel(tmp, fep->hwp + FEC_ATIME_INC);
}
Unlike fec_ptp_gettime(), which checks fep->ptp_clk_on under a mutex,
fec_ptp_adjfine() doesn't seem to verify if the underlying clocks are active.
If an ioctl triggers this while the network interface is down and clocks are
disabled, could this direct hardware access cause a synchronous abort
and crash the system?
> failed_reset:
> pm_runtime_put_noidle(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
[ ... ]
> @@ -5499,7 +5500,8 @@ fec_drv_remove(struct platform_device *pdev)
> ERR_PTR(ret));
>
> cancel_work_sync(&fep->tx_timeout_work);
[Severity: Critical]
This is a pre-existing issue, but does ignoring a runtime PM resume failure
here lead to a kernel panic?
Just above this context, pm_runtime_get_sync() is called:
ret = pm_runtime_get_sync(&pdev->dev);
if (ret < 0)
dev_err(&pdev->dev, ...);
If pm_runtime_get_sync() fails, the device clocks remain disabled. However,
execution proceeds to fec_ptp_stop(), which eventually calls
fec_ptp_enable_pps() and performs direct register writes
(writel(0, fep->hwp + FEC_TCSR(fep->pps_channel))).
Could writing to the SoC registers while their backing clocks are disabled
cause a synchronous external abort? Should the teardown path gracefully
handle this resume failure?
> - fec_ptp_stop(pdev);
> + if (fep->bufdesc_ex)
> + fec_ptp_stop(pdev);
> unregister_netdev(ndev);
[Severity: High]
This is a pre-existing issue, but does cancelling the timeout work before
unregistering the network device leave a race condition window?
In the current sequence:
cancel_work_sync(&fep->tx_timeout_work);
if (fep->bufdesc_ex)
fec_ptp_stop(pdev);
unregister_netdev(ndev);
The network device is still fully active when cancel_work_sync() is called.
Could the network core's watchdog timer fire right after the cancellation,
invoke .ndo_tx_timeout, and re-queue the work item via schedule_work()?
If that happens, and the driver subsequently unregisters and frees the device,
could the re-armed work item execute later and cause a use-after-free?
> fec_enet_mii_remove(fep);
> if (fep->reg_phy)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260826103428.32807-1-phucduc.bui@gmail.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net v2] net: fec: only stop PTP if it was initialized
2026-08-26 10:34 [PATCH net v2] net: fec: only stop PTP if it was initialized phucduc.bui
2026-08-27 10:35 ` sashiko-bot
@ 2026-08-27 19:50 ` patchwork-bot+netdevbpf
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-08-27 19:50 UTC (permalink / raw)
To: Bui Duc Phuc
Cc: wei.fang, frank.li, shenwei.wang, andrew+netdev, davem, edumazet,
kuba, pabeni, richardcochran, imx, linux-kernel, netdev, Frank.Li
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Wed, 26 Aug 2026 17:34:28 +0700 you wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> fec_ptp_init() is only called when fep->bufdesc_ex is available.
> However, fec_probe() unconditionally calls fec_ptp_stop() on the
> failed_init path, and fec_drv_remove() unconditionally calls
> fec_ptp_stop() during device removal.
>
> [...]
Here is the summary with links:
- [net,v2] net: fec: only stop PTP if it was initialized
https://git.kernel.org/netdev/net/c/dd890ae29299
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread