Re: [PATCH iwl-net v2 5/5] ice: do not block on the aux device lock in ndo_setup_tc

[sashiko-bot]

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 <poros@xxxxxxxxxx>

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/20261023070258.730168-1-poros@xxxxxxxxxx?part=5