Re: [PATCH iwl-net v2 3/5] 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: premature AFTER_TC_CHANGE and unlock before ice_dcb_noncontig_cfg()
- [High] ice: inconsistent QoS and DCB state on error path in ice_pf_dcb_cfg()
- [Medium] ice: unrolled state mutations when ice_pf_dcb_cfg() fails with -EBUSY
--

commit 0b49c36f284346b01578d0498826a110d7c3a315
Author: Petr Oros <poros@xxxxxxxxxx>

ice: avoid RTNL deadlock when notifying the RDMA auxiliary driver

This commit avoids an ABBA deadlock between rtnl_lock and the RDMA
auxiliary device lock by acquiring the auxiliary device lock with
device_trylock() via ice_aux_dev_trylock() around RDMA event delivery.
In ice_pf_dcb_cfg(), the locks are held across the DCB reconfiguration
from IIDC_RDMA_EVENT_BEFORE_TC_CHANGE through
IIDC_RDMA_EVENT_AFTER_TC_CHANGE, and the non-contiguous traffic class
fallback is deferred until after the locks are released.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
> index 0bc6dd3756879..9916bd408071c 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
> @@ -352,11 +355,16 @@ 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 noncontig = false;
> +	bool dcb_ena;
>  
>  	curr_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
> +	dcb_ena = test_bit(ICE_FLAG_DCB_ENA, pf->flags);

[ ... ]

> @@ -405,16 +413,21 @@ 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_aux_dev_trylock(pf)) {
> +		assign_bit(ICE_FLAG_DCB_ENA, pf->flags, dcb_ena);
> +		ret = -EBUSY;
> +		goto unlock_rtnl;
> +	}

[Severity: Medium]
When ice_aux_dev_trylock(pf) fails here and returns -EBUSY, what happens to
the state mutations that already took place earlier in ice_pf_dcb_cfg() and
its callers?

Earlier in ice_pf_dcb_cfg(), when ice_dcb_get_num_tc(new_cfg) > 1,
ice_tear_down_devlink_rate_tree(pf) has already run before
ice_aux_dev_trylock(pf):

    if (ice_dcb_get_num_tc(new_cfg) > 1) {
        if (pf->hw.port_info->is_custom_tx_enabled) {
            ...
            return -EBUSY;
        }
        ice_tear_down_devlink_rate_tree(pf);

        set_bit(ICE_FLAG_DCB_ENA, pf->flags);
    }

Unlike the is_custom_tx_enabled check, failing at ice_aux_dev_trylock(pf)
reverts ICE_FLAG_DCB_ENA while leaving the devlink rate tree torn down.

In addition, callers that configure 1 TC (where is_custom_tx_enabled could
never trigger), such as 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():

    memset(&pi->qos_cfg.local_dcbx_cfg, 0, sizeof(*dcbcfg));
    ...
    ret = ice_pf_dcb_cfg(pf, dcbcfg, locked);

If ice_aux_dev_trylock(pf) returns -EBUSY before curr_cfg is populated,
does local_dcbx_cfg remain completely zeroed out? Similarly, in
ice_dcbnl_setapp(), a negative return from ice_pf_dcb_cfg() is converted to
ICE_DCB_NO_HW_CHG without rolling back dcb_ieee_setapp(), dscp_mapped,
desired_dcbx_cfg, or ice_aq_set_pfc_mode().

> +
> +	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);
>  
> @@ -441,13 +454,28 @@ 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);
> +	noncontig = !ice_dcb_tc_contig(curr_cfg->etscfg.prio_table);
>  
>  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);

[Severity: High]
Can jumping to out on error leave curr_cfg, ICE_FLAG_DCB_ENA, and
privd->qos_info inconsistent with the actual VSI configuration and cause
active RDMA queue pairs to transition to the error state?

In ice_pf_dcb_cfg(), if ice_set_dcb_cfg() succeeds (updating firmware and
PRTDCB_TUP2TC) but ice_query_port_ets() fails and jumps to out:

1. curr_cfg still contains new_cfg (unlike the ice_set_dcb_cfg() failure
   path, which restores old_cfg), and neither error path restores
   ICE_FLAG_DCB_ENA to dcb_ena.

2. Because ice_pf_dcb_recfg(pf, true) was skipped, vsi->tc_cfg.ena_tc still
   only has the old traffic classes enabled.

3. At out, ice_setup_dcb_qos_info(pf, &privd->qos_info) populates
   privd->qos_info from the new curr_cfg and PRTDCB_TUP2TC register, and
   ice_send_event_to_aux() delivers IIDC_RDMA_EVENT_AFTER_TC_CHANGE.

When irdma handles IIDC_RDMA_EVENT_AFTER_TC_CHANGE via
icrdma_iidc_event_handler() -> irdma_change_l2params() ->
irdma_sc_suspend_resume_qps(), it attempts to resume QPs on the new TCs in
qos_info->up2tc[] via irdma_ws_add() -> icrdma_lan_register_qset() ->
ice_add_rdma_qset(). Since vsi->tc_cfg.ena_tc was never updated for those
new TCs, ice_ena_vsi_rdma_qset() fails with -EINVAL, causing
irdma_sc_suspend_resume_qps() to call irdma_modify_qp_to_err() on the QPs.

> +	ice_aux_dev_unlock(pf);
> +
>  	/* enable previously downed VSIs */
>  	ice_dcb_ena_dis_vsi(pf, true, true);
> +	if (noncontig)
> +		ice_dcb_noncontig_cfg(pf);

[Severity: High]
When a DCB configuration has non-contiguous traffic classes in
curr_cfg->etscfg.prio_table, does deferring ice_dcb_noncontig_cfg(pf) until
after IIDC_RDMA_EVENT_AFTER_TC_CHANGE and ice_aux_dev_unlock(pf) break RDMA
QP resume?

Previously, ice_pf_dcb_cfg() called ice_pf_dcb_recfg(pf, false), which saw
!ice_dcb_tc_contig(dcbcfg->etscfg.prio_table) and immediately ran
ice_dcb_noncontig_cfg(pf) to reset local_dcbx_cfg and hardware PRTDCB_TUP2TC
back to the default TC0 configuration before populating privd->qos_info,
sending IIDC_RDMA_EVENT_AFTER_TC_CHANGE, or re-enabling the VSIs.

With this change:

1. ice_set_dcb_cfg() programs the non-contiguous prio_table into hardware
   PRTDCB_TUP2TC.

2. ice_pf_dcb_recfg(pf, true) configures the PF VSI with
   tc_map = ICE_DFLT_TRAFFIC_CLASS (only TC0 enabled in vsi->tc_cfg.ena_tc)
   and skips ice_dcb_noncontig_cfg(pf) because locked is true.

3. Before ice_dcb_noncontig_cfg(pf) runs, ice_setup_dcb_qos_info() reads
   PRTDCB_TUP2TC from hardware:

    dcbx_cfg = &pf->hw.port_info->qos_cfg.local_dcbx_cfg;
    up2tc = rd32(&pf->hw, PRTDCB_TUP2TC);

    qos_info->num_tc = ice_dcb_get_num_tc(dcbx_cfg);

    for (i = 0; i < IIDC_MAX_USER_PRIORITY; i++)
        qos_info->up2tc[i] = (up2tc >> (i * 3)) & 0x7;

   This populates qos_info->up2tc[] with the un-reset non-contiguous TCs
   (> 0) while vsi->tc_cfg.ena_tc only has BIT(0) set.

4. ice_send_event_to_aux() then sends IIDC_RDMA_EVENT_AFTER_TC_CHANGE. When
   irdma resumes QPs via irdma_sc_suspend_resume_qps(), irdma_ws_add() ->
   ice_add_rdma_qset() -> ice_ena_vsi_rdma_qset() fails with -EINVAL for
   any TC > 0, and irdma_modify_qp_to_err() transitions the QPs to the
   error state.

5. Finally, ice_aux_dev_unlock(pf) drops the locks and
   ice_dcb_ena_dis_vsi(pf, true, true) re-enables the VSIs before
   ice_dcb_noncontig_cfg(pf) resets the configuration, and if the second
   ice_aux_dev_trylock(pf) inside ice_dcb_noncontig_cfg() ->
   ice_dcb_sw_dflt_cfg() -> ice_pf_dcb_cfg() fails with -EBUSY, the fallback
   aborts with local_dcbx_cfg zeroed out.

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