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

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@xxxxxxxxxx?part=1