Re: [PATCH] net: dsa: vitesse-vsc73xx: Fix SPI device reference leak in vsc73xx_spi_probe()
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-21 11:09:04
Also in:
lkml, stable
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 1 · Medium: 0 · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [High] Incomplete fix: the newly added spi_dev_put() calls do not cover the shutdown()->remove() ordering, so the reference leak the commit… -- net: dsa: vitesse-vsc73xx: Fix SPI device reference leak in vsc73xx_spi_probe() This adds spi_dev_put() calls to balance the spi_dev_get() done in vsc73xx_spi_probe(), covering the two probe error paths and vsc73xx_spi_remove().
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/vitesse-vsc73xx-spi.c b/drivers/net/dsa/vitesse-vsc73xx-spi.c index 85b9a0f51dd89..7b784320c41e7 100644 --- a/drivers/net/dsa/vitesse-vsc73xx-spi.c +++ b/drivers/net/dsa/vitesse-vsc73xx-spi.c@@ -153,10 +153,15 @@ static int vsc73xx_spi_probe(struct spi_device *spi) ret = spi_setup(spi); if (ret < 0) { dev_err(dev, "spi setup failed.\n"); + spi_dev_put(vsc_spi->spi); return ret; } - return vsc73xx_probe(&vsc_spi->vsc); + ret = vsc73xx_probe(&vsc_spi->vsc); + if (ret) + spi_dev_put(vsc_spi->spi); + + return ret; } static void vsc73xx_spi_remove(struct spi_device *spi)@@ -167,6 +172,8 @@ static void vsc73xx_spi_remove(struct spi_device *spi) return; vsc73xx_remove(&vsc_spi->vsc); + + spi_dev_put(vsc_spi->spi); } static void vsc73xx_spi_shutdown(struct spi_device *spi)
[Severity: High] Does this still leak the spi_device reference when shutdown() runs before remove()? The new spi_dev_put() sits after the NULL drvdata guard in vsc73xx_spi_remove(): struct vsc73xx_spi *vsc_spi = spi_get_drvdata(spi); if (!vsc_spi) return; vsc73xx_remove(&vsc_spi->vsc); spi_dev_put(vsc_spi->spi); and vsc73xx_spi_shutdown() deliberately clears drvdata without dropping the reference: vsc73xx_shutdown(&vsc_spi->vsc); spi_set_drvdata(spi, NULL); Documentation/networking/dsa/dsa.rst describes this ordering as permitted: to be called even if ``shutdown()`` was already called. Therefore, drivers are expected to implement a mutual exclusion method between ``remove()`` and ``shutdown()`` by setting their drvdata to NULL after any of these has run So on reboot followed by SPI controller teardown, for example dspi_shutdown() -> dspi_remove() -> spi_unregister_controller() -> spi_unregister_device() -> device_del() -> vsc73xx_spi_remove(), the remove callback returns early at the !vsc_spi check and the extra reference on the spi_device (and its per-CPU spi_statistics) is never dropped. The commit message does not mention the shutdown path. Would it be simpler to drop the reference entirely instead of adding matching puts, i.e. in vsc73xx_spi_probe(): vsc_spi->spi = spi; vsc_spi is devm_kzalloc()'d on &spi->dev, and vsc_spi->spi is only dereferenced from vsc73xx_spi_read()/vsc73xx_spi_write() while the driver is bound, a window in which the driver core already keeps the spi_device alive. This file is also the only user of spi_dev_get()/spi_dev_put() outside drivers/spi and drivers/platform/x86; peer DSA SPI drivers just store the pointer, see b53_spi_probe() in drivers/net/dsa/b53/b53_spi.c and ksz_spi_probe() in drivers/net/dsa/microchip/ksz_spi.c. That would make every path, including shutdown, symmetric without adding three call sites to keep in sync. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917104920.2147568-1-vulab%40iscas.ac.cn