* Re: [PATCH] net: dsa: mv88e6xxx: fix netdev reference leak on remove
2026-09-16 2:03 [PATCH] net: dsa: mv88e6xxx: fix netdev reference leak on remove Guangshuo Li
@ 2026-09-16 12:09 ` Andrew Lunn
2026-09-17 20:05 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: Andrew Lunn @ 2026-09-16 12:09 UTC (permalink / raw)
To: Guangshuo Li
Cc: Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Florian Fainelli, netdev, linux-kernel, stable
On Wed, Sep 16, 2026 at 10:03:40AM +0800, Guangshuo Li wrote:
> mv88e6xxx_probe() uses the referenced network device provided through
> platform data, but the remove path does not drop the corresponding
> reference.
>
> The probe failure path correctly calls dev_put() for pdata->netdev.
> However, after a successful probe, mv88e6xxx_remove() tears down the
> switch resources without releasing this reference, leaving the network
> device reference count unbalanced after driver removal.
>
> Call dev_put() in mv88e6xxx_remove() after the switch resources have
> been torn down.
dev_put() and dev_get() should be used in pairs. Where is the
dev_get()?
Andrew
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net: dsa: mv88e6xxx: fix netdev reference leak on remove
2026-09-16 2:03 [PATCH] net: dsa: mv88e6xxx: fix netdev reference leak on remove Guangshuo Li
2026-09-16 12:09 ` Andrew Lunn
@ 2026-09-17 20:05 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-09-17 20:05 UTC (permalink / raw)
To: lgs201920130244
Cc: andrew, olteanv, davem, edumazet, kuba, pabeni, f.fainelli,
netdev, linux-kernel, stable
Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 2 · Low: 0
- [High] mv88e6xxx_remove() (drivers/net/dsa/mv88e6xxx/chip.c:7499-7500)
now calls dev_put(pdata->netdev), but the mv88e6xxx driver never…
- [Medium] The changelog states as established fact that
'mv88e6xxx_probe() uses the referenced network device provided through…
- [Medium] The commit message asserts 'The probe failure path correctly
calls dev_put() for pdata->netdev', but one probe failure exit still…
--
net: dsa: mv88e6xxx: fix netdev reference leak on remove
This adds a dev_put(pdata->netdev) call at the end of mv88e6xxx_remove()
to mirror the dev_put() that already exists on the probe error path, on
the grounds that a successful probe leaves the platform-data supplied
net_device reference unbalanced. The change is tagged Fixes: and
Cc: stable.
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 7f68a0c558026..4869fcba34cb4 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> mv88e6xxx_probe() uses the referenced network device provided through
> platform data, but the remove path does not drop the corresponding
> reference.
>
> The probe failure path correctly calls dev_put() for pdata->netdev.
[Severity: Medium]
Can the phrase "uses the referenced network device" be backed up by code in
the driver? Searching drivers/net/dsa/mv88e6xxx/ for an acquire turns up
nothing: there is no dev_hold(), no netdev_hold() and no dev_get_by_name()
anywhere in the driver. The only two reference operations are the existing
dev_put() at the probe out: label and the one this patch adds.
What mv88e6xxx_probe() actually does with pdata->netdev is validate it and
publish its embedded struct device:
if (pdata) {
compat_info = pdata_device_get_match_data(dev);
if (!pdata->netdev)
return -EINVAL;
for (port = 0; port < DSA_MAX_PORTS; port++) {
...
pdata->cd.netdev[port] = &pdata->netdev->dev;
break;
}
}
The DSA core does its own balanced accounting for the conduit in
dsa_port_parse():
conduit = to_net_dev(d);
netdev_hold(conduit, &dp->conduit_tracker, GFP_KERNEL);
put_device(d);
and releases it from dsa_switch_release_ports() (reached via
dsa_unregister_switch()) and dsa_switch_shutdown().
Also, include/linux/platform_data/mv88e6xxx.h documents only that cd must
come first; it says nothing about who holds or drops the netdev reference.
Could the changelog state which side of the interface takes the reference,
and could that rule be written down in the header if the patch is respun?
One more behavioural note for backporters: before this patch the reference
was consumed only on probe failure and retained on success; afterwards it is
also consumed on remove. A provider written against the old behaviour
(hold before mdio_device_register(), put after removal) would now double-put.
> @@ -7472,6 +7472,7 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>
> static void mv88e6xxx_remove(struct mdio_device *mdiodev)
> {
> + struct dsa_mv88e6xxx_pdata *pdata = mdiodev->dev.platform_data;
> struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
> struct mv88e6xxx_chip *chip;
>
> @@ -7494,6 +7495,9 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
> mv88e6xxx_irq_poll_free(chip);
>
> mv88e6xxx_phy_destroy(chip);
> +
> + if (pdata)
> + dev_put(pdata->netdev);
> }
[Severity: High]
Can this underflow the refcount of pdata->netdev? Since the driver never
acquires a reference on that net_device, this put has no matching get.
Platform data outlives a single binding, so the put here is not idempotent
across the device lifetime:
bind -> mv88e6xxx_probe() takes no reference
unbind -> mv88e6xxx_remove() dev_put(pdata->netdev)
bind -> mv88e6xxx_probe() takes no reference
unbind -> mv88e6xxx_remove() dev_put(pdata->netdev)
The same holds for a deferred or failed probe followed by a successful one:
the out: label in mv88e6xxx_probe() already issues one put, and remove now
issues another, for at most one donated reference.
out:
if (pdata)
dev_put(pdata->netdev);
return err;
Once the count drops below what was taken, the conduit net_device can be
freed while pdata->cd.netdev[] still points at its embedded struct device,
which the next probe dereferences through dev_find_class()/get_device()/
to_net_dev() in dsa_port_parse().
Would it be better to make the acquire side explicit, i.e. add a
dev_hold()/netdev_hold() in mv88e6xxx_probe() paired with puts on both the
error and remove paths, or alternatively drop the unmatched dev_put() in the
probe error path? As written the patch extends the unmatched-put pattern
rather than balancing it.
[Severity: Medium]
Taking the ownership model in this patch at face value, is one probe failure
exit still missing the put? After the pdata block has validated
pdata->netdev and stored &pdata->netdev->dev into pdata->cd.netdev[port],
the compatible check in mv88e6xxx_probe() returns directly:
if (!compat_info)
return -EINVAL;
pdata_device_get_match_data() returns NULL whenever pdata->compatible
matches no entry of the driver's of_match_table:
for (; matches->name[0] || matches->type[0] || matches->compatible[0];
matches++) {
if (!strcmp(pdata->compatible, matches->compatible))
return matches->data;
}
return NULL;
so that path skips the out: label and its dev_put(pdata->netdev), which
contradicts the changelog statement that "The probe failure path correctly
calls dev_put() for pdata->netdev". Would changing it to
"err = -EINVAL; goto out;" be appropriate here?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916020340.2580417-1-lgs201920130244%40gmail.com
^ permalink raw reply [flat|nested] 3+ messages in thread