Re: [PATCH v5 net 2/7] i40e: avoid null ptr dereference in i40e_ptp_stop()

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jul 08, 2026 at 04:04:56PM +0100, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> i40e: avoid null ptr dereference in i40e_ptp_stop()
>     
> Check if main_vsi is not null before calling dev_info() to avoid a null
> pointer dereference when i40e_vsi_clear() frees the main VSI during rebuild.
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > index ff62b5f2c8150..ca93df4d67859 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> > @@ -1556,8 +1556,9 @@ void i40e_ptp_stop(struct i40e_pf *pf)
> >  	if (pf->ptp_clock) {
> >  		ptp_clock_unregister(pf->ptp_clock);
> >  		pf->ptp_clock = NULL;
> > -		dev_info(&pf->pdev->dev, "%s: removed PHC on %s\n", __func__,
> > -			 main_vsi->netdev->name);
> > +		if (main_vsi)
> > +			dev_info(&pf->pdev->dev, "%s: removed PHC on %s\n", __func__,
> > +				 main_vsi->netdev->name);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but this lockless check appears
> to introduce a time-of-check to time-of-use race condition.

Thanks Simon I saw these, however since all of the issues are classified
as pre-existing issues IMHO these changes are okay to be taken and then
we could go with another round of fixes.

> 
> When i40e_remove() calls i40e_ptp_stop(pf), it does so unsynchronized from
> the background i40e_rebuild() task. If i40e_rebuild() fails during setup, it
> calls i40e_vsi_clear(main_vsi), which frees the VSI and its netdev.
> 
> If i40e_ptp_stop() caches main_vsi right before it is cleared, couldn't the
> if (main_vsi) check pass, but the subsequent main_vsi->netdev->name access
> dereference freed memory, causing a use-after-free?
> 
> [Severity: High]
> This is another pre-existing issue, but I noticed that i40e_ptp_stop() doesn't
> seem to cancel the background workqueue task ptp_extts0_work.
> 
> During device removal, i40e_remove() calls i40e_ptp_stop() and then
> subsequently frees the i40e_pf structure. If the work executes after the PF
> is freed, will it access the deallocated i40e_pf structure and trigger a
> use-after-free?
> 
> [Severity: High]
> This is also a pre-existing issue, but I am looking at how pf->ptp_tx_skb is
> cleared earlier in i40e_ptp_stop(). The teardown uses I40E_FLAG_PTP_ENA as a 
> gate guard:
> 
> i40e_ptp_stop() clears I40E_FLAG_PTP_ENA, then locklessly checks and frees
> pf->ptp_tx_skb using dev_kfree_skb_any().
> 
> The interrupt handler i40e_ptp_tx_hwtstamp() uses the same bit as a guard:
> 
>     if (!test_bit(I40E_FLAG_PTP_ENA, pf->flags))
>             return;
> 
> If the interrupt handler passes the test_bit check just before i40e_ptp_stop()
> executes clear_bit(), both contexts could concurrently read the same
> pf->ptp_tx_skb pointer into local variables before it is NULLed out. Could
> this result in a double free of the SKB?
> 
> [Severity: High]
> There is a similar pre-existing issue related to the teardown of the
> timestamping state in i40e_ptp_stop() racing with the TX datapath.
> 
> The TX datapath i40e_tstamp() protects the state using a bit lock:
> 
>     if (pf->ptp_tx &&
>         !test_and_set_bit_lock(__I40E_PTP_TX_IN_PROGRESS, pf->state)) {
>             ...
>             pf->ptp_tx_skb = skb_get(skb);
>     }
> 
> However, i40e_ptp_stop() checks pf->ptp_tx_skb locklessly and only clears
> the bit lock if the SKB was present. If the TX datapath acquires the lock
> and is preempted right before assigning pf->ptp_tx_skb, i40e_ptp_stop()
> might execute, see a NULL ptp_tx_skb, and skip the cleanup block.
> 
> When the TX datapath resumes, it assigns the SKB pointer. Since the device
> is subsequently destroyed, does this cause the SKB to be permanently leaked?
> 
> >  	}
> >  
> >  	if (i40e_is_ptp_pin_dev(&pf->hw)) {
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing the driver's
> cleanup paths, I noticed a potential leak in the i40e_probe() error handling:
> 
> drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_probe() {
> ...
> err_vsis:
>         set_bit(__I40E_DOWN, pf->state);
>         i40e_ptp_stop(pf);
>         i40e_clear_interrupt_scheme(pf);
>         kfree(pf->vsi);
> ...
> }
> 
> If initialization fails after VSIs are successfully created, this cleanup
> path calls kfree(pf->vsi) which frees the pointer array, but does not appear
> to unregister the netdev or free the actual VSI structs and hardware rings.
> 
> Could this leave an active, registered netdev with dangling pointers to the
> subsequently freed PF, potentially triggering a use-after-free upon network
> traffic or userspace access?
> 
> [Severity: High]
> This is a pre-existing issue, but I noticed an ad-hoc locking pattern in
> i40e_remove():
> 
> drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_remove() {
> ...
>         while (test_and_set_bit(__I40E_RESET_RECOVERY_PENDING, pf->state))
>                 usleep_range(1000, 2000);
> ...
> }
> 
> This attempts to synchronize with the background reset path by spinning on
> a bit flag, which acts as a sleepable lock without the standard fairness or
> deadlock detection guarantees. 
> 
> Since this ad-hoc locking scheme fails to provide proper mutual exclusion
> earlier in the function, could this be the direct cause of why i40e_ptp_stop()
> executes unsynchronized from i40e_rebuild()? Should a proper mutex be used
> here instead?