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?
