[PATCH iwl-net v3 0/4] ice: fix RTNL vs auxiliary device lock inversion

[PATCH iwl-net v3 0/4] ice: fix RTNL vs auxiliary device lock inversion

[Petr Oros]

ice takes the auxiliary device lock of its RDMA auxiliary device from
three places that run with the RTNL lock held. The RDMA auxiliary probe
takes the two in the opposite order, because __driver_attach() holds the
auxiliary device lock across ib_register_device(), which needs the RTNL
lock. The result is a hard deadlock of everything that needs the RTNL
lock, so the boot never completes:

  systemd-udevd   state:D
    really_probe                  <- holds device_lock(&adev->dev)
    icrdma_probe
    irdma_ib_register_device
    ib_register_device
    setup_device
    irdma_roce_port_immutable
    ib_query_port
    irdma_query_port
    ib_get_eth_speed              <- blocks on rtnl_lock()

  lldpad          state:D
    rtnetlink_rcv_msg             <- holds rtnl_lock()
    dcb_doit
    dcbnl_ieee_set
    ice_dcbnl_setets
    ice_pf_dcb_cfg
    ice_send_event_to_aux         <- blocks on device_lock(&adev->dev)

Lockdep cannot flag it, because device_initialize() marks dev->mutex
with lockdep_set_novalidate_class(). A PREEMPT_RT kernel with
CONFIG_DEBUG_RT_MUTEXES reports "rtmutex deadlock detected" instead.

The probe side cannot be changed, so the three ice callers use
device_trylock() and refuse the operation with -EBUSY when the driver
core holds the lock. Patch 1 fixes a separate race on the same sites,
the auxiliary device pointer was tested before pf->adev_mutex was taken.
Patch 2 keeps the locks across the whole DCB reconfiguration so that
BEFORE_TC_CHANGE and AFTER_TC_CHANGE are either both delivered or the
request is refused before anything changes, and re-arms the service
task events. Patches 3 and 4 turn the remaining two locks into trylocks.

The series is based on the dev-queue branch of net-queue, which carries
Przemek's "ice: fix DCB lock ordering" [1]. That series takes the RTNL
lock before pf->tc_mutex and then drops the mutex, so the lock order
patch of v2 is gone and ndo_setup_tc() has no mutex to release on the
new error path.

[1]
https://lore.kernel.org/all/20261102105346.30739-1-przemyslaw.kitszel@intel.com/

The proper fix is provider-owned registration as in i40e and mlx5, which
makes the inversion impossible by construction. That is a net-next change
across ice, irdma and iidc_rdma.h, this series is the backportable part.

Tested on an E810-XXV with two PFs, irdma bound to both auxiliary
devices, on net tip commit 191aa41548b2d2 ("MAINTAINERS: add Bobby
Eshleman as vsock reviewer") with and without this series. A reproducer
binds the irdma auxiliary driver while driving one of the three RTNL
held callers into the probe window:

                        without the series   with the series
  ethtool set_channels  deadlock 10/10       0/10, window entered 300x
  ndo_setup_tc mqprio   deadlock             0/10, window entered 50x
  dcbnl ets set         deadlock             0/10, window entered 30x

A window entry is a contended device_trylock(), so each one used to
close the cycle. 25 reboots on the patched kernel were clean.

Changes since v2:
- rebased on "ice: fix DCB lock ordering", patch 2 of v2 dropped
- patch 2 runs the fallback for non-contiguous traffic classes in place
  of the transaction instead of after it, the deferred one let the RDMA
  driver resume on traffic classes the PF VSI did not have
- Reviewed-by from Aleksandr on patches 1 and 3

Link to v2:
https://lore.kernel.org/all/20261023070258.730168-1-poros@redhat.com/
Link to v1:
https://lore.kernel.org/all/20261019065301.3983537-1-poros@redhat.com/

Petr Oros (4):
  ice: check the auxiliary device pointer under adev_mutex
  ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver
  ice: do not block on the aux device lock in ethtool set_channels
  ice: do not block on the aux device lock in ndo_setup_tc

 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 47 ++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 26 +++++++----
 drivers/net/ethernet/intel/ice/ice_idc.c     | 48 ++++++++++++++++----
 drivers/net/ethernet/intel/ice/ice_idc_int.h |  2 +
 drivers/net/ethernet/intel/ice/ice_main.c    | 40 +++++++++++-----
 5 files changed, 126 insertions(+), 37 deletions(-)


base-commit: 1fcc5adeeda91622627c5c238985b60421ee9e70
-- 
2.56.0

[PATCH iwl-net v3 1/4] ice: check the auxiliary device pointer under adev_mutex

[Petr Oros]

ice_set_channels() and the mqprio path of ice_setup_tc() test
cdev->adev before taking pf->adev_mutex and dereference it afterwards.
ice_unplug_aux_dev() clears the pointer under the mutex from the
service task, from ice_prepare_for_reset() and from the devlink RoCE
and iWARP parameter handlers, none of which hold the RTNL lock, so the
pointer can go away between the test and the device_lock() call.

Take pf->adev_mutex first and test the pointer under it. Without a
device the mutex is released again and the rebuild runs unlocked as
before.

Fixes: a6a0974aae42 ("ice: Prevent set_channel from changing queues while RDMA active")
Fixes: 4b31fd4d77ff ("ice: Fix RDMA VSI removal during queue rebuild")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Petr Oros <poros@redhat.com>
---
v3:
- rebased, Reviewed-by from Aleksandr

v2: https://lore.kernel.org/all/20261023070258.730168-2-poros@redhat.com/
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 22 +++++++++++++-------
 drivers/net/ethernet/intel/ice/ice_main.c    | 22 +++++++++++++-------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 35dff5e937402d..910664b35b4d35 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3888,6 +3888,7 @@ static int ice_vsi_set_dflt_rss_lut(struct ice_vsi *vsi, int req_rss_size)
 static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 {
 	struct ice_netdev_priv *np = netdev_priv(dev);
+	struct auxiliary_device *adev = NULL;
 	struct ice_vsi *vsi = np->vsi;
 	struct ice_pf *pf = vsi->back;
 	int new_rx = 0, new_tx = 0;
@@ -3943,14 +3944,19 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 		return -EINVAL;
 	}
 
-	if (pf->cdev_info && pf->cdev_info->adev) {
+	if (pf->cdev_info) {
 		mutex_lock(&pf->adev_mutex);
-		device_lock(&pf->cdev_info->adev->dev);
-		locked = true;
-		if (pf->cdev_info->adev->dev.driver) {
-			netdev_err(dev, "Cannot change channels when RDMA is active\n");
-			ret = -EBUSY;
-			goto adev_unlock;
+		adev = pf->cdev_info->adev;
+		if (adev) {
+			device_lock(&adev->dev);
+			locked = true;
+			if (adev->dev.driver) {
+				netdev_err(dev, "Cannot change channels when RDMA is active\n");
+				ret = -EBUSY;
+				goto adev_unlock;
+			}
+		} else {
+			mutex_unlock(&pf->adev_mutex);
 		}
 	}
 
@@ -3966,7 +3972,7 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 
 adev_unlock:
 	if (locked) {
-		device_unlock(&pf->cdev_info->adev->dev);
+		device_unlock(&adev->dev);
 		mutex_unlock(&pf->adev_mutex);
 	}
 	return ret;
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 1c6e85f3388668..1cc481ba4ca5f0 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -9538,6 +9538,7 @@ ice_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 	struct ice_netdev_priv *np = netdev_priv(netdev);
 	enum flow_block_binder_type binder_type;
 	struct iidc_rdma_core_dev_info *cdev;
+	struct auxiliary_device *adev = NULL;
 	struct ice_pf *pf = np->vsi->back;
 	flow_setup_cb_t *flower_handler;
 	bool locked = false;
@@ -9570,14 +9571,19 @@ ice_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 		}
 
 		cdev = pf->cdev_info;
-		if (cdev && cdev->adev) {
+		if (cdev) {
 			mutex_lock(&pf->adev_mutex);
-			device_lock(&cdev->adev->dev);
-			locked = true;
-			if (cdev->adev->dev.driver) {
-				netdev_err(netdev, "Cannot change qdisc when RDMA is active\n");
-				err = -EBUSY;
-				goto adev_unlock;
+			adev = cdev->adev;
+			if (adev) {
+				device_lock(&adev->dev);
+				locked = true;
+				if (adev->dev.driver) {
+					netdev_err(netdev, "Cannot change qdisc when RDMA is active\n");
+					err = -EBUSY;
+					goto adev_unlock;
+				}
+			} else {
+				mutex_unlock(&pf->adev_mutex);
 			}
 		}
 
@@ -9586,7 +9592,7 @@ ice_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 
 adev_unlock:
 		if (locked) {
-			device_unlock(&cdev->adev->dev);
+			device_unlock(&adev->dev);
 			mutex_unlock(&pf->adev_mutex);
 		}
 		return err;
-- 
2.56.0

[PATCH iwl-net v3 2/4] ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver

[Petr Oros]

ice_send_event_to_aux() blocks on the auxiliary device lock, and the
dcbnl callers reach it with the RTNL lock held. The RDMA auxiliary probe
takes the two in the opposite order, because __driver_attach() holds the
auxiliary device lock across ib_register_device(), which needs the RTNL
lock:

  systemd-udevd   state:D
    really_probe                  <- holds device_lock(&adev->dev)
    icrdma_probe
    irdma_ib_register_device
    ib_register_device
    setup_device
    irdma_roce_port_immutable
    ib_query_port
    irdma_query_port
    ib_get_eth_speed              <- blocks on rtnl_lock()

  lldpad          state:D
    rtnetlink_rcv_msg             <- holds rtnl_lock()
    dcb_doit
    dcbnl_ieee_set
    ice_dcbnl_setets
    ice_pf_dcb_cfg
    ice_send_event_to_aux         <- blocks on device_lock(&adev->dev)

Everything else that needs the RTNL lock piles up behind lldpad and the
boot never completes. The probe side cannot be changed, so take the
auxiliary device lock with device_trylock() around the event and let the
caller decide what to do when it is contended.

The DCB reconfiguration sends IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, on which
the RDMA driver suspends its queue pairs, and
IIDC_RDMA_EVENT_AFTER_TC_CHANGE, on which it resumes them. Neither can be
dropped, the first would change the traffic classes behind the RDMA
driver's back and the second would leave its queue pairs suspended. So
ice_pf_dcb_cfg() takes the locks once before the first event, keeps them
across the whole reconfiguration and sends the second event before
releasing them. The driver core holds the lock while it binds, unbinds or
suspends the auxiliary device and in a few of its sysfs handlers, and the
request is then refused with -EBUSY. The RTNL lock is taken before the
auxiliary locks also for the callers that did not hold it, so every path
takes the two in the same order, and the first event is delivered with
the RTNL lock held on those paths too, as it already was for the dcbnl
callers. The second event is now sent on the error paths too, which used
to leave the queue pairs suspended after a failed commit.
ice_pf_dcb_recfg() still sends the second event on its own for the LLDP
MIB change path, which sends no first event, so a dropped one is ignored
by the RDMA driver there.

The fallback for non-contiguous traffic classes is a nested
ice_pf_dcb_cfg() and would block on pf->adev_mutex under the locks. So
ice_pf_dcb_cfg() runs it in place of the transaction when the new
configuration is not contiguous, before it takes the locks, and
ice_pf_dcb_recfg() leaves it to the caller when the locks are held. The
other caller that passes locked, ice_vsi_recfg_qs(), already held
pf->adev_mutex there and would have deadlocked the same way through
ice_send_event_to_aux().

ice_pf_dcb_cfg() already returns -EBUSY when the custom Tx scheduler is
enabled, so the refused request takes the same path through its callers.

The critical error and MTU change events are sent from the service task,
which consumes their pending bits before sending, so those are re-armed
instead. The critical error event takes the locks before it consumes
pf->oicr_err_reg, so a contended lock leaves the register value to the
next run instead of merging it back underneath the interrupt handler.

Fixes: 348048e724a0 ("ice: Implement iidc operations")
Signed-off-by: Petr Oros <poros@redhat.com>
---
v3:
- rebased on "ice: fix DCB lock ordering"
- run the fallback for non-contiguous traffic classes in place of the
  transaction, before the locks, instead of after them

v2: https://lore.kernel.org/all/20261023070258.730168-4-poros@redhat.com/
v1: https://lore.kernel.org/all/20261019065301.3983537-2-poros@redhat.com/
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 47 ++++++++++++++++---
 drivers/net/ethernet/intel/ice/ice_idc.c     | 48 ++++++++++++++++----
 drivers/net/ethernet/intel/ice/ice_idc_int.h |  2 +
 drivers/net/ethernet/intel/ice/ice_main.c    | 14 ++++--
 4 files changed, 90 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index 5355519726df9f..dba74d940d25b9 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -5,6 +5,9 @@
 #include "ice_dcb_nl.h"
 #include "devlink/devlink.h"
 
+static bool ice_dcb_tc_contig(u8 *prio_table);
+static int ice_dcb_noncontig_cfg(struct ice_pf *pf);
+
 /**
  * ice_dcb_get_ena_tc - return bitmap of enabled TCs
  * @dcbcfg: DCB config to evaluate for enabled TCs
@@ -369,11 +372,15 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 	struct ice_aqc_port_ets_elem buf = { 0 };
 	struct ice_dcbx_cfg *old_cfg, *curr_cfg;
 	struct device *dev = ice_pf_to_dev(pf);
+	struct iidc_rdma_priv_dev_info *privd;
+	struct iidc_rdma_core_dev_info *cdev;
 	struct iidc_rdma_event *event;
 	int ret = ICE_DCB_NO_HW_CHG;
 	struct ice_vsi *pf_vsi;
+	bool dcb_ena;
 
 	curr_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
+	dcb_ena = test_bit(ICE_FLAG_DCB_ENA, pf->flags);
 
 	/* FW does not care if change happened */
 	if (!pf->hw.port_info->qos_cfg.is_sw_lldp)
@@ -422,16 +429,26 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 		goto free_cfg;
 	}
 
-	set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
-	ice_send_event_to_aux(pf, event);
-	kfree(event);
-
 	/* avoid race conditions by holding the lock while disabling and
 	 * re-enabling the VSI
 	 */
 	if (!locked)
 		rtnl_lock();
 
+	if (!ice_dcb_tc_contig(new_cfg->etscfg.prio_table)) {
+		ret = ice_dcb_noncontig_cfg(pf);
+		goto unlock_rtnl;
+	}
+
+	if (!ice_aux_dev_trylock(pf)) {
+		assign_bit(ICE_FLAG_DCB_ENA, pf->flags, dcb_ena);
+		ret = -EBUSY;
+		goto unlock_rtnl;
+	}
+
+	set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
+	ice_send_event_to_aux(pf, event);
+
 	/* disable VSIs affected by DCB changes */
 	ice_dcb_ena_dis_vsi(pf, false, true);
 
@@ -458,13 +475,25 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 		goto out;
 	}
 
-	ice_pf_dcb_recfg(pf, false);
+	ice_pf_dcb_recfg(pf, true);
 
 out:
+	cdev = pf->cdev_info;
+	if (cdev) {
+		privd = cdev->iidc_priv;
+		ice_setup_dcb_qos_info(pf, &privd->qos_info);
+	}
+	clear_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
+	set_bit(IIDC_RDMA_EVENT_AFTER_TC_CHANGE, event->type);
+	ice_send_event_to_aux(pf, event);
+	ice_aux_dev_unlock(pf);
+
 	/* enable previously downed VSIs */
 	ice_dcb_ena_dis_vsi(pf, true, true);
+unlock_rtnl:
 	if (!locked)
 		rtnl_unlock();
+	kfree(event);
 free_cfg:
 	kfree(old_cfg);
 	return ret;
@@ -778,7 +807,8 @@ void ice_pf_dcb_recfg(struct ice_pf *pf, bool locked)
 			 */
 			if (!ice_dcb_tc_contig(dcbcfg->etscfg.prio_table)) {
 				tc_map = ICE_DFLT_TRAFFIC_CLASS;
-				ice_dcb_noncontig_cfg(pf);
+				if (!locked)
+					ice_dcb_noncontig_cfg(pf);
 			}
 		} else if (vsi->type == ICE_VSI_CHNL) {
 			tc_map = BIT(ice_get_first_droptc(vsi));
@@ -813,7 +843,10 @@ void ice_pf_dcb_recfg(struct ice_pf *pf, bool locked)
 			return;
 
 		set_bit(IIDC_RDMA_EVENT_AFTER_TC_CHANGE, event->type);
-		ice_send_event_to_aux(pf, event);
+		if (ice_aux_dev_trylock(pf)) {
+			ice_send_event_to_aux(pf, event);
+			ice_aux_dev_unlock(pf);
+		}
 		kfree(event);
 	}
 }
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 09dfcffeb5df97..9e8bff128e0e63 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -30,34 +30,62 @@ ice_get_auxiliary_drv(struct iidc_rdma_core_dev_info *cdev)
 			    struct iidc_rdma_core_auxiliary_drv, adrv.driver);
 }
 
+/**
+ * ice_aux_dev_trylock - take the locks that guard the RDMA AUX driver
+ * @pf: pointer to PF struct
+ *
+ * Return: true when the locks are held, false when the auxiliary device lock
+ * was contended and nothing is held.
+ */
+bool ice_aux_dev_trylock(struct ice_pf *pf)
+{
+	struct iidc_rdma_core_dev_info *cdev = pf->cdev_info;
+
+	mutex_lock(&pf->adev_mutex);
+	if (cdev && cdev->adev && !device_trylock(&cdev->adev->dev)) {
+		mutex_unlock(&pf->adev_mutex);
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * ice_aux_dev_unlock - release the locks taken by ice_aux_dev_trylock()
+ * @pf: pointer to PF struct
+ */
+void ice_aux_dev_unlock(struct ice_pf *pf)
+{
+	struct iidc_rdma_core_dev_info *cdev = pf->cdev_info;
+
+	if (cdev && cdev->adev)
+		device_unlock(&cdev->adev->dev);
+	mutex_unlock(&pf->adev_mutex);
+}
+
 /**
  * ice_send_event_to_aux - send event to RDMA AUX driver
  * @pf: pointer to PF struct
  * @event: event struct
+ *
+ * The caller holds the locks taken by ice_aux_dev_trylock().
  */
 void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_rdma_event *event)
 {
+	struct iidc_rdma_core_dev_info *cdev = pf->cdev_info;
 	struct iidc_rdma_core_auxiliary_drv *iadrv;
-	struct iidc_rdma_core_dev_info *cdev;
 
 	if (WARN_ON_ONCE(!in_task()))
 		return;
 
-	cdev = pf->cdev_info;
+	lockdep_assert_held(&pf->adev_mutex);
+
 	if (!cdev)
 		return;
 
-	mutex_lock(&pf->adev_mutex);
-	if (!cdev->adev)
-		goto finish;
-
-	device_lock(&cdev->adev->dev);
 	iadrv = ice_get_auxiliary_drv(cdev);
 	if (iadrv && iadrv->event_handler)
 		iadrv->event_handler(cdev, event);
-	device_unlock(&cdev->adev->dev);
-finish:
-	mutex_unlock(&pf->adev_mutex);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_idc_int.h b/drivers/net/ethernet/intel/ice/ice_idc_int.h
index 17dbfcfb6a2aff..4cccb4764271ae 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc_int.h
+++ b/drivers/net/ethernet/intel/ice/ice_idc_int.h
@@ -9,6 +9,8 @@
 
 struct ice_pf;
 
+bool ice_aux_dev_trylock(struct ice_pf *pf);
+void ice_aux_dev_unlock(struct ice_pf *pf);
 void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_rdma_event *event);
 
 #endif /* !_ICE_IDC_INT_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index 1cc481ba4ca5f0..bbe4409b16cf57 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2385,13 +2385,16 @@ static void ice_service_task(struct work_struct *work)
 		struct iidc_rdma_event *event;
 
 		event = kzalloc_obj(*event);
-		if (event) {
+		if (event && ice_aux_dev_trylock(pf)) {
 			set_bit(IIDC_RDMA_EVENT_CRIT_ERR, event->type);
 			/* report the entire OICR value to AUX driver */
 			swap(event->reg, pf->oicr_err_reg);
 			ice_send_event_to_aux(pf, event);
-			kfree(event);
+			ice_aux_dev_unlock(pf);
+		} else {
+			set_bit(ICE_AUX_ERR_PENDING, pf->state);
 		}
+		kfree(event);
 	}
 
 	/* unplug aux dev per request, if an unplug request came in
@@ -2408,11 +2411,14 @@ static void ice_service_task(struct work_struct *work)
 		struct iidc_rdma_event *event;
 
 		event = kzalloc_obj(*event);
-		if (event) {
+		if (event && ice_aux_dev_trylock(pf)) {
 			set_bit(IIDC_RDMA_EVENT_AFTER_MTU_CHANGE, event->type);
 			ice_send_event_to_aux(pf, event);
-			kfree(event);
+			ice_aux_dev_unlock(pf);
+		} else {
+			set_bit(ICE_FLAG_MTU_CHANGED, pf->flags);
 		}
+		kfree(event);
 	}
 
 	ice_clean_adminq_subtask(pf);
-- 
2.56.0

[PATCH iwl-net v3 3/4] ice: do not block on the aux device lock in ethtool set_channels

[Petr Oros]

ice_set_channels() takes the auxiliary device lock to test whether the
RDMA driver is bound, and ethtool ops run with the RTNL lock held. This
is the same inversion against the RDMA auxiliary probe as the one fixed
in ice_send_event_to_aux(), reached through a different caller.

Use device_trylock() and report the device as busy when it fails. The
existing diagnostic is not reused, a contended lock does not mean the
RDMA driver is bound. It is a netdev_dbg(), the condition is transient
and userspace already learns about it from -EBUSY.

Fixes: a6a0974aae42 ("ice: Prevent set_channel from changing queues while RDMA active")
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Petr Oros <poros@redhat.com>
---
v3:
- rebased, Reviewed-by from Aleksandr

v2: https://lore.kernel.org/all/20261023070258.730168-5-poros@redhat.com/
v1: https://lore.kernel.org/all/20261019065301.3983537-3-poros@redhat.com/
---
 drivers/net/ethernet/intel/ice/ice_ethtool.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 910664b35b4d35..61684812733bba 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3948,7 +3948,11 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 		mutex_lock(&pf->adev_mutex);
 		adev = pf->cdev_info->adev;
 		if (adev) {
-			device_lock(&adev->dev);
+			if (!device_trylock(&adev->dev)) {
+				mutex_unlock(&pf->adev_mutex);
+				netdev_dbg(dev, "RDMA auxiliary device is busy, try again\n");
+				return -EBUSY;
+			}
 			locked = true;
 			if (adev->dev.driver) {
 				netdev_err(dev, "Cannot change channels when RDMA is active\n");
-- 
2.56.0

[PATCH iwl-net v3 4/4] ice: do not block on the aux device lock in ndo_setup_tc

[Petr Oros]

The TC_SETUP_QDISC_MQPRIO path of ice_setup_tc() takes the auxiliary
device lock to test whether the RDMA driver is bound, and ndo_setup_tc()
runs with the RTNL lock held. This closes the same cycle against the
RDMA auxiliary probe as the two preceding patches.

Use device_trylock() and report the device as busy when it fails. When
the qdisc is added, userspace gets -EBUSY and can retry. When it is
deleted, mqprio_disable_offload() ignores the return value, so the
hardware traffic classes and the channel VSIs stay in place until a
later qdisc teardown gets through, the same as the existing -EBUSY for
a bound RDMA driver. The message is a netdev_warn() because of that
second case, nothing else reports it.

Fixes: 4b31fd4d77ff ("ice: Fix RDMA VSI removal during queue rebuild")
Signed-off-by: Petr Oros <poros@redhat.com>
---
v3:
- rebased on "ice: fix DCB lock ordering", no pf->tc_mutex to release
  on the new error path

v2: https://lore.kernel.org/all/20261023070258.730168-6-poros@redhat.com/
v1: https://lore.kernel.org/all/20261019065301.3983537-4-poros@redhat.com/
---
 drivers/net/ethernet/intel/ice/ice_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index bbe4409b16cf57..fbcce9bd798dc9 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -9581,7 +9581,11 @@ 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");
+					return -EBUSY;
+				}
 				locked = true;
 				if (adev->dev.driver) {
 					netdev_err(netdev, "Cannot change qdisc when RDMA is active\n");
-- 
2.56.0

Re: [PATCH iwl-net v3 4/4] 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 to consider:
- [High] ice: orphaned channel VSIs and memory leak when ice_setup_tc()
  fails teardown
--

commit e098adc9e591059f39322dbf2abaa105d7a546c3
Author: Petr Oros <poros@redhat.com>

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 ordering cycle with RTNL.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index bbe4409b16cf5..fbcce9bd798dc 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -9581,7 +9581,11 @@ 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");
> +					return -EBUSY;
> +				}

[Severity: High]
As reported by Loktionov, Aleksandr on v1 [1], does returning -EBUSY here
during qdisc destruction (qopt.hw == 0) orphan the channel VSIs and leak
vsi->ch_list?

When a user deletes or replaces an offloaded mqprio qdisc while the RDMA
auxiliary device lock is held (for example, during auxiliary driver probe
or unbind), mqprio_destroy() calls mqprio_disable_offload(), ignores the
-EBUSY return, and frees the Qdisc.  Because the Qdisc is gone, no later
qdisc teardown will run for it.

ICE_FLAG_TC_MQPRIO stays set and vsi->ch_list remains populated.  Driver
unload skips ICE_VSI_CHNL VSIs in ice_vsi_release_all() without calling
ice_remove_q_channels(), leaking the struct ice_channel entries, TC flower
channel filters, and hardware channel VSIs.  If a new offloaded mqprio
qdisc is added before then, ice_setup_tc_mqprio_qdisc() only removes
existing channels when !hw, so it appends duplicate channels to
vsi->ch_list and can index past vsi->tx_rings[] and vsi->rx_rings[] in
ice_remove_q_channels() if the new qdisc configures fewer queues.

[1] https://lore.kernel.org/r/CH3PR11MB566895E0410B0694F734EA7DAFE5662@CH3PR11MB566895.namprd11.prod.outlook.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113161453.1234851-1-poros@redhat.com?part=4

Re: [PATCH iwl-net v3 2/4] ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] ice: inconsistent QoS state on error path in ice_pf_dcb_cfg()
- [Medium] ice: unrolled DCB state and zeroed local_dcbx_cfg when
  ice_pf_dcb_cfg() fails
- [Medium] ice: read of pf->cdev_info before adev_mutex in
  ice_aux_dev_trylock()
--

commit 5ebebfa9f362bf112076cd4d87281d6a453d0c5c
Author: Petr Oros <poros@redhat.com>

ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver

This commit replaces blocking device_lock() acquisitions when sending events
to the RDMA auxiliary driver with ice_aux_dev_trylock() to prevent an ABBA
deadlock between rtnl_lock and the auxiliary device lock.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index 5355519726df9..dba74d940d25b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
[ ... ]
> @@ -422,16 +429,26 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
>  		goto free_cfg;
>  	}
>  
> -	set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
> -	ice_send_event_to_aux(pf, event);
> -	kfree(event);
> -
>  	/* avoid race conditions by holding the lock while disabling and
>  	 * re-enabling the VSI
>  	 */
>  	if (!locked)
>  		rtnl_lock();
>  
> +	if (!ice_dcb_tc_contig(new_cfg->etscfg.prio_table)) {
> +		ret = ice_dcb_noncontig_cfg(pf);
> +		goto unlock_rtnl;
> +	}
> +
> +	if (!ice_aux_dev_trylock(pf)) {
> +		assign_bit(ICE_FLAG_DCB_ENA, pf->flags, dcb_ena);
> +		ret = -EBUSY;
> +		goto unlock_rtnl;
> +	}

[Severity: Medium]
Does this leave local_dcbx_cfg zeroed and ICE_FLAG_DCB_ENA cleared when
ice_aux_dev_trylock() fails with -EBUSY?

When new_cfg has non-contiguous traffic classes, ice_pf_dcb_cfg() clears
ICE_FLAG_DCB_ENA at the start of the function and calls
ice_dcb_noncontig_cfg() -> ice_dcb_sw_dflt_cfg(), which zeroes
pi->qos_cfg.local_dcbx_cfg via memset() before calling the nested
ice_pf_dcb_cfg():

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_dcb_sw_dflt_cfg() {
	...
	memset(&pi->qos_cfg.local_dcbx_cfg, 0, sizeof(*dcbcfg));
	...
	ret = ice_pf_dcb_cfg(pf, dcbcfg, locked);
}

If the nested ice_pf_dcb_cfg() then hits contention in ice_aux_dev_trylock()
and returns -EBUSY, its dcb_ena is already false and local_dcbx_cfg is
already zeroed, and the outer ice_pf_dcb_cfg() jumps straight to unlock_rtnl
without restoring curr_cfg from old_cfg or ICE_FLAG_DCB_ENA from its saved
dcb_ena. Direct calls to ice_dcb_sw_dflt_cfg() and ice_dcb_init_cfg()
similarly zero local_dcbx_cfg before calling ice_pf_dcb_cfg() without
restoring it on -EBUSY.

In addition, for multi-TC configs, ice_pf_dcb_cfg() calls
ice_dcb_prep_multi_tc() (tearing down the devlink rate tree) before
ice_aux_dev_trylock(), and ice_dcbnl_setapp() updates the dcbnl app table,
new_cfg->dscp_mapped, new_cfg->app[], and hardware PFC mode before calling
ice_pf_dcb_cfg() without rolling them back on -EBUSY.

> @@ -458,13 +475,25 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
>  		goto out;
>  	}
>  
> -	ice_pf_dcb_recfg(pf, false);
> +	ice_pf_dcb_recfg(pf, true);
>  
>  out:
> +	cdev = pf->cdev_info;
> +	if (cdev) {
> +		privd = cdev->iidc_priv;
> +		ice_setup_dcb_qos_info(pf, &privd->qos_info);
> +	}
> +	clear_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
> +	set_bit(IIDC_RDMA_EVENT_AFTER_TC_CHANGE, event->type);
> +	ice_send_event_to_aux(pf, event);
> +	ice_aux_dev_unlock(pf);

[Severity: High]
If ice_query_port_ets() fails and jumps to out, does populating
privd->qos_info and sending IIDC_RDMA_EVENT_AFTER_TC_CHANGE transition
active RDMA queue pairs to the error state?

When ice_query_port_ets() fails, curr_cfg and PRTDCB_TUP2TC already hold
new_cfg, but ice_pf_dcb_recfg() is skipped so vsi->tc_cfg.ena_tc and the VSI
scheduler nodes still only have the old traffic classes. Running
ice_setup_dcb_qos_info() at out populates privd->qos_info with the unapplied
traffic classes, so when irdma handles IIDC_RDMA_EVENT_AFTER_TC_CHANGE and
calls ice_add_rdma_qset() to resume its QPs on those TCs, ice_cfg_vsi_rdma()
and ice_ena_vsi_rdma_qset() fail with -EINVAL and irdma moves the QPs to
IB_QPS_ERR. The ice_query_port_ets() error path also leaves curr_cfg and
ICE_FLAG_DCB_ENA unrolled.

> diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
> index 09dfcffeb5df9..9e8bff128e0e6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_idc.c
> +++ b/drivers/net/ethernet/intel/ice/ice_idc.c
> @@ -30,34 +30,62 @@ ice_get_auxiliary_drv(struct iidc_rdma_core_dev_info *cdev)
>  			    struct iidc_rdma_core_auxiliary_drv, adrv.driver);
>  }
>  
> +/**
> + * ice_aux_dev_trylock - take the locks that guard the RDMA AUX driver
> + * @pf: pointer to PF struct
> + *
> + * Return: true when the locks are held, false when the auxiliary device lock
> + * was contended and nothing is held.
> + */
> +bool ice_aux_dev_trylock(struct ice_pf *pf)
> +{
> +	struct iidc_rdma_core_dev_info *cdev = pf->cdev_info;
> +
> +	mutex_lock(&pf->adev_mutex);
> +	if (cdev && cdev->adev && !device_trylock(&cdev->adev->dev)) {
> +		mutex_unlock(&pf->adev_mutex);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/**
> + * ice_aux_dev_unlock - release the locks taken by ice_aux_dev_trylock()
> + * @pf: pointer to PF struct
> + */
> +void ice_aux_dev_unlock(struct ice_pf *pf)
> +{
> +	struct iidc_rdma_core_dev_info *cdev = pf->cdev_info;
> +
> +	if (cdev && cdev->adev)
> +		device_unlock(&cdev->adev->dev);
> +	mutex_unlock(&pf->adev_mutex);
> +}

[Severity: Medium]
Can reading pf->cdev_info before acquiring pf->adev_mutex in
ice_aux_dev_trylock() cause an unbalanced device_unlock(&cdev->adev->dev) or
a use-after-free?

In ice_load(), the netdev and its dcbnl_ops are registered before
ice_init_rdma() allocates pf->cdev_info and ice_rdma_finalize_setup() ->
ice_plug_aux_dev() sets cdev->adev under pf->adev_mutex:

CPU0                                  CPU1
ice_dcbnl_setets()                    ice_load()
  ice_pf_dcb_cfg()                      ...
    ice_aux_dev_trylock()               ice_init_rdma()
      cdev = pf->cdev_info; /* NULL */    pf->cdev_info = kzalloc(...);
                                        ice_rdma_finalize_setup()
                                          ice_plug_aux_dev()
                                            mutex_lock(&pf->adev_mutex)
                                            cdev->adev = adev;
                                            mutex_unlock(&pf->adev_mutex)
      mutex_lock(&pf->adev_mutex)
      /* cdev is NULL, skips trylock */
    ice_send_event_to_aux()
    ice_aux_dev_unlock()
      cdev = pf->cdev_info;
      device_unlock(&cdev->adev->dev)  <-- unlocks unheld mutex

Conversely, if ice_aux_dev_trylock() samples a non-NULL pf->cdev_info while
racing with ice_unload() or ice_suspend() calling ice_deinit_rdma(),
dereferencing cdev->adev after mutex_lock(&pf->adev_mutex) reads freed
memory. Reading pf->cdev_info after mutex_lock(&pf->adev_mutex) (and
protecting pf->cdev_info teardown under pf->adev_mutex) would avoid both.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113161453.1234851-1-poros@redhat.com?part=2