Re: [PATCH net-next v2] net: fec: Propagate PTP initialization errors

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The device interrupt handlers are exposed to a use-after-free window on probe error paths because devm_request_irq() is used alongside manual free_netdev().
- [High] Use-after-free on perout_timer due to incorrect teardown ordering in fec_ptp_stop().
--

commit c7933bca59e467b625e0e61dff17b0ea0651dbf8
Author: bui duc phuc <phucduc.bui@gmail.com>
Date:   Thu Sep 3 09:24:30 2026 +0000

    net: fec: Propagate PTP initialization errors

    This commit modifies fec_ptp_init() to return an error code instead of
    silently ignoring failures. It adds error propagation for devm_request_irq()
    and ptp_clock_register() up through the probe function.

    Link v1: https://lore.kernel.org/all/20260820111416.97917-1-phucduc.bui@gmail.com/

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..c6e29b5c2abb4 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5384,8 +5384,11 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_reset;
>  
>  	irq_cnt = fec_enet_get_irq_cnt(pdev);
> -	if (fep->bufdesc_ex)
> -		fec_ptp_init(pdev, irq_cnt);
> +	if (fep->bufdesc_ex) {
> +		ret = fec_ptp_init(pdev, irq_cnt);
> +		if (ret)
> +			goto failed_reset;

[Severity: High]
This isn't a bug introduced by this patch, but returning an error here
exposes a pre-existing use-after-free window in the error path.

Because both the PPS interrupt and MAC interrupts are registered using
devm_request_irq(), their lifetime is tied to the platform device. When we
jump to failed_reset, the cleanup code eventually calls free_netdev(ndev).

Since devm_request_irq() teardown happens via devres_release_all() only
after fec_probe() returns, the interrupts remain active while the netdevice
and driver private data are already freed. 

If an interrupt fires during this window, wouldn't the handlers
(fec_pps_interrupt or fec_enet_interrupt) dereference the freed ndev and
fep pointers? Should the driver avoid mixing devm_request_irq() with manual
free_netdev() to prevent this?

> +	}
>  
>  	ret = fec_enet_init(ndev);
>  	if (ret)

[ ... ]

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..557a76797ebaa 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -779,26 +779,32 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  		      HRTIMER_MODE_REL);
>  
>  	irq = platform_get_irq_byname_optional(pdev, "pps");
> -	if (irq < 0)
> +	if (irq < 0 && irq != -ENXIO)
> +		return irq;
> +	if (irq == -ENXIO) {
>  		irq = platform_get_irq_optional(pdev, irq_idx);
> -	/* Failure to get an irq is not fatal,
> -	 * only the PTP_CLOCK_PPS clock events should stop
> -	 */
> -	if (irq >= 0) {
> +		if (irq < 0 && irq != -ENXIO)
> +			return irq;
> +	}
> +
> +	if (irq > 0) {
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>  				       0, pdev->name, ndev);
>  		if (ret < 0)
> -			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
> -				 ret);
> +			return ret;
>  	}
>  
>  	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>  	if (IS_ERR(fep->ptp_clock)) {
> +		ret = PTR_ERR(fep->ptp_clock);
>  		fep->ptp_clock = NULL;
>  		dev_err(&pdev->dev, "ptp_clock_register failed\n");
> +		return ret;
>  	}
>  
>  	schedule_delayed_work(&fep->time_keep, HZ);
> +
> +	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but since this patch handles the setup of
the PTP clock, it's worth noting the teardown path has a potential
use-after-free.

In fec_ptp_stop(), the driver cancels fep->perout_timer before calling
ptp_clock_unregister(fep->ptp_clock):

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

Because the PTP clock device is still registered with the subsystem while
the timer is cancelled, could a concurrent userspace thread issue a
PTP_CLK_REQ_PEROUT ioctl and re-arm the timer just before the netdevice is
freed by the driver remove function? 

If so, when the re-armed timer expires, its callback 
(fec_ptp_pps_perout_handler) would execute on freed memory.

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