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

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

On Tue, Sep 22, 2026 at 01:31:06AM +0000, netdev-bot+sashiko@kernel.org wrote:
> 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.

I didn't want to touch ptp initially and this commit was rather to satisfy
sashiko (which was successful!) and here the broader concern is being
raised and I suppose I won't escape from addressing this :<

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