Re: [PATCH iwl-net 2/2] idpf: release PTP before freeing vports on the init task error path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] idpf: double free of adapter->ptp due to concurrent idpf_ptp_release()
--

commit 140d271eb2e765967ee1a351714e7ad1ecc7d8f6
Author: Emil Tantilov <emil.s.tantilov@intel.com>

idpf: release PTP before freeing vports on the init task error path

Release PTP, which cancels the aux worker, before the unwind frees the
vports, matching the idpf_vc_core_deinit() order. Previously it was
possible for idpf_ptp_update_cached_phctime() which runs via the PTP
auxiliary worker to read a non-NULL but already freed slot, pass its NULL
check, and dereference the freed vport.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index a1c739cdd5406..9c541dd6ed4d4 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -1747,6 +1747,8 @@ void idpf_init_task(struct work_struct *work)
>  	return;
>  
>  unwind_vports:
> +	idpf_ptp_release(adapter);
> +

[Severity: High]
Does this create a race condition that can result in a double free of
adapter->ptp?

If idpf_remove() or a hard reset is triggered, it invokes idpf_vc_core_deinit().

idpf_vc_core_deinit() calls idpf_ptp_release() before it calls
idpf_deinit_task(), which is what cancels this init_task:

drivers/net/ethernet/intel/idpf/idpf_virtchnl.c:idpf_vc_core_deinit() {
    ...
    idpf_ptp_release(adapter);
    idpf_deinit_task(adapter);
    ...
}

Concurrently, if this asynchronous idpf_init_task() fails during
initialization, its error path (unwind_vports) also calls idpf_ptp_release()
without locking or serialization against concurrent teardown.

idpf_ptp_release() performs an unlocked check-and-free:

drivers/net/ethernet/intel/idpf/idpf_ptp.c:idpf_ptp_release() {
    struct idpf_ptp *ptp = adapter->ptp;

    if (!ptp)
        return;
    ...
    kfree(ptp);
    adapter->ptp = NULL;
}

If both threads execute this concurrently, could both read the non-NULL pointer,
leading to concurrent executions of ptp_clock_unregister() and a double free
via kfree(ptp)?

>  	if (default_vport) {
>  		for (index = 0; index < adapter->max_vports; index++) {
>  			if (adapter->vports[index])
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009212537.3996-1-emil.s.tantilov@intel.com?part=2