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

["Tantilov, Emil S" <emil.s.tantilov@intel.com>]

On 10/13/2026 9:41 AM, Tantilov, Emil S wrote:
> 
> 
> On 10/9/2026 2:56 PM, sashiko-bot@kernel.org wrote:
>> 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)?

Actually, idpf_init_task() cannot run concurrently with 
idpf_vc_core_deinit().

The init task is only queued from idpf_vc_core_init(), and from itself 
to create the next vport. idpf_vc_core_init() is only called from
idpf_init_hard_reset(), which runs from the event task, on driver load 
as well as on a reset. Once idpf_vc_core_init() succeeds,
idpf_init_hard_reset() waits for the init task to finish:

     while (test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags))
         msleep(100);

The init task clears IDPF_HR_RESET_IN_PROG only in its last run, at the
end of the success path, or as the last statement of the unwind, after
its idpf_ptp_release(). The bit stays set while it re-queues itself.

idpf_remove() and idpf_shutdown() cancel the event task before calling
idpf_vc_core_deinit(), and a hard reset calls it from the event task
itself, whose previous run has already waited for the init task. So
idpf_vc_core_deinit() finds either adapter->ptp intact, or already
released and cleared by the unwind, in which case idpf_ptp_release()
returns early. This patch does not change that: the unwind already
released PTP before clearing IDPF_HR_RESET_IN_PROG, it now does so
before freeing the vports as well.

Two concurrent releases are possible on shutdown, but between two
idpf_vc_core_deinit() calls: idpf_shutdown() does not set
IDPF_REMOVE_IN_PROG, so a Tx timeout or an RDMA reset request can 
re-queue the event task after it was cancelled and start a hard reset 
alongside the shutdown. That is being addressed by different patch, 
pending on -net:
https://lore.kernel.org/intel-wired-lan/ca88d67c-1595-4bd7-9560-080de7104e6d@intel.com/T/#u

which sets the bit in idpf_shutdown() as in idpf_remove().

>>
>>>       if (default_vport) {
>>>           for (index = 0; index < adapter->max_vports; index++) {
>>>               if (adapter->vports[index])
>> [ ... ]
>>
> 
> I will address it in v2.

Correction, I think this series can stay as is with the one hit above 
being dealt with in a patch in the "destroy vport on shutdown" series:
https://lore.kernel.org/intel-wired-lan/20261013121224.84482-1-luckilystar08@gmail.com/T/#t

Sorry for the confusion,
Emil