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

[PATCH iwl-net 0/3] 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 same two locks in the opposite order, because __driver_attach()
holds the auxiliary device lock across ib_register_device(), which needs
the RTNL lock. ib_query_port() reaches ib_get_eth_speed() first from the
port immutable setup and again from ib_cache_setup_one(). The result is
a hard deadlock that leaves the whole RTNL-serialised part of the stack
stuck, 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)

linkwatch, addrconf, NetworkManager and every ethtool ioctl then pile up
behind lldpad. On a PREEMPT_RT kernel with CONFIG_DEBUG_RT_MUTEXES the
rtmutex chain walker reports "rtmutex deadlock detected" from whichever
task closes the cycle instead of hanging silently. Plain lockdep cannot
flag it, because device_initialize() marks dev->mutex with
lockdep_set_novalidate_class().

The probe side cannot be changed, so the three ice callers stop blocking
and use device_trylock() instead. Contention is only possible while the
driver core is binding, unbinding or suspending the auxiliary device,
since all three sites already serialise against each other under
pf->adev_mutex, and ice_unplug_aux_dev() clears cdev->adev under that
mutex before calling auxiliary_device_delete(). ice_unplug_aux_dev()
itself is never reached with the RTNL lock held, so there is no fourth
site to convert.

Patch 1 then handles a contended lock per event, because the right
answer differs. BEFORE_TC_CHANGE has to arrive before the traffic
classes change, so the DCB reconfiguration is refused with -EBUSY. The
MTU change and critical error events are sent from the service task,
which consumes their pending bits before sending, so they are re-armed
for the next run. AFTER_TC_CHANGE keeps its existing fire and forget
behaviour, and the commit message explains why deferring that one is
not safe. Patches 2 and 3 only turn the lock into a trylock, where a
contended lock means the operation is refused with -EBUSY and can be
retried, which is what the existing check already does when the RDMA
driver is bound.

This keeps the fix small enough to backport. It does not address the
underlying layering problem, which is that ice reaches for the driver
core lock of a foreign device at all. i40e and mlx5 both solve the same
need with provider-owned registration instead, and converting IIDC to
that model would make the inversion impossible by construction. That is
a net-next change across ice, irdma and iidc_rdma.h, and I am happy to
follow up with it if there is interest.

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, same config
otherwise. 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(), that is, the exact
moment really_probe() holds the auxiliary device lock and waits for the
RTNL lock, so each one is an occasion that used to close the cycle.
Without the series the console shows both halves and the box only
returns through hung_task_panic. 25 reboots on the patched kernel were
clean.

Petr Oros (3):
  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 |  9 +++++++-
 drivers/net/ethernet/intel/ice/ice_ethtool.c |  6 +++++-
 drivers/net/ethernet/intel/ice/ice_idc.c     | 20 ++++++++++++++----
 drivers/net/ethernet/intel/ice/ice_idc_int.h |  2 +-
 drivers/net/ethernet/intel/ice/ice_main.c    | 22 +++++++++++++++-----
 5 files changed, 47 insertions(+), 12 deletions(-)

-- 
2.56.0

[PATCH iwl-net 1/3] 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 use
device_trylock() and report whether the event reached the driver.

IIDC_RDMA_EVENT_BEFORE_TC_CHANGE has to arrive before the traffic
classes change, so ice_pf_dcb_cfg() refuses the request with -EBUSY
rather than run it behind the RDMA driver's back. 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.
AFTER_TC_CHANGE keeps its fire and forget behaviour, it can be neither
refused, the hardware has already changed, nor deferred, every lock that
would serialise a deferred delivery against the dcbnl callers forms
another cycle.

Fixes: 348048e724a0 ("ice: Implement iidc operations")
Cc: stable@vger.kernel.org
Signed-off-by: Petr Oros <poros@redhat.com>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c |  9 ++++++++-
 drivers/net/ethernet/intel/ice/ice_idc.c     | 20 ++++++++++++++++----
 drivers/net/ethernet/intel/ice/ice_idc_int.h |  2 +-
 drivers/net/ethernet/intel/ice/ice_main.c    | 16 ++++++++++++----
 4 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index 0bc6dd37568792..2022b46c990cb2 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -355,8 +355,10 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 	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)
@@ -406,7 +408,12 @@ int ice_pf_dcb_cfg(struct ice_pf *pf, struct ice_dcbx_cfg *new_cfg, bool locked)
 	}
 
 	set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
-	ice_send_event_to_aux(pf, event);
+	if (!ice_send_event_to_aux(pf, event)) {
+		assign_bit(ICE_FLAG_DCB_ENA, pf->flags, dcb_ena);
+		kfree(event);
+		ret = -EBUSY;
+		goto free_cfg;
+	}
 	kfree(event);
 
 	/* avoid race conditions by holding the lock while disabling and
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index 102d63c3018b1f..7fd35ce966ea65 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -34,30 +34,42 @@ ice_get_auxiliary_drv(struct iidc_rdma_core_dev_info *cdev)
  * ice_send_event_to_aux - send event to RDMA AUX driver
  * @pf: pointer to PF struct
  * @event: event struct
+ *
+ * Return: false when the auxiliary device lock was contended.
  */
-void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_rdma_event *event)
+bool ice_send_event_to_aux(struct ice_pf *pf, struct iidc_rdma_event *event)
 {
 	struct iidc_rdma_core_auxiliary_drv *iadrv;
 	struct iidc_rdma_core_dev_info *cdev;
+	bool sent = true;
 
 	if (WARN_ON_ONCE(!in_task()))
-		return;
+		return true;
 
 	cdev = pf->cdev_info;
 	if (!cdev)
-		return;
+		return true;
 
 	mutex_lock(&pf->adev_mutex);
 	if (!cdev->adev)
 		goto finish;
 
-	device_lock(&cdev->adev->dev);
+	if (!device_trylock(&cdev->adev->dev)) {
+		dev_dbg(ice_pf_to_dev(pf),
+			"auxiliary device busy, dropped RDMA event 0x%lx\n",
+			*event->type);
+		sent = false;
+		goto finish;
+	}
+
 	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);
+
+	return sent;
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_idc_int.h b/drivers/net/ethernet/intel/ice/ice_idc_int.h
index 17dbfcfb6a2aff..e7d249d6140d92 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc_int.h
+++ b/drivers/net/ethernet/intel/ice/ice_idc_int.h
@@ -9,6 +9,6 @@
 
 struct ice_pf;
 
-void ice_send_event_to_aux(struct ice_pf *pf, struct iidc_rdma_event *event);
+bool 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 d88835482d3aac..edb62606d2aae3 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -2313,11 +2313,16 @@ static void ice_service_task(struct work_struct *work)
 		struct iidc_rdma_event *event;
 
 		event = kzalloc_obj(*event);
-		if (event) {
+		if (!event) {
+			set_bit(ICE_AUX_ERR_PENDING, pf->state);
+		} else {
 			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);
+			if (!ice_send_event_to_aux(pf, event)) {
+				pf->oicr_err_reg |= event->reg;
+				set_bit(ICE_AUX_ERR_PENDING, pf->state);
+			}
 			kfree(event);
 		}
 	}
@@ -2336,9 +2341,12 @@ static void ice_service_task(struct work_struct *work)
 		struct iidc_rdma_event *event;
 
 		event = kzalloc_obj(*event);
-		if (event) {
+		if (!event) {
+			set_bit(ICE_FLAG_MTU_CHANGED, pf->flags);
+		} else {
 			set_bit(IIDC_RDMA_EVENT_AFTER_MTU_CHANGE, event->type);
-			ice_send_event_to_aux(pf, event);
+			if (!ice_send_event_to_aux(pf, event))
+				set_bit(ICE_FLAG_MTU_CHANGED, pf->flags);
 			kfree(event);
 		}
 	}
-- 
2.56.0

[PATCH iwl-net 2/3] 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")
Cc: stable@vger.kernel.org
Signed-off-by: Petr Oros <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 bf9a821c543b7f..f754ffe03285e2 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3944,7 +3944,11 @@ static int ice_set_channels(struct net_device *dev, struct ethtool_channels *ch)
 
 	if (pf->cdev_info && pf->cdev_info->adev) {
 		mutex_lock(&pf->adev_mutex);
-		device_lock(&pf->cdev_info->adev->dev);
+		if (!device_trylock(&pf->cdev_info->adev->dev)) {
+			mutex_unlock(&pf->adev_mutex);
+			netdev_dbg(dev, "RDMA auxiliary device is busy, try again\n");
+			return -EBUSY;
+		}
 		locked = true;
 		if (pf->cdev_info->adev->dev.driver) {
 			netdev_err(dev, "Cannot change channels when RDMA is active\n");
-- 
2.56.0

[PATCH iwl-net 3/3] 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, with
a distinct message for the same reason as in the previous patch.

Fixes: 4b31fd4d77ff ("ice: Fix RDMA VSI removal during queue rebuild")
Cc: stable@vger.kernel.org
Signed-off-by: Petr Oros <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 edb62606d2aae3..7b251136149615 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -9448,7 +9448,11 @@ ice_setup_tc(struct net_device *netdev, enum tc_setup_type type,
 		cdev = pf->cdev_info;
 		if (cdev && cdev->adev) {
 			mutex_lock(&pf->adev_mutex);
-			device_lock(&cdev->adev->dev);
+			if (!device_trylock(&cdev->adev->dev)) {
+				mutex_unlock(&pf->adev_mutex);
+				netdev_dbg(netdev, "RDMA auxiliary device is busy, try again\n");
+				return -EBUSY;
+			}
 			locked = true;
 			if (cdev->adev->dev.driver) {
 				netdev_err(netdev, "Cannot change qdisc when RDMA is active\n");
-- 
2.56.0