Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Critical] iavf: early return in iavf_parse_vf_resource_msg() skips VSI initialization causing panic - [High] core: strict type mismatch in min() macro causing build failure - [High] iavf: division by zero in iavf_fill_rss_lut() from unbounded qregion_width - [High] iavf: RMW data race on adapter->flags leading to lost state updates - [High] iavf: early success return from iavf_parse_vf_resource_msg causes BUG_ON panic - [Medium] iavf: auto-requesting queues on every reset destroys user configuration - [Medium] iavf: ignored ENOMEM from iavf_parse_vf_resource_msg leaves inconsistent RSS state --
commit 30797d207471017781d2b1737aa86bb6b0ea80c5 Author: Ahmed Zaki <[email protected]> iavf: use new opcodes to request more than 16 queues This commit introduces new opcodes to enable the VF to request a variable number of queues (VIRTCHNL_VF_LARGE_NUM_QPAIRS). It defines a capability exchange using VIRTCHNL_OP_GET_MAX_RSS_QREGION and introduces V2 opcodes for enabling/disabling queues and mapping queue vectors. > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c > b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 8059e855dc096..f8193b0cae4ab 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c > @@ -1775,10 +1775,14 @@ int iavf_config_rss(struct iavf_adapter *adapter) > **/ > static void iavf_fill_rss_lut(struct iavf_adapter *adapter) > { > - u16 i; > + struct virtchnl_max_rss_qregion *qregion = &adapter->max_rss_qregion; > + int max = adapter->num_active_queues; > + > + if (LARGE_NUM_QPAIRS_SUPPORT(adapter) && qregion->qregion_width) > + max = min(max, (int)BIT(qregion->qregion_width)); [Severity: High] Is it possible for a malicious or buggy PF to trigger a division by zero here? If qregion_width is >= 32 (on 32-bit systems) or >= 64, the BIT() shift invokes undefined behavior. On 64-bit systems, a width of 32 results in 4294967296, which becomes 0 when cast to int. This makes max = 0 via min(), causing a divide-by-zero panic on the subsequent modulo operation. > > - for (i = 0; i < adapter->rss_lut_size; i++) > - adapter->rss_lut[i] = i % adapter->num_active_queues; > + for (int i = 0; i < adapter->rss_lut_size; i++) > + adapter->rss_lut[i] = i % max; > } [ ... ] > @@ -2486,6 +2493,23 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter > *adapter) > > return -EAGAIN; > } > + /* in LARGE mode num_queue_pairs is the PF limit, not our allocation */ > + if (!num_req_queues && LARGE_NUM_QPAIRS_SUPPORT(adapter) && > + adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_REQ_QUEUES) { > + adapter->current_op = VIRTCHNL_OP_UNKNOWN; > + qnum = min(adapter->vsi_res->num_queue_pairs, > + num_online_cpus()); [Severity: High] Does this code trigger a strict type mismatch build failure? The min() macro requires both arguments to be of the exact same type. Here, adapter->vsi_res->num_queue_pairs is a u16, but num_online_cpus() returns an unsigned int. > + > + return iavf_request_queues(adapter, qnum); [Severity: High] Could returning 0 here (success) instead of -EAGAIN falsely signal the state machine to proceed? If it returns 0 instead of -EAGAIN, the initialization state machine falsely believes parsing completed and advances before the PF can respond. This leads to calling netdev_rss_key_fill() with a length of 0, hitting a BUG_ON. [Severity: Critical] Does this early return skip essential VSI initialization, causing a NULL pointer dereference later? By returning early, critical fields like adapter->vsi.back remain uninitialized (NULL). Once the interface receives packets, a checksum error will crash the kernel in iavf_rx_csum() when it executes vsi->back->hw_csum_rx_error++. [Severity: Medium] Will this unconditionally clear user configuration on every runtime reset? Because adapter->num_req_queues is reset to 0 at the end of this function: adapter->num_req_queues = 0; the initial auto-request condition (!num_req_queues) will evaluate to true on the next normal reset. This causes an unexpected auto-request to the PF, resulting in a double-reset and persistent loss of user queue configurations. > + } > + /* PF could lower the limit on reset, e.g. when RSS LUT got smaller */ > + if (adapter->num_active_queues > adapter->vsi_res->num_queue_pairs) { > + adapter->flags |= IAVF_FLAG_REINIT_MSIX_NEEDED; > + adapter->num_req_queues = adapter->vsi_res->num_queue_pairs; > + iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED); > + > + return -EAGAIN; > + } > adapter->num_req_queues = 0; > adapter->vsi.id = adapter->vsi_res->vsi_id; [ ... ] > @@ -2494,13 +2518,40 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter > *adapter) > vsi->netdev = adapter->netdev; > vsi->qs_handle = adapter->vsi_res->qset_handle; > if (adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) { > - adapter->rss_key_size = adapter->vf_res->rss_key_size; > - adapter->rss_lut_size = adapter->rss_lut_size; > + if ((adapter->rss_key && > + adapter->rss_key_size != adapter->vf_res->rss_key_size) || > + (adapter->rss_lut && > + adapter->rss_lut_size != adapter->vf_res->rss_lut_size)) { > + reconfig_rss = true; > + } else { > + adapter->rss_key_size = adapter->vf_res->rss_key_size; > + adapter->rss_lut_size = adapter->vf_res->rss_lut_size; > + } > } else { > adapter->rss_key_size = IAVF_HKEY_ARRAY_SIZE; > adapter->rss_lut_size = IAVF_HLUT_ARRAY_SIZE; > } > > + if (reconfig_rss) { > + u8 *rss_key, *rss_lut; > + > + rss_key = kmalloc(adapter->vf_res->rss_key_size, GFP_KERNEL); > + rss_lut = kmalloc(adapter->vf_res->rss_lut_size, GFP_KERNEL); > + if (!rss_lut || !rss_key) { > + kfree(rss_key); > + kfree(rss_lut); > + return -ENOMEM; [Severity: Medium] Is it safe to ignore the -ENOMEM return value when this allocation fails? In iavf_virtchnl_completion(), the caller ignores this return value and blindly proceeds to iavf_process_config(). This leaves the driver's RSS state out of sync with the PF, causing silent misconfigurations. > + } > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index 3b9545d2c0217..89d95d3b564ce 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > @@ -602,6 +793,40 @@ void iavf_map_queues(struct iavf_adapter *adapter) > kfree(vimi); > } > > +/** > + * iavf_request_queues - request queues via virtchnl > + * @adapter: adapter structure > + * @num: number of requested queues > + * > + * Return: 0 on success, negative on failure. > + */ > +int iavf_request_queues(struct iavf_adapter *adapter, int num) > +{ > + struct virtchnl_vf_res_request vfres = { num }; > + int err; > + > + if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) { > + /* bail because we already have a command pending */ > + dev_err(&adapter->pdev->dev, "Cannot request queues, command %d > pending\n", > + adapter->current_op); > + return -EBUSY; > + } > + > + adapter->current_op = VIRTCHNL_OP_REQUEST_QUEUES; > + adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED; [Severity: High] Does this non-atomic read-modify-write operation on adapter->flags race with lockless contexts? For example, iavf_tx_timeout() updates this variable from a softirq context: adapter->flags |= flags; If iavf_request_queues() executes concurrently, the updates from the watchdog timer could be lost, preventing the device from recovering from a TX hang. > + adapter->num_req_queues = num; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
