Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [Critical] ice: Out-of-bounds array access on fixed-size devlink resource tracking arrays - [High] ice: Multi-NAC E825C device probe failure due to shared devlink resource collision - [High] ice: Build error due to undefined macro/function `kzalloc_obj()` - [High] ice: Data race and potential use-after-free in `ice_vsi_decfg` due to unlocked VSI RSS state modification - [Medium] ice: VSI rebuilds silently revert Devlink Global LUT configuration to defaults - [Low] ice: Missing fallback return statements after `scoped_guard` blocks cause -Wreturn-type build errors --
commit 1692bc7b621e54d0bc3517380fe13e034d0d404f Author: Przemek Kitszel <[email protected]> Subject: ice: represent RSS LUTs as devlink resources This commit introduces VSI, GLOBAL, and PF RSS LUTs for the E800 family, exposing them as devlink resources. It enables the PF to switch between PF LUT and GLOBAL LUT and allows configuration sizing through devlink. > diff --git a/drivers/net/ethernet/intel/ice/devlink/resource.c > b/drivers/net/ethernet/intel/ice/devlink/resource.c > new file mode 100644 > index 0000000000000..09672da3ad393 > --- /dev/null > +++ b/drivers/net/ethernet/intel/ice/devlink/resource.c > @@ -0,0 +1,527 @@ [ ... ] > +static int ice_devl_res_take(struct ice_pf *pf, > + enum ice_devl_resource_id res_id, int slot, > + void *owner) > +{ > + struct ice_devl_resource *res = &pf->adapter->resources[res_id]; > + int beg, end, new_id = ICE_ANY_SLOT; > + u16 lut_id; > + > + if (res_id == ICE_RSS_LUT_GLOBAL) { [ ... ] > + end = slot == ICE_ANY_SLOT ? res->max_size : slot + 1; > + beg = slot == ICE_ANY_SLOT ? 0 : slot; > + for (int id = beg; id < end; id++) { > + if (!res->owner[id]) { > + new_id = id; > + break; > + } > + } > + if (new_id == ICE_ANY_SLOT) { > + if (WARN_ON(res_id == ICE_RSS_LUT_GLOBAL)) > + ice_free_rss_global_lut(&pf->hw, lut_id); > + > + return -ENOSPC; > + } > + > + res->owner[new_id] = owner; > + res->pf_id[new_id] = pf->hw.logical_pf_id; [Severity: Critical] Could this lead to an out-of-bounds access? The res->owner and res->pf_id arrays are sized ICE_MAX_DEVL_RESOURCE_UNITS (16 elements). If the firmware provides a lut_id >= 16, or an adapter has more than 16 physical functions, it seems we might read and write outside the array boundaries. > + return new_id; > +} [ ... ] > +static u64 ice_rss_lut_pf_occ_get_global(void *priv) > +{ > + struct ice_adapter *adapter; > + struct ice_pf *pf = priv; > + > + adapter = pf->adapter; > + scoped_guard(ice_adapter_devl, adapter) > + return ice_is_devl_res_owned_by(adapter, ICE_RSS_LUT_GLOBAL, > pf); > +} [Severity: Low] Is there a missing return statement here? The scoped_guard() block is implemented as a loop, and compilers (like older GCCs) may not statically prove it executes exactly once. This might cause a -Werror=return-type warning. [ ... ] > +int ice_take_rss_lut_pf(struct ice_pf *pf) > +{ > + int ret, pf_id = pf->hw.logical_pf_id; > + > + scoped_guard(ice_adapter_devl, pf->adapter) { > + ret = ice_devl_res_take(pf, ICE_RSS_LUT_PF, pf_id, pf); > + if (ret >= 0) > + return 0; > + > + return ret; > + } > +} [Severity: High] Could this cause a collision on multi-NAC devices like the E825C? These devices share a single ice_adapter and devlink instance. Since logical_pf_id resets per NAC (e.g. the first PF of the second NAC also has logical_pf_id == 0), ice_devl_res_take() might find res->owner[0] already taken by the first NAC and return -ENOSPC. [Severity: Low] Additionally, is there a missing return statement after the scoped_guard() block here as well, similar to the warning in ice_rss_lut_pf_occ_get_global? [ ... ] > +void ice_devl_whole_dev_resources_register(const struct ice_hw *hw, > + struct ice_adapter *adapter) > +{ > + struct devlink *devlink = adapter->devlink; > + int pf_lut_cnt = hw->dev_caps.num_funcs; > + > + devl_assert_locked(devlink); > + > + adapter->resources[ICE_RSS_LUT_GLOBAL] = (struct ice_devl_resource) { > + .name = "lut_512", > + .parent_id = ICE_RSS_LUT_BOTH, > + .max_size = ICE_NUM_GLOBAL_LUTS, > + .get = ice_rss_lut_whole_dev_occ_get_global, > + .set = ice_devl_resource_deny_occ_set, > + }; > + adapter->resources[ICE_RSS_LUT_PF] = (struct ice_devl_resource) { > + .name = "lut_2048", > + .parent_id = ICE_RSS_LUT_BOTH, > + .max_size = pf_lut_cnt, [Severity: High] Since pf_lut_cnt is based on the first initializing NAC's function count, does this fail to account for the total PFs across the entire adapter for multi-NAC devices? > + .get = ice_rss_lut_whole_dev_occ_get_pf, > + .set = ice_devl_resource_deny_occ_set, > + }; [ ... ] > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c > b/drivers/net/ethernet/intel/ice/ice_lib.c > index f81b310a98c3e..ad43d52ea83cb 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c [ ... ] > @@ -1306,6 +1311,32 @@ static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx > *ctxt, struct ice_vsi *vsi) > FIELD_PREP(ICE_AQ_VSI_Q_OPT_RSS_HASH_M, hash_type); > } > > +int ice_vsi_update_rss_lut(struct ice_vsi *vsi, enum ice_lut_type lut_type, > + u8 global_lut_id) > +{ > + struct ice_vsi_ctx *ctx; > + int err; > + > + ctx = kzalloc_obj(*ctx); [Severity: High] Is kzalloc_obj() defined? It looks like this might cause a compilation failure because it's not a standard kernel macro or declared in this series. > + if (!ctx) > + return -ENOMEM; > + > + ctx->info.valid_sections = cpu_to_le16(ICE_AQ_VSI_PROP_Q_OPT_VALID); [ ... ] > @@ -2427,6 +2464,18 @@ static int ice_vsi_cfg_def(struct ice_vsi *vsi) > > vsi->vsw = pf->first_sw; > > + if (vsi->flags & ICE_VSI_FLAG_INIT) { > + scoped_guard(mutex, &pf->rss_lut_lock) { > + u16 lut_size = vsi->rss_table_size; > + > + ice_vsi_set_dflt_rss_params(vsi); [Severity: Medium] Will this unconditionally hardcode the PF LUT type and silently discard the user's active devlink Global LUT configuration during VSI rebuilds? > + if (vsi->rss_table_size != lut_size) { > + devm_kfree(dev, vsi->rss_lut_user); > + vsi->rss_lut_user = NULL; > + } > + } > + } [ ... ] > @@ -2634,15 +2694,16 @@ void ice_vsi_decfg(struct ice_vsi *vsi) > ice_vsi_put_qs(vsi); > ice_vsi_free_arrays(vsi); > > - /* SR-IOV determines needed MSIX resources all at once instead of per > - * VSI since when VFs are spawned we know how many VFs there are and how > - * many interrupts each VF needs. SR-IOV MSIX resources are also > - * cleared in the same manner. > - */ > - > - if (vsi->type == ICE_VSI_VF && > - vsi->agg_node && vsi->agg_node->valid) > - vsi->agg_node->num_vsis--; > + if (vsi->flags & ICE_VSI_FLAG_INIT) { > + if (vsi->type == ICE_VSI_PF) { > + ice_free_rss_lut_flr(pf); > + /* 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); [Severity: High] Could this be a data race or use-after-free? We are modifying vsi->rss_lut_type and freeing vsi->rss_lut_user outside of the pf->rss_lut_lock mutex. If this runs asynchronously during VSI rebuilds, it might free vsi->rss_lut_user while another thread (like in ice_maybe_change_rss_lut() or ice_set_rxfh()) expects the pointer to be valid. > + vsi->rss_lut_user = NULL; > + } > + } > + } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=13
