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

[sashiko-bot@kernel.org]

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