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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Symptom-only fix: when `i40e_rebuild()` aborts because the
  main VSI could not be re-created, the PTP subsystem is left in an…

--

i40e: avoid null ptr dereference in i40e_ptp_stop()

If the main VSI cannot be re-created during a reset, i40e_rebuild() aborts
after i40e_vsi_clear() has freed the main VSI, so a later i40e_remove()
calls i40e_ptp_stop() with pf->vsi[pf->lan_vsi] == NULL.  The patch adds a
main_vsi NULL check around the dev_info() that dereferences
main_vsi->netdev->name.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ptp.c b/drivers/net/ethernet/intel/i40e/i40e_ptp.c
> index ff62b5f2c815..ca93df4d6785 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: Medium]

Is guarding the log message here enough, or does it just make the window
between the failed rebuild and the eventual remove survivable?

In that window nothing appears to touch PTP at all.  i40e_ptp_stop() has
only two callers, i40e_probe() (error path) and i40e_remove(), and none of
the abort paths in i40e_rebuild() (end_unlock / end_core_reset /
clear_recovery) call i40e_ptp_init() or i40e_ptp_stop():

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_setup_pf_switch() {
	...
		if (!main_vsi) {
			dev_info(&pf->pdev->dev, "setup of MAIN VSI failed\n");
			i40e_cloud_filter_exit(pf);
			i40e_fdir_teardown(pf);
			return -EAGAIN;
		}
	...
}

Across the reset, the only PTP action is i40e_prep_for_reset() calling
i40e_ptp_save_hw_time(), which is a pure read.

So after the abort, pf->ptp_clock stays registered and I40E_FLAG_PTP_ENA,
pf->ptp_tx, pf->ptp_rx and pf->tstamp_config stay set, even though the main
VSI and its netdev were freed, and the register programming that
i40e_ptp_init() normally redoes after a reset never runs:

drivers/net/ethernet/intel/i40e/i40e_ptp.c:i40e_ptp_init() {
	...
		regval = rd32(hw, I40E_PRTTSYN_CTL0);
		regval |= I40E_PRTTSYN_CTL0_TSYNENA_MASK;
		wr32(hw, I40E_PRTTSYN_CTL0, regval);
	...
}

i40e_ptp_set_increment(), i40e_ptp_set_timestamp_mode(),
i40e_ptp_restore_hw_time() and i40e_ptp_set_1pps_signal_hw() are skipped in
the same way, and i40e_ptp_init()'s own kerneldoc describes it as
reconfiguring the device after a reset.

Does that leave userspace with an openable /dev/ptpN whose gettime/settime/
adjfine operate on a timesync block the reset left disabled, and PF flags
still advertising timestamping that hardware will not deliver?  A Tx
timestamp request in that state would set __I40E_PTP_TX_IN_PROGRESS for a
timestamp that never arrives, recovered only by the watchdog.

Would it be better to quiesce PTP on the rebuild-failure path, for example
unregistering the PHC or at least clearing I40E_FLAG_PTP_ENA, rather than
adding a NULL check per consumer?  As written, every future user of the main
VSI or of the PTP state in this window needs its own check.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918212458.550425-1-anthony.l.nguyen%40intel.com