Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ice: orphaned channel VSIs and memory leak when ice_setup_tc() fails teardown --
commit 58934f25b30092ac8064d34365ba48edb6a32a06 Author: Petr Oros <[email protected]> ice: do not block on the aux device lock in ndo_setup_tc This commit replaces device_lock() with device_trylock() on the RDMA auxiliary device in the TC_SETUP_QDISC_MQPRIO path of ice_setup_tc() to avoid a lock dependency cycle with the RTNL lock, returning -EBUSY when the lock is contended. > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index 9de375f5c4deb..7eb653a7ebacd 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -9451,7 +9451,12 @@ ice_setup_tc(struct net_device *netdev, enum > tc_setup_type type, > mutex_lock(&pf->adev_mutex); > adev = cdev->adev; > if (adev) { > - device_lock(&adev->dev); > + if (!device_trylock(&adev->dev)) { > + mutex_unlock(&pf->adev_mutex); > + netdev_warn(netdev, "RDMA auxiliary > device is busy, qdisc not changed\n"); > + err = -EBUSY; > + goto tc_unlock; > + } [Severity: High] Can failing with -EBUSY during qdisc removal permanently leak the struct ice_channel entries on vsi->ch_list and leave channel VSI state orphaned? The commit message notes that when the qdisc is deleted, mqprio_disable_offload() ignores the return value so the hardware traffic classes and channel VSIs stay in place until a later qdisc teardown gets through. However, in net/sched/sch_mqprio.c, mqprio_destroy() calls mqprio_disable_offload() once while destroying the Qdisc: net/sched/sch_mqprio.c:mqprio_destroy() { ... if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc) mqprio_disable_offload(sch); else netdev_set_num_tc(dev, 0); } Once mqprio_destroy() finishes, the Qdisc object is freed and no mqprio Qdisc remains attached to the net_device, so no subsequent teardown call will happen for it (including on unregister_netdev()). During driver unload (ice_remove() -> ice_unload() -> ice_deinit() -> ice_vsi_release_all()), ice_deinit_pf_sw() releases the main VSI without calling ice_remove_q_channels(), and ice_vsi_release_all() skips ICE_VSI_CHNL VSIs: drivers/net/ethernet/intel/ice/ice_main.c:ice_vsi_release_all() { ... if (pf->vsi[i]->type == ICE_VSI_CHNL) continue; err = ice_vsi_release(pf->vsi[i]); ... } Moreover, if a user later adds a new offloaded mqprio qdisc (hw != 0) to attempt another teardown, ice_setup_tc_mqprio_qdisc() only removes existing channels when disabling offload: drivers/net/ethernet/intel/ice/ice_main.c:ice_setup_tc_mqprio_qdisc() { ... if (!hw && !test_bit(ICE_FLAG_TC_MQPRIO, pf->flags)) ice_remove_q_channels(vsi, true); ... if (hw) { ... ret = ice_create_q_channels(vsi); ... } } Wouldn't adding a new offloaded mqprio qdisc leave the old struct ice_channel entries on vsi->ch_list, rebuild the main VSI rings, and append duplicate channels via ice_create_q_channels(), potentially indexing past vsi->tx_rings[] and vsi->rx_rings[] in ice_remove_q_channels() if the new qdisc configures fewer queues? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
