Re: [PATCH net 01/13] i40e: Cleanup PTP registration on probe failure

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/4/2026 10:14 PM, Jacob Keller wrote:
> From: Matt Vollrath <tactii@gmail.com>
> 
> Fix two conditions which would leak PTP registration on probe failure:
> 
> 1. i40e_setup_pf_switch can encounter an error in
>    i40e_setup_pf_filter_control, call i40e_ptp_init, then return
>    non-zero, sending i40e_probe to err_vsis.
> 
> 2. i40e_setup_misc_vector can return non-zero, sending i40e_probe to
>    err_vsis.
> 
> Both of these conditions have been present since PTP was introduced in
> this driver.
> 
> Found with coccinelle.
> 
> Fixes: beb0dff1251db ("i40e: enable PTP")
> Signed-off-by: Matt Vollrath <tactii@gmail.com>
> Tested-by: Sunitha Mekala <sunithax.d.mekala@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 028bd500603a..f06fcef644e5 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -16108,6 +16108,7 @@ static int i40e_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	/* Unwind what we've done if something failed in the setup */
>  err_vsis:
>  	set_bit(__I40E_DOWN, pf->state);
> +	i40e_ptp_stop(pf);
>  	i40e_clear_interrupt_scheme(pf);
>  	kfree(pf->vsi);
>  err_switch_setup:
> 

Sashiko points out two possible issues, both of which I think are better
served by follow-up fixes:

> This isn't a bug introduced by this patch, but is it safe to use
> i40e_ptp_stop() in this error path without canceling its workqueue task?
> The PTP initialization logic initializes a background work item
> pf->ptp_extts0_work, which is scheduled asynchronously by the hardware
> interrupt handler. The i40e_ptp_stop() function disables hardware
> interrupts and unregisters the clock without calling
> cancel_work_sync(&pf->ptp_extts0_work). 
> Could this leave the work item pending if an event fires right before
> interrupts are disabled, leading to a use-after-free when the module
> teardown frees the pf structure?


I think we need to double check that extts0_work is cancelled properly
when stopping PTP. Or confirm that it won't cause a problem as-is.

Sashiko also says:

> This isn't a bug introduced by this patch, but does this error path leak VSI
> structures and leave netdevs registered?
> If i40e_setup_pf_switch() fails late, the main VSI has already been
> allocated and its netdev registered via register_netdev(). The err_vsis
> cleanup block only executes kfree(pf->vsi), which frees the array of
> pointers but fails to invoke i40e_vsi_release(vsi) or unregister the netdev.
> Since the driver then proceeds to i40e_free_pf(pf) and frees the underlying
> driver structure, could subsequent userspace operations on the
> still-registered netdev execute driver callbacks that dereference the freed
> pf structure?

The only way that the function can fail late appears to be if
i40e_validate_filter_settings() fails.. but that has a comment around
i40e_setup_pf_filter_control() which claims that we shouldn't the probe
just because that failed.. so I think there is maybe an issue but I'm
not 100% sure how to resolve it properly.. It does seem like there may
be a pre-existing issue in the i40e probe error cleanup.