Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors

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

New issues:
- [Medium] `fec_ptp_init()` incorrectly returns 0 (success) instead of propagating the actual error code when `ptp_clock_register()` fails.

Pre-existing issues:
- [High] Concurrent execution of the PPS interrupt handler and device teardown causes a use-after-free on `fep->ptp_clock`.
--

--- Patch [1]: [PATCH 1/2] net: fec: Propagate PTP initialization errors ---
commit 72db297960b04e49a3fe9e579f0ca646d2fb313b
Author: bui duc phuc <[email protected]>

net: fec: Propagate PTP initialization errors

Change fec_ptp_init() to return an error code instead of silently
ignoring failures during PTP initialization.

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..8ad680411b0c3 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c

[ ... ]

> @@ -779,26 +779,29 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but is it safe to use devm_request_irq() here
without synchronizing during teardown?

During module removal, fec_drv_remove() calls fec_ptp_stop(), which sets
fep->pps_enable = 0 and unregisters fep->ptp_clock. However, since the
interrupt is managed by devres, it isn't freed or synchronized until after
the remove function completes.

If the interrupt fires concurrently on another CPU:

CPU 1 (fec_ptp_stop)
    ptp_clock_unregister(fep->ptp_clock);

CPU 2 (fec_pps_interrupt)
    if (fep->pps_enable) {
        event.type = PTP_CLOCK_PPS;
        ptp_clock_event(fep->ptp_clock, &event);
    }

If CPU 2 reads fep->pps_enable as true just before CPU 1 clears it, could it
then dereference the freed fep->ptp_clock?

>  
>  	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>  	if (IS_ERR(fep->ptp_clock)) {
>  		fep->ptp_clock = NULL;
>  		dev_err(&pdev->dev, "ptp_clock_register failed\n");
> +		return PTR_ERR(fep->ptp_clock);

[Severity: Medium]
Since fep->ptp_clock is set to NULL on the line directly above this one,
doesn't PTR_ERR(fep->ptp_clock) evaluate to 0 here?

This would cause the function to return success instead of propagating the
negative error code from ptp_clock_register().

>  	}
>  
>  	schedule_delayed_work(&fep->time_keep, HZ);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1