Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] ice: AB-BA deadlock between PF devlink lock and vfs.table_lock - [High] ice: Hardware out-of-bounds queue programming due to bypassed VF rss_size clamping - [Medium] ice: Devlink resource wipe on PF VSI rebuild breaks VF isolation - [Medium] ice: Indefinite deferral of VF resets due to missing service task wakeup --
commit f0498e5b28b0f370443f9612e9323e5875839c0d Author: Przemek Kitszel <[email protected]> Subject: ice: support up to 256 VF queues This commit adds support for configuring up to 256 VF queues by allowing the devlink assignment of GLOBAL or PF RSS LUTs to VFs. It introduces a devlink instance for VFs to track RSS LUT resources under the PF devlink instance. > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c > b/drivers/net/ethernet/intel/ice/ice_lib.c > index ad43d52ea83cb..43c8bfedbea23 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c [ ... ] > @@ -1663,7 +1680,9 @@ int ice_vsi_cfg_rss_lut_key(struct ice_vsi *vsi) > (test_bit(ICE_FLAG_TC_MQPRIO, pf->flags))) { > vsi->rss_size = min_t(u16, vsi->rss_size, vsi->ch_rss_size); > } else { > - vsi->rss_size = min_t(u16, vsi->rss_size, vsi->num_rxq); > + /* VF rss_size is the queue capacity of its LUT, keep it */ > + if (vsi->type != ICE_VSI_VF) > + vsi->rss_size = min_t(u16, vsi->rss_size, vsi->num_rxq); [Severity: High] Could bypassing this clamp cause hardware out-of-bounds queue programming? When a GLOBAL LUT or PF LUT is assigned to a VF via devlink, vsi->rss_size is updated to the maximum capacity of the LUT (such as 64 or 256), and a deferred VF reset is scheduled. During the VF rebuild, this change intentionally bypasses clamping vsi->rss_size to vsi->num_rxq for VFs. If the VF only has its default number of queues allocated (for instance, 4) but the hardware RSS LUT is populated with queue indices up to 63 or 255, will the hardware steer incoming packets to unallocated queues? This seems likely to trigger fatal Malicious Driver Detection (MDD) events or silent packet blackholing. [ ... ] > @@ -2697,11 +2716,11 @@ void ice_vsi_decfg(struct ice_vsi *vsi) > if (vsi->flags & ICE_VSI_FLAG_INIT) { > if (vsi->type == ICE_VSI_PF) { > ice_free_rss_lut_flr(pf); [Severity: Medium] Does this unintentionally wipe the devlink resource tracking for VFs without reprogramming the hardware? When a PF VSI is rebuilt (for example, during an MTU change or via ethtool -L), ice_vsi_decfg() calls ice_free_rss_lut_flr(). That function clears the software devlink owner state for all GLOBAL LUTs assigned to VFs of this PF. However, it does not reprogram the VF hardware or issue a VF reset. The VFs will continue to use the LUTs in hardware while the devlink manager believes they are free. If devlink assigns the freed LUT to a different VF, could this result in shared queue mappings that break VM network isolation and corrupt traffic routing? > - /* reset restores PF LUT, user LUT has wrong size */ > - if (vsi->rss_lut_type != ICE_LUT_PF) { > - devm_kfree(ice_pf_to_dev(pf), > vsi->rss_lut_user); > - vsi->rss_lut_user = NULL; > - } > + } else if (vsi->type == ICE_VSI_VF) { > + struct ice_vf *vf = vsi->vf; > + > + vf->num_req_qs = 0; > + vf->num_vf_qs = min(vf->num_vf_qs, pf->vfs.num_qps_per); > } > } > } [ ... ] > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > index 320b393ddaaf0..6bc71cdc624c7 100644 > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c [ ... ] > @@ -1010,6 +1011,18 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags) > return err; > } > > +/** > + * ice_schedule_vf_reset - reset VF, deferred to next service_task context > + * @vf: VF to reset > + */ > +void ice_schedule_vf_reset(struct ice_vf *vf) > +{ > + set_bit(ICE_VF_STATE_NEEDS_RESET, vf->vf_states); > + /* pairs with test_and_clear_bit() in ice_handle_deferred_vf_reset() */ > + smp_mb__after_atomic(); > + set_bit(ICE_VF_RESET_PENDING, vf->pf->state); > +} [Severity: Medium] Is there a missing explicit wakeup for the service task here? The ICE_VF_RESET_PENDING bit is set to schedule a deferred reset. However, the service task loop in ice_service_task() does not include ICE_VF_RESET_PENDING in the list of flags that keeps its timer active. If the system is otherwise idle, wouldn't this delay the reset indefinitely until an unrelated event triggers the service task? This could create a race condition where subsequent user configuration commands fail because the VF has not yet applied the prior devlink resource changes. [ ... ] > @@ -1450,3 +1466,44 @@ void ice_vf_update_mac_lldp_num(struct ice_vf *vf, > struct ice_vsi *vsi, > if (was_ena != is_ena) > ice_vsi_cfg_sw_lldp(vsi, false, is_ena); > } > + > +void ice_init_vf_devlink(struct ice_vf *vf) > +{ > + struct devlink *pf_devlink = priv_to_devlink(vf->pf); > + static const struct devlink_ops noop = {}; > + struct devlink *devlink; > + int err; > + > + lockdep_assert_held(&vf->pf->vfs.table_lock); > + > + devlink = devlink_alloc(&noop, 0, &vf->vfdev->dev); > + if (!devlink) > + return; > + > + scoped_guard(devl, pf_devlink) [Severity: High] Does this locking sequence create an AB-BA deadlock with devlink callbacks? Here, pf->vfs.table_lock is already held (as asserted above), and we acquire the PF devlink instance lock via scoped_guard(devl, pf_devlink). However, in devlink callbacks like ice_tear_down_devlink_rate_tree(), the PF devlink instance lock is acquired first, followed by mutex_lock(&pf->vfs.table_lock). If SR-IOV configuration runs concurrently with a devlink rate tree teardown, could this inverted locking order cause a deadlock? > + err = devl_nested_devlink_set(pf_devlink, devlink); > + if (err) { > + devlink_free(devlink); > + return; > + } > + > + vf->devlink = devlink; > + devlink_register(devlink); > + > + ice_devlink_vf_resources_register(vf); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
