[PATCH net] net: fec: only stop PTP if it was initialized

[PATCH net] net: fec: only stop PTP if it was initialized

[phucduc.bui]

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.

Check fep->bufdesc_ex before calling fec_ptp_stop() to avoid stopping
PTP when it was not initialized.

Fixes: 32cba57ba74b ("net: fec: introduce fec_ptp_stop and use in probe fail path")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ced4dbf8cd90..7a87f40d12d9 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);
 failed_reset:
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-- 
2.43.0

Re: [PATCH net] net: fec: only stop PTP if it was initialized

[Frank Li]

On Mon, Aug 24, 2026 at 05:08:34PM +0700, phucduc.bui@gmail.com 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.
>
> Check fep->bufdesc_ex before calling fec_ptp_stop() to avoid stopping
> PTP when it was not initialized.
>
> Fixes: 32cba57ba74b ("net: fec: introduce fec_ptp_stop and use in probe fail path")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ced4dbf8cd90..7a87f40d12d9 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);
>  failed_reset:
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> --
> 2.43.0
>
>

RE: [PATCH net] net: fec: only stop PTP if it was initialized

[Wei Fang]

> 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.
>
> Check fep->bufdesc_ex before calling fec_ptp_stop() to avoid stopping PTP when
> it was not initialized.
>
> Fixes: 32cba57ba74b ("net: fec: introduce fec_ptp_stop and use in probe fail
> path")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index ced4dbf8cd90..7a87f40d12d9 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);
>  failed_reset:
>       pm_runtime_put_noidle(&pdev->dev);
>       pm_runtime_disable(&pdev->dev);
> --
> 2.43.0

Thanks for fixing this issue.

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


NXP Confidential

RE: [PATCH net] net: fec: only stop PTP if it was initialized

[Wei Fang]

Sorry for replying this thread with same content, just to remove the
"NXP Confidential" label. Sorry for the noise.

> 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.
> 
> Check fep->bufdesc_ex before calling fec_ptp_stop() to avoid stopping PTP when
> it was not initialized.
> 
> Fixes: 32cba57ba74b ("net: fec: introduce fec_ptp_stop and use in probe fail
> path")
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index ced4dbf8cd90..7a87f40d12d9 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);
>  failed_reset:
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> --
> 2.43.0

Thanks for fixing this issue.

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

Re: [PATCH net] net: fec: only stop PTP if it was initialized

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete fix leads to NULL pointer dereference in `fec_drv_remove()` via uninitialized hrtimer cancellation.
- [High] Use-after-free (UAF) due to improper work cancellation order in `fec_drv_remove()`.
- [High] Use-after-free (UAF) due to improper timer cancellation order in `fec_ptp_stop()`.
- [High] Unprotected hardware access leading to kernel panic on timer expiry with clocks disabled.
--

--- Patch [1]: [PATCH net] net: fec: only stop PTP if it was initialized ---
commit 1bdf32b63293bc50d538fd180f2074583880906f
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.

Check fep->bufdesc_ex before calling fec_ptp_stop() 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
> --- 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 since this patch fixes the missing
check in the probe error path, should we also add the same check to
fec_drv_remove()?

fec_drv_remove() calls fec_ptp_stop() unconditionally:

fec_drv_remove()
        ...
        cancel_work_sync(&fep->tx_timeout_work);
        fec_ptp_stop(pdev);
        unregister_netdev(ndev);
        ...

If the device is unbound via sysfs and fep->bufdesc_ex is false, this
will call hrtimer_cancel() on an uninitialized perout_timer, which could
cause a null pointer dereference.

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free in the same device removal path:

fec_drv_remove()
        ...
        cancel_work_sync(&fep->tx_timeout_work);
        fec_ptp_stop(pdev);
        unregister_netdev(ndev);
        ...

Since cancel_work_sync() happens before unregister_netdev(), couldn't
a concurrent tx timeout call schedule_work(&fep->tx_timeout_work)
again before the network device is fully unregistered?

[Severity: High]
This is another pre-existing issue, but looking at fec_ptp_stop(), does the
order of cleanup create a race condition?

fec_ptp_stop()
        ...
        hrtimer_cancel(&fep->perout_timer);
        if (fep->ptp_clock)
                ptp_clock_unregister(fep->ptp_clock);

If a concurrent userspace PTP_CLK_REQ_PEROUT ioctl calls
fec_ptp_enable() before ptp_clock_unregister() finishes, could the
timer be re-armed after it was cancelled, leading to a use-after-free
when the timer expires later?

[Severity: High]
This is a pre-existing issue, but is it possible for the perout_timer
to fire when the hardware clocks are disabled?

When the interface is brought down, the clocks are disabled:

fec_enet_close()
        ...
        fec_enet_clk_enable(ndev, false);

If a delayed ptp perout timer expires after this, the timer callback
will unconditionally access hardware registers:

fec_ptp_pps_perout()
        ...
        /* Get the current ptp hardware time counter */
        ptp_hc = fec_ptp_read(&fep->cc);

Could this trigger a synchronous external abort on platforms where the
clocks are disabled?

>  failed_reset:
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824100834.31655-1-phucduc.bui@gmail.com?part=1

Re: [PATCH net] net: fec: only stop PTP if it was initialized

[Bui Duc Phuc]

> [Severity: High]
> This is a pre-existing issue, but since this patch fixes the missing
> check in the probe error path, should we also add the same check to
> fec_drv_remove()?
>
> fec_drv_remove() calls fec_ptp_stop() unconditionally:
>
> fec_drv_remove()
>         ...
>         cancel_work_sync(&fep->tx_timeout_work);
>         fec_ptp_stop(pdev);
>         unregister_netdev(ndev);
>         ...
>

That makes sense. Checking fep->bufdesc_ex before calling
fec_ptp_stop() in fec_drv_remove()
is indeed necessary. I will update the patch and send v2.


> - [High] Use-after-free (UAF) due to improper work cancellation order in `fec_drv_remove()`.
> - [High] Use-after-free (UAF) due to improper timer cancellation order in `fec_ptp_stop()`.
> - [High] Unprotected hardware access leading to kernel panic on timer expiry with clocks disabled.
> --

These pre-existing issues are outside the scope of this patch.
Since I do not have the physical hardware to properly validate fixes
for these issues,
I think they would be better addressed separately by someone with
access to the relevant hardware.

For the work cancellation issue, a similar case was discussed here:

https://lore.kernel.org/all/20260728062730.2394873-1-satishkh@cisco.com/