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 <[email protected]> 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/[email protected]?part=1
