[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

Re: [PATCH iwl-net 1/3] ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ice: dropping AFTER_TC_CHANGE in ice_send_event_to_aux() wedges RDMA QPs
- [High] ice: incomplete rollback on -EBUSY in ice_pf_dcb_cfg() and callers
- [Medium] ice: unsynchronized oicr_err_reg RMW in ice_service_task() races with IRQ
--

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

ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver

This commit switches ice_send_event_to_aux() from device_lock() to
device_trylock() to avoid an ABBA deadlock between the auxiliary device
lock and rtnl_lock during RDMA auxiliary device probing. When the lock is
contended, ice_pf_dcb_cfg() aborts with -EBUSY before changing traffic
classes, and ice_service_task() re-arms pending error and MTU change bits.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index 0bc6dd3756879..2022b46c990cb 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;
> +	}

[Severity: High]
When ice_send_event_to_aux() returns false here, does aborting with -EBUSY
leave earlier state changes in ice_pf_dcb_cfg() and its callers
unreverted?

Earlier in ice_pf_dcb_cfg(), when ice_dcb_get_num_tc(new_cfg) > 1, the
devlink rate tree is torn down before reaching ice_send_event_to_aux():

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_pf_dcb_cfg()
	if (ice_dcb_get_num_tc(new_cfg) > 1) {
		...
		ice_tear_down_devlink_rate_tree(pf);

		set_bit(ICE_FLAG_DCB_ENA, pf->flags);
	}

Taking the goto free_cfg path restores ICE_FLAG_DCB_ENA, but leaves the VF
devlink rate tree destroyed.

In addition, several callers mutate DCB state before calling
ice_pf_dcb_cfg() and do not roll it back or propagate -EBUSY:

1. In drivers/net/ethernet/intel/ice/ice_dcb_nl.c, ice_dcbnl_setapp()
registers the app via dcb_ieee_setapp(), sets new_cfg->dscp_mapped,
calls ice_aq_set_pfc_mode() to switch hardware to DSCP-based PFC, and
appends to new_cfg->app[], then overwrites the -EBUSY return with
ICE_DCB_NO_HW_CHG:

	ret = ice_pf_dcb_cfg(pf, new_cfg, true);
	/* return of zero indicates new cfg applied */
	if (ret == ICE_DCB_HW_CHG_RST)
		ice_dcbnl_devreset(netdev);
	else
		ret = ICE_DCB_NO_HW_CHG;

A subsequent retry of ice_dcbnl_setapp() for the same DSCP value will then
fail with -EINVAL because test_and_set_bit(app->protocol,
new_cfg->dscp_mapped) sees the bit already set. ice_dcbnl_cee_set_all()
similarly masks -EBUSY with ICE_DCB_NO_HW_CHG.

2. In drivers/net/ethernet/intel/ice/ice_dcb_lib.c, both
ice_dcb_sw_dflt_cfg() and ice_dcb_init_cfg() zero
pi->qos_cfg.local_dcbx_cfg with memset() before calling ice_pf_dcb_cfg().
If ice_pf_dcb_cfg() aborts at goto free_cfg before memcpy(curr_cfg,
new_cfg, ...), local_dcbx_cfg remains zeroed out while the hardware
retains the previous configuration.

3. In drivers/net/ethernet/intel/ice/ice_dcb_nl.c, ice_dcbnl_setets(),
ice_dcbnl_setpfc(), and ice_dcbnl_delapp() modify
pf->hw.port_info->qos_cfg.desired_dcbx_cfg in-place before calling
ice_pf_dcb_cfg() without restoring it on failure.

>  	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 102d63c3018b1..7fd35ce966ea6 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;
> +	}

[Severity: High]
Can dropping IIDC_RDMA_EVENT_AFTER_TC_CHANGE here leave RDMA QPs
permanently suspended or initialized with stale QoS parameters?

In ice_pf_dcb_cfg(), IIDC_RDMA_EVENT_BEFORE_TC_CHANGE is delivered first,
and ice_send_event_to_aux() immediately releases device_unlock(&cdev->adev->dev).
In the RDMA auxiliary driver, icrdma_iidc_event_handler() handles
BEFORE_TC_CHANGE by calling icrdma_prep_tc_change(), which sets
iwdev->vsi.tc_change_pending = true, suspends all QPs via
irdma_sc_suspend_resume_qps(&iwdev->vsi, IRDMA_OP_SUSPEND), and resets the
work scheduler tree via irdma_ws_reset(&iwdev->vsi).

After reconfiguring the VSI and hardware, ice_pf_dcb_cfg() calls
ice_pf_dcb_recfg(), which sends IIDC_RDMA_EVENT_AFTER_TC_CHANGE and
ignores the return value:

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_pf_dcb_recfg()
		set_bit(IIDC_RDMA_EVENT_AFTER_TC_CHANGE, event->type);
		ice_send_event_to_aux(pf, event);
		kfree(event);

If cdev->adev->dev is contended when AFTER_TC_CHANGE is sent (for example,
by a concurrent sysfs or driver-core operation taking device_lock() on
cdev->adev->dev), ice_send_event_to_aux() drops the event. Because only
the AFTER_TC_CHANGE path in icrdma_iidc_event_handler() ->
irdma_change_l2params() clears vsi->tc_change_pending = false and calls
irdma_sc_suspend_resume_qps(vsi, IRDMA_OP_RESUME):

drivers/infiniband/hw/irdma/icrdma_if.c:icrdma_iidc_event_handler()
	} else if (*event->type & BIT(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE)) {
		if (iwdev->vsi.tc_change_pending)
			return;

		icrdma_prep_tc_change(iwdev);
	} else if (*event->type & BIT(IIDC_RDMA_EVENT_AFTER_TC_CHANGE)) {
		...
		if (!iwdev->vsi.tc_change_pending)
			return;
		...
		irdma_change_l2params(&iwdev->vsi, &l2params);

dropping AFTER_TC_CHANGE leaves iwdev->vsi.tc_change_pending stuck true,
all existing QPs suspended, new QP work-scheduler additions rejected with
-EBUSY in irdma_ws_add(), and later BEFORE_TC_CHANGE events returning
early because tc_change_pending is already set.

Similarly, if icrdma_probe() starts right after BEFORE_TC_CHANGE ran (when
adev->dev.driver was still NULL) or concurrently with
ice_dcb_process_lldp_set_mib_change() -> ice_pf_dcb_recfg(), icrdma_probe()
can read idc_priv->qos_info before ice_setup_dcb_qos_info() updates it,
block on rtnl_lock() in irdma_ib_register_device() while holding
device_lock(&adev->dev), cause ice_pf_dcb_recfg() to drop AFTER_TC_CHANGE,
and finish probing with stale TC/QoS parameters.

[ ... ]

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index c26cef0214552..595a2099c9564 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -2385,11 +2385,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);
> +			}

[Severity: Medium]
Can the read-modify-write on pf->oicr_err_reg here race with
ice_misc_intr() and lose newly reported OICR error bits?

pf->oicr_err_reg is a plain u32 updated by the hardirq handler
ice_misc_intr():

drivers/net/ethernet/intel/ice/ice_main.c:ice_misc_intr()
	if (oicr & ICE_AUX_CRIT_ERR) {
		pf->oicr_err_reg |= oicr;
		set_bit(ICE_AUX_ERR_PENDING, pf->state);
		ena_mask &= ~ICE_AUX_CRIT_ERR;
	}

In ice_service_task(), pf->oicr_err_reg |= event->reg runs in process
context with interrupts enabled. If ice_misc_intr() fires between the load
and store of pf->oicr_err_reg, the store from ice_service_task() will
overwrite the bits just recorded by the interrupt handler.

>  			kfree(event);
>  		}
>  	}

[ ... ]

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

[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: TOCTOU NULL dereference on cdev_info->adev in ice_set_channels()
--

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

ice: do not block on the aux device lock in ethtool set_channels

This commit replaces device_lock() with device_trylock() in
ice_set_channels() to avoid a lock inversion between the RTNL lock and the
auxiliary device lock during RDMA auxiliary device probe, returning -EBUSY
when the lock is contended.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> index aa87d4f6324e8..976402b6836de 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
> +++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
> @@ -3947,7 +3947,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");

[Severity: High]
This problem wasn't introduced by this patch, but can pf->cdev_info->adev
become NULL between the unlocked check in ice_set_channels() and the
device_trylock(&pf->cdev_info->adev->dev) call?

Paths that do not hold rtnl_lock, such as ice_service_task() handling
ICE_FLAG_UNPLUG_AUX_DEV, ice_prepare_for_reset(), or
ice_devlink_enable_roce_set() and ice_devlink_enable_iw_set(), can run
concurrently and call ice_unplug_aux_dev():

drivers/net/ethernet/intel/ice/ice_idc.c:ice_unplug_aux_dev() {
	...
	mutex_lock(&pf->adev_mutex);
	adev = pf->cdev_info->adev;
	pf->cdev_info->adev = NULL;
	mutex_unlock(&pf->adev_mutex);

	auxiliary_device_delete(adev);
	auxiliary_device_uninit(adev);
}

If ice_unplug_aux_dev() clears pf->cdev_info->adev after ice_set_channels()
checks it but before ice_set_channels() acquires pf->adev_mutex,
device_trylock(&pf->cdev_info->adev->dev) will dereference a NULL pointer
(and conversely, if ice_plug_aux_dev() assigns cdev->adev concurrently, the
RDMA active check could be skipped).

In contrast, ice_send_event_to_aux() checks cdev->adev after acquiring
pf->adev_mutex:

drivers/net/ethernet/intel/ice/ice_idc.c:ice_send_event_to_aux() {
	...
	mutex_lock(&pf->adev_mutex);
	if (!cdev->adev)
		goto finish;

	if (!device_trylock(&cdev->adev->dev)) {
	...
}

Should ice_set_channels() likewise acquire pf->adev_mutex before testing
pf->cdev_info->adev?

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

[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ice: TOCTOU race on cdev->adev in ice_setup_tc() causing NULL dereference
- [High] ice: dropped AFTER_TC_CHANGE event and rate tree teardown in ice_pf_dcb_cfg()
- [High] ice: lock inversion between adev_mutex and tc_mutex in ice_setup_tc()
- [Medium] ice: ice_setup_tc() aborts non-failable mqprio offload teardown on trylock
--

commit bc1b4cca10360a37e77b845d47a02beb259d8055
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(),
returning -EBUSY when the lock is contended to avoid deadlocking with
the RDMA auxiliary driver probe under the RTNL lock.

> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index 595a2099c9564..4ed62f1498f1f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -9556,7 +9556,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;
> +			}

[Severity: High]
Can cdev->adev become NULL before mutex_lock(&pf->adev_mutex) is
acquired, causing device_trylock(&cdev->adev->dev) to dereference a NULL
pointer?

In drivers/net/ethernet/intel/ice/ice_idc.c, ice_unplug_aux_dev() clears
pf->cdev_info->adev under pf->adev_mutex:

drivers/net/ethernet/intel/ice/ice_idc.c:ice_unplug_aux_dev() {
	...
	mutex_lock(&pf->adev_mutex);
	adev = pf->cdev_info->adev;
	pf->cdev_info->adev = NULL;
	mutex_unlock(&pf->adev_mutex);

	auxiliary_device_delete(adev);
	auxiliary_device_uninit(adev);
}

ice_unplug_aux_dev() and ice_plug_aux_dev() can run without rtnl_lock(),
for example from ice_service_task() in
drivers/net/ethernet/intel/ice/ice_main.c when ICE_FLAG_UNPLUG_AUX_DEV or
ICE_FLAG_PLUG_AUX_DEV is processed, from ice_prepare_for_reset(), or from
the devlink RoCE/iWARP parameter callbacks.

If ice_unplug_aux_dev() clears cdev->adev between the unlocked
if (cdev && cdev->adev) check and mutex_lock(&pf->adev_mutex) in
ice_setup_tc() (or in ice_set_channels() in
drivers/net/ethernet/intel/ice/ice_ethtool.c from the preceding patch in
this series), cdev->adev will be NULL when device_trylock(&cdev->adev->dev)
is called. Conversely, if ice_plug_aux_dev() assigns cdev->adev just after
the unlocked check, ice_setup_tc() skips pf->adev_mutex and reconfigures
queues concurrently with auxiliary device registration. Should
pf->adev_mutex be acquired before checking cdev->adev, matching
ice_send_event_to_aux()?

[Severity: Medium]
What happens if device_trylock(&cdev->adev->dev) fails when ice_setup_tc()
is called to tear down an existing MQPRIO offload (when
mqprio_qopt->qopt.hw == 0)?

When an offloaded MQPRIO qdisc is deleted or replaced (on a PF where
cdev->adev is registered and no RDMA driver is bound),
mqprio_destroy() -> mqprio_disable_offload() in net/sched/sch_mqprio.c
invokes ndo_setup_tc() and ignores the return value:

net/sched/sch_mqprio.c:mqprio_disable_offload() {
	...
	case TC_MQPRIO_MODE_DCB:
	case TC_MQPRIO_MODE_CHANNEL:
		dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO,
					      &mqprio);
		break;
}

If another thread transiently holds device_lock(&cdev->adev->dev) (such as
a sysfs attribute read or bus probe/rescan), returning -EBUSY here skips
ice_setup_tc_mqprio_qdisc():

drivers/net/ethernet/intel/ice/ice_main.c:ice_setup_tc_mqprio_qdisc() {
	...
	if (!hw) {
		clear_bit(ICE_FLAG_TC_MQPRIO, pf->flags);
		vsi->ch_rss_size = 0;
		memcpy(&vsi->mqprio_qopt, mqprio_qopt, sizeof(*mqprio_qopt));
		goto config_tcf;
	}
}

Because the qdisc destruction still completes in the core while
ice_setup_tc_mqprio_qdisc() is skipped, won't this leave
ICE_FLAG_TC_MQPRIO set, leak the ADQ channel VSIs on vsi->ch_list by
skipping ice_remove_q_channels(), and leave netdev TC state and queue
counts out of sync with no way for userspace to retry the teardown?

>  			locked = true;
>  			if (cdev->adev->dev.driver) {
>  				netdev_err(netdev, "Cannot change qdisc when RDMA is active\n");

[Severity: High]
Does holding pf->adev_mutex and cdev->adev->dev.mutex across
mutex_lock(&pf->tc_mutex) in ice_setup_tc() invert the lock order against
the DCB paths?

In ice_setup_tc() in drivers/net/ethernet/intel/ice/ice_main.c, when
cdev->adev->dev.driver is NULL, the lock acquisition order is:

  rtnl_lock -> pf->adev_mutex -> cdev->adev->dev.mutex -> pf->tc_mutex

However, the DCB netlink handlers in
drivers/net/ethernet/intel/ice/ice_dcb_nl.c (such as ice_dcbnl_setets(),
ice_dcbnl_setpfc(), ice_dcbnl_setapp(), ice_dcbnl_delapp(), and
ice_dcbnl_cee_set_all()) acquire pf->tc_mutex before calling
ice_pf_dcb_cfg() -> ice_send_event_to_aux(), which acquires
pf->adev_mutex:

drivers/net/ethernet/intel/ice/ice_dcb_nl.c:ice_dcbnl_setets() {
	...
	mutex_lock(&pf->tc_mutex);
	...
	err = ice_pf_dcb_cfg(pf, new_cfg, true);
	...
}

And in drivers/net/ethernet/intel/ice/ice_dcb_lib.c,
ice_dcb_process_lldp_set_mib_change() and ice_dcb_rebuild() run from
ice_service_task() without rtnl_lock() and acquire pf->tc_mutex before
both rtnl_lock() and pf->adev_mutex:

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_dcb_process_lldp_set_mib_change() {
	...
	mutex_lock(&pf->tc_mutex);
	...
	rtnl_lock();
	...
	ice_pf_dcb_recfg(pf, false);
		-> ice_send_event_to_aux()
			-> mutex_lock(&pf->adev_mutex);
}

Could this trigger a lockdep circular locking dependency warning or an
ABBA deadlock if ice_dcb_process_lldp_set_mib_change() or
ice_dcb_rebuild() holds pf->tc_mutex while waiting for rtnl_lock() or
pf->adev_mutex, at the same time ice_setup_tc() holds rtnl_lock() and
pf->adev_mutex while waiting for pf->tc_mutex?

[Severity: High]
Also related to the series change in ice_send_event_to_aux() in
drivers/net/ethernet/intel/ice/ice_idc.c: when ice_send_event_to_aux() uses
device_trylock(&cdev->adev->dev), can DCB reconfiguration leave the
devlink rate tree torn down or leave RDMA QPs permanently suspended?

First, in ice_pf_dcb_cfg() in
drivers/net/ethernet/intel/ice/ice_dcb_lib.c,
ice_tear_down_devlink_rate_tree(pf) is called before sending
IIDC_RDMA_EVENT_BEFORE_TC_CHANGE:

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_pf_dcb_cfg() {
	...
	if (ice_dcb_get_num_tc(new_cfg) > 1) {
		...
		ice_tear_down_devlink_rate_tree(pf);

		set_bit(ICE_FLAG_DCB_ENA, pf->flags);
	}
	...
	set_bit(IIDC_RDMA_EVENT_BEFORE_TC_CHANGE, event->type);
	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;
	}
}

If ice_send_event_to_aux() fails its trylock and returns false,
ICE_FLAG_DCB_ENA is restored, but the devlink rate tree has already been
torn down.

Second, when IIDC_RDMA_EVENT_BEFORE_TC_CHANGE is delivered,
icrdma_iidc_event_handler() -> icrdma_prep_tc_change() in
drivers/infiniband/hw/irdma/icrdma_if.c sets
iwdev->vsi.tc_change_pending = true and suspends RDMA QPs, and
ice_send_event_to_aux() immediately releases cdev->adev->dev.mutex. Later,
ice_pf_dcb_recfg() sends IIDC_RDMA_EVENT_AFTER_TC_CHANGE via a separate
ice_send_event_to_aux() call and ignores the return value:

drivers/net/ethernet/intel/ice/ice_dcb_lib.c:ice_pf_dcb_recfg() {
	...
	set_bit(IIDC_RDMA_EVENT_AFTER_TC_CHANGE, event->type);
	ice_send_event_to_aux(pf, event);
	kfree(event);
}

If &cdev->adev->dev is contended when IIDC_RDMA_EVENT_AFTER_TC_CHANGE is
sent, ice_send_event_to_aux() drops the event, so the RDMA driver never
receives AFTER_TC_CHANGE to clear tc_change_pending and resume its QPs.

[ ... ]

>
> -- 
> 2.51.2
>

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

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

[Przemek Kitszel]

> 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(-)
> 

trylock is clearly an antipattern, but we don't have anything better,
especially for a -net material

side note: we have resolved the same issue via rtnl_unlock()&relock()
in OOT (to be published within days), which is even worse anti-pattern

Would be best to have proper API as -next thing, glad to hear your
support on that!

side note2: I have a few fixes around the thing you touches, but no
conflicts so far, will CC you