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

[sashiko-bot@kernel.org]

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