Re: [PATCH net 6/8] i40e: do not expose netdev too early

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The patch narrows but does not close the window it claims to
  close for the driver's primary netdev.
- [Low] Dead error-path code left behind by the reordering: the block at
  i40e_main.c:14498-14502 (`err_rings: if (vsi->netdev_registered) {…
- [Low] Two pieces of externally visible netdev state are still installed
  after registration, contradicting the 'publish late' invariant the…

--

i40e: do not expose netdev too early

This moves register_netdev() in i40e_vsi_setup() from the middle of the
netdev setup case to the end of the function, after ring and q_vector
allocation, ring-to-vector mapping and VMDQ2 RSS configuration.  The
error path is reworked so an allocated but unregistered netdev is still
freed.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 2b4b061302db..82037faf960f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -14449,15 +14449,6 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
>  				goto err_netdev;
>  			SET_NETDEV_DEVLINK_PORT(vsi->netdev, &pf->devlink_port);
>  		}
> -		ret = register_netdev(vsi->netdev);
> -		if (ret)
> -			goto err_dl_port;
> -		vsi->netdev_registered = true;
> -		netif_carrier_off(vsi->netdev);
> -#ifdef CONFIG_I40E_DCB
> -		/* Setup DCB netlink interface */
> -		i40e_dcbnl_setup(vsi);
> -#endif /* CONFIG_I40E_DCB */
>  		fallthrough;
>  	case I40E_VSI_FDIR:
>  		/* set up vectors and rings if needed */
> @@ -14485,6 +14476,19 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
>  		if (ret)
>  			goto err_config;
>  	}
> +
> +	if (vsi->netdev) {
> +		ret = register_netdev(vsi->netdev);
> +		if (ret)
> +			goto err_config;
> +		vsi->netdev_registered = true;

[Severity: High]
For the MAIN VSI, does this really close the window described in the
commit message?  register_netdev() is still called from inside
i40e_vsi_setup(), which runs from
i40e_probe() -> i40e_setup_pf_switch() -> i40e_vsi_setup().

After i40e_vsi_setup() returns for the MAIN VSI, i40e_setup_pf_switch()
still programs PF-wide state the datapath depends on:

	i40e_vlan_stripping_disable(main_vsi);
	i40e_fdir_sb_setup(pf);
	ret = i40e_setup_pf_filter_control(pf);
	...
	if (test_bit(I40E_FLAG_RSS_ENA, pf->flags))
		i40e_pf_config_rss(pf);
	i40e_link_event(pf);
	i40e_ptp_init(pf);

Note that only VMDQ2 gets RSS configured inside i40e_vsi_setup():

	if (test_bit(I40E_HW_CAP_RSS_AQ, pf->hw.caps) &&
	    vsi->type == I40E_VSI_VMDQ2) {
		ret = i40e_vsi_config_rss(vsi);

so the MAIN netdev is published before PFQF_HENA/HKEY/HLUT, PFQF_CTL_0 and
the VLAN strip configuration are written.  Back in i40e_probe(),
__I40E_DOWN is cleared and i40e_setup_misc_vector() arms the ICR0 admin
queue vector only after i40e_setup_pf_switch() has returned.

i40e_open() gates on:

	if (test_bit(__I40E_TESTING, pf->state) ||
	    test_bit(__I40E_BAD_EEPROM, pf->state))
		return -EBUSY;

which does not test __I40E_DOWN or any probe-complete state, and
register_netdev() drops RTNL before returning while probe holds no lock
that blocks rtnetlink.  Can a concurrent "ip link set up" (udev or
NetworkManager reacting to the freshly published ifindex) therefore reach
i40e_vsi_open() -> i40e_up_complete() while the PF's global hardware state
is still being programmed and while i40e_intr() still refuses to service
ICR0 because __I40E_DOWN is set?

There is also the probe failure case.  i40e_setup_pf_switch() returns the
(otherwise non-fatal) i40e_setup_pf_filter_control() error, and a
i40e_setup_misc_vector() failure jumps to err_vsis:

	err_vsis:
		set_bit(__I40E_DOWN, pf->state);
		i40e_ptp_stop(pf);
		i40e_clear_interrupt_scheme(pf);
		kfree(pf->vsi);

with no unregister_netdev() anywhere on that path, and err_switch_setup
then frees the PF itself.  Does this leave an already registered, possibly
already opened netdev in the system whose netdev_priv()->vsi->back points
at freed memory, so a later ndo_open, ndo_get_stats64, ethtool or close
touches freed q_vectors and a freed struct i40e_pf?

Would registering the netdev only once i40e_probe() has completed, or
gating ndo_open on a "PF ready" state, be a more complete fix than moving
registration a few statements later inside i40e_vsi_setup()?

> +		netif_carrier_off(vsi->netdev);
> +#ifdef CONFIG_I40E_DCB
> +		/* Setup DCB netlink interface */
> +		i40e_dcbnl_setup(vsi);
> +#endif /* CONFIG_I40E_DCB */
> +	}

[Severity: Low]
Since the commit declares the "publish late" invariant, should these two
steps move ahead of register_netdev() as well?

register_netdev() emits NETDEV_REGISTER/RTM_NEWLINK and drops RTNL before
returning, while alloc_etherdev_mq() leaves __LINK_STATE_NOCARRIER clear.
Can the initial link notification therefore advertise IFF_LOWER_UP, with a
carrier-down linkwatch event following immediately once
netif_carrier_off() runs?  netif_carrier_off() sets the bit before its
early return:

net/sched/sch_generic.c:netif_carrier_off() {
	if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
		if (dev->reg_state == NETREG_UNINITIALIZED)
			return;
	...
}

so it looks intended to be callable before registration.

Similarly, i40e_dcbnl_setup() installs the ops and only then populates the
state:

drivers/net/ethernet/intel/i40e/i40e_dcb_nl.c:i40e_dcbnl_setup() {
	dev->dcbnl_ops = &dcbnl_ops;

	/* Set initial IEEE DCB settings */
	i40e_dcbnl_set_all(vsi);
}

Does a DCB request arriving between register_netdev() and the dcbnl_ops
assignment get -EOPNOTSUPP from dcb_doit() on a DCB-capable port:

net/dcb/dcbnl.c:dcb_doit() {
	if (!netdev->dcbnl_ops)
		return -EOPNOTSUPP;
	...
}

and is there a second window where dcbnl_ops is live but
i40e_dcbnl_set_all() has not run yet?  Assigning dcbnl_ops and clearing
carrier before register_netdev(), leaving only i40e_dcbnl_set_all()
afterwards, would avoid both windows.

> +
>  	return vsi;
>  
>  err_config:
> @@ -14495,13 +14499,14 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
>  	if (vsi->netdev_registered) {
>  		vsi->netdev_registered = false;
>  		unregister_netdev(vsi->netdev);
> -		free_netdev(vsi->netdev);
> -		vsi->netdev = NULL;
>  	}

[Severity: Low]
Is this err_rings block still reachable after the reordering?

i40e_vsi_mem_alloc() sets vsi->netdev_registered = false for every VSI, and
after this patch the only assignment of true sits after every
goto err_config/err_qvec/err_rings/err_netdev in i40e_vsi_setup(), followed
only by netif_carrier_off(), i40e_dcbnl_setup() and "return vsi".  If
register_netdev() itself fails the flag is still false, so the condition
looks always false when the unwind path runs.

Would dropping it be clearer, so a later patch adding a step after
registration does not assume the unwind already deregisters the netdev?

> -err_dl_port:
>  	if (vsi->type == I40E_VSI_MAIN)
>  		i40e_devlink_destroy_port(pf);
>  err_netdev:
> +	if (vsi->netdev) {
> +		free_netdev(vsi->netdev);
> +		vsi->netdev = NULL;
> +	}
>  	i40e_aq_delete_element(&pf->hw, vsi->seid, NULL);
>  err_vsi:
>  	i40e_vsi_clear(vsi);

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