Re: [PATCH iwl-net v3] ice: Fix "Unknown bps" during link events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ice: data race on bitfields in struct ice_vsi causing state corruption
- [Medium] ice: unsynchronized RMW of phy_info->link_info in ice_link_event()
--

commit 35fcfc2f1fd4d9de2d01f00551ed70ac2dd02bbf
Author: Jakub Kaminski <jakub.kaminski@intel.com>
Subject: ice: Fix "Unknown bps" during link events

The commit refactors the link event flow in the ice driver to use refreshed
link state rather than event values when the status read succeeds. It also
safely caches event values on read failures, introduces ice_update_media_caps(),
and updates ice_print_link_msg() to take the link speed as a parameter.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index c26cef0214552..ac0867ef812a0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -844,8 +844,9 @@ static void ice_print_topo_conflict(struct ice_vsi *vsi)
>   * ice_print_link_msg - print link up or down message
>   * @vsi: the VSI whose link status is being queried
>   * @isup: boolean for if the link is now up or down
> + * @link_speed: current link speed value to display
>   */
> -void ice_print_link_msg(struct ice_vsi *vsi, bool isup)
> +void ice_print_link_msg(struct ice_vsi *vsi, bool isup, u16 link_speed)
>  {

[Severity: High]
Does this function trigger a data race on the bitfields in struct ice_vsi
that can cause state corruption?

The current_isup, irqs_ready, and stat_offsets_loaded bitfields share a
single byte in struct ice_vsi:

	u8 irqs_ready:1;
	u8 current_isup:1;		 /* Sync 'link up' logging */
	u8 stat_offsets_loaded:1;

Compilers implement assignments to these bitfields as non-atomic
read-modify-write (RMW) operations on the shared byte. This function
modifies current_isup locklessly from the ice_service_task workqueue.

Concurrently, ice_vsi_req_irq_msix(), ice_vsi_free_irq(), and
ice_up_complete() modify irqs_ready and stat_offsets_loaded from process
context (e.g., ndo_open/ndo_stop) protected by the RTNL lock. 

A concurrent RMW by the service task can silently overwrite and revert
changes to irqs_ready or stat_offsets_loaded. Reverting irqs_ready to true
after it was set to false during interface teardown can lead to a double-free
of IRQs on the next down operation, or missed interrupts if reverted to false.

[ ... ]

> @@ -1188,23 +1190,32 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up,
>  	old_link = !!(phy_info->link_info_old.link_info & ICE_AQ_LINK_UP);
>  	old_link_speed = phy_info->link_info_old.link_speed;
>  
> +	/* don't drop an event that the refreshed link info no longer shows */
> +	changed = link_up != old_link || link_speed != old_link_speed;
> +
>  	/* update the link info structures and re-enable link events,
>  	 * don't bail on failure due to other book keeping needed
>  	 */
> -	status = ice_update_link_info(pi);
> -	if (status)
> +	err = ice_aq_get_link_info(pi, true, NULL, NULL);
> +	if (!err) {
> +		link_up = phy_info->link_info.link_info & ICE_AQ_LINK_UP;
> +		link_speed = phy_info->link_info.link_speed;
> +		err = ice_update_media_caps(pi);
> +	} else {
> +		/* read failed, keep the cache in sync with the applied state */
> +		if (link_up)
> +			phy_info->link_info.link_info |= ICE_AQ_LINK_UP;
> +		else
> +			phy_info->link_info.link_info &= ~ICE_AQ_LINK_UP;

[Severity: Medium]
Does this code introduce an unprotected read-modify-write operation on the
shared link_info member?

The driver's service task handles link events via ice_link_event(). On this
newly added failure path for ice_aq_get_link_info(), the code performs a
read-modify-write (RMW) operation on a shared structure field:
phy_info->link_info.link_info |= ICE_AQ_LINK_UP;

Concurrently, other execution paths (e.g., ethtool callbacks like
ice_set_link_ksettings() running under the RTNL lock) can trigger
ice_aq_get_link_info(), which unconditionally updates the same
pi->phy.link_info cache without holding any locks.

While the commit message acknowledges that accesses to phy.link_info are not
serialized between the service task and other contexts, adding a new bitwise
RMW operation on the error path increases the risk of tearing the link_info
byte or silently clobbering concurrent state updates.

> +		phy_info->link_info.link_speed = link_speed;
> +	}
> +	if (err)
>  		dev_dbg(dev, "Failed to update link status on port %d, err %d aq_err %s\n",
> -			pi->lport, status,
> +			pi->lport, err,
>  			libie_aq_str(pi->hw->adminq.sq_last_status));
>  
>  	ice_check_link_cfg_err(pf, pi->phy.link_info.link_cfg_err);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016111653.1196653-1-lukasz.czapnik@intel.com?part=1