Re: [PATCH v5 net-next 4/9] net: dsa: netc: tie setup resources and reset to DSA setup/teardown

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dsa: AB-BA deadlock between dsa2_mutex and pci device_lock during switch setup/teardown
- [High] netc: race condition in netc_switch_reset() leading to unheld PCI device lock
--

commit f362c9d18af3d66bf5d5401a43abc544295b1901
Author: Wei Fang <wei.fang@nxp.com>
Subject: net: dsa: netc: tie setup resources and reset to DSA setup/teardown

This commit modifies the NETC switch driver to explicitly allocate and free
resources during DSA setup and teardown, instead of relying on devm allocations
during PCI probe. It also adds a PCI reset into netc_setup() to ensure a
clean hardware state when starting up.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> +static int netc_switch_reset(struct netc_switch *priv)
> +{
> +	if (priv->in_probe)
> +		return pci_reset_function_locked(priv->pdev);

[Severity: High]
Can a concurrent thread execute this path without holding the PCI device
lock?

In a multi-switch DSA tree, priv->in_probe is set to true during
netc_switch_probe(). The thread then calls dsa_register_switch(), which
internally acquires and releases dsa2_mutex. If the DSA tree is incomplete,
it returns, and the thread can be preempted before executing
priv->in_probe = false.

Concurrently, a second thread could probe the second switch, complete the
tree, acquire dsa2_mutex, and call dsa_tree_setup_switches(). This invokes
netc_setup() for the first switch. Observing priv->in_probe == true, the
second thread would call pci_reset_function_locked() on the first switch's
PCI device, despite not holding its device lock.

> +
> +	return pci_reset_function(priv->pdev);

[Severity: High]
Does this introduce an AB-BA deadlock between the PCI device lock and
dsa2_mutex?

When a multi-switch DSA tree is set up, netc_setup() can be called for an
already-probed switch. In this case, priv->in_probe is false, and
netc_switch_reset() calls pci_reset_function(), which acquires the PCI
device lock. However, netc_setup() is invoked by the DSA core with the
global dsa2_mutex held.

Conversely, when a switch is unbound (e.g. via sysfs), the driver core first
acquires the switch's device lock and then calls the driver's remove
callback. The netc_switch_remove() function calls dsa_unregister_switch(),
which attempts to acquire dsa2_mutex:

net/dsa/dsa.c:dsa_unregister_switch() {
    ...
    mutex_lock(&dsa2_mutex);
    dsa_switch_remove(ds);
    mutex_unlock(&dsa2_mutex);
    ...
}

If Thread 1 is unbinding Switch A (holding the device lock for A and waiting
for dsa2_mutex) while Thread 2 is probing Switch B (holding dsa2_mutex and
calling pci_reset_function(A) inside netc_setup(A)), the system could
deadlock.

> +}

[ ... ]

> @@ -2475,12 +2565,15 @@ static int netc_switch_probe(struct pci_dev *pdev,
>  	ds->ageing_time_max = U32_MAX;
>  	ds->priv = priv;
>  	priv->ds = ds;
> +	priv->in_probe = true;
>  
>  	err = dsa_register_switch(ds);
>  	if (err)
>  		return dev_err_probe(dev, err,
>  				     "Failed to register DSA switch\n");
>  
> +	priv->in_probe = false;
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012095924.1129312-1-wei.fang@oss.nxp.com?part=4