[PATCH] net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

[PATCH] net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

[Selby Mashiki]

In lan966x_probe(), fwnode_handle_put(ports) is called after the
fwnode_for_each_available_child_node(ports, portnp) loop finishes.
However, if lan966x_fdb_init(), lan966x_ptp_init(), lan966x_fdma_init(),
or lan966x_vcap_init() subsequently fails, execution jumps to or falls
through to cleanup_ports, calling fwnode_handle_put(ports) a second time
and skipping lan966x_mdb_deinit().

Add cleanup_mdb and cleanup_switch labels so post-loop initialization
failures deinitialize MDB and clean up ports without calling
fwnode_handle_put() on ports again.

Signed-off-by: Selby Mashiki <mashikiselby@gmail.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 1179a6e12..7abfd97ad 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -1228,7 +1228,7 @@ static int lan966x_probe(struct platform_device *pdev)
 	lan966x_mdb_init(lan966x);
 	err = lan966x_fdb_init(lan966x);
 	if (err)
-		goto cleanup_ports;
+		goto cleanup_mdb;
 
 	err = lan966x_ptp_init(lan966x);
 	if (err)
@@ -1255,10 +1255,15 @@ static int lan966x_probe(struct platform_device *pdev)
 cleanup_fdb:
 	lan966x_fdb_deinit(lan966x);
 
+cleanup_mdb:
+	lan966x_mdb_deinit(lan966x);
+	goto cleanup_switch;
+
 cleanup_ports:
 	fwnode_handle_put(ports);
 	fwnode_handle_put(portnp);
 
+cleanup_switch:
 	lan966x_cleanup_ports(lan966x);
 
 	cancel_delayed_work_sync(&lan966x->stats_work);
-- 
2.56.0

Re: [PATCH] net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

[Selby Mashiki]

Hi,

Here is the additional information for this patch as well:

- Discovery: Found via a static analysis script scanning for
  fwnode_handle reference count imbalances across error paths, followed
  by manual code inspection of lan966x_probe().
- Triggered vs. theoretical: Theoretical (found by static analysis and
  code inspection, not triggered at runtime).
- Hardware testing: Compile-tested only (-Werror); not tested on real
  Microchip LAN966x hardware as I do not have access to the device.

Best regards,
Selby Mashiki

Re: [PATCH] net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

[Daniel Machon]

Hi Selby,

> In lan966x_probe(), fwnode_handle_put(ports) is called after the
> fwnode_for_each_available_child_node(ports, portnp) loop finishes.
> However, if lan966x_fdb_init(), lan966x_ptp_init(), lan966x_fdma_init(),
> or lan966x_vcap_init() subsequently fails, execution jumps to or falls
> through to cleanup_ports, calling fwnode_handle_put(ports) a second time
> and skipping lan966x_mdb_deinit().
> 
> Add cleanup_mdb and cleanup_switch labels so post-loop initialization
> failures deinitialize MDB and clean up ports without calling
> fwnode_handle_put() on ports again.
> 
> Signed-off-by: Selby Mashiki <mashikiselby@gmail.com>
> ---
>  drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 1179a6e12..7abfd97ad 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -1228,7 +1228,7 @@ static int lan966x_probe(struct platform_device *pdev)
>  	lan966x_mdb_init(lan966x);
>  	err = lan966x_fdb_init(lan966x);
>  	if (err)
> -		goto cleanup_ports;
> +		goto cleanup_mdb;
>  
>  	err = lan966x_ptp_init(lan966x);
>  	if (err)
> @@ -1255,10 +1255,15 @@ static int lan966x_probe(struct platform_device *pdev)
>  cleanup_fdb:
>  	lan966x_fdb_deinit(lan966x);
>  
> +cleanup_mdb:
> +	lan966x_mdb_deinit(lan966x);
> +	goto cleanup_switch;
> +
>  cleanup_ports:
>  	fwnode_handle_put(ports);
>  	fwnode_handle_put(portnp);
>  
> +cleanup_switch:
>  	lan966x_cleanup_ports(lan966x);
>  
>  	cancel_delayed_work_sync(&lan966x->stats_work);
> -- 
> 2.56.0

This might fix the mentioned deinit issues listed in the commit description,
however it is still racy with the netdevs, which are unregistered at a later
point.

As I mentioned in my lan966x-pci series [1], I will redo the whole error/deinit
path in a series for net-next, which will fix it for good, including the issues
in this patch.

I can't reject this with pw-bot, since I am currently not listed as a maintainer
for lan966x. Horatiu is pre-occupied with PHY stuff, so he probably wont reply
either. I will add a patch that adds me to maintainers once net-next opens, so
patches are routed properly to me.

[1] https://lore.kernel.org/netdev/20261002090248.xwlcm3zclt6imaou@DEN-DL-M70577/

/Daniel

Re: [PATCH] net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 (1) · Low: 1

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] The new cleanup_mdb label calls lan966x_mdb_deinit()
  (lan966x_main.c:1302) and then jumps to cleanup_switch, so the…
- [Low] The commit fixes a real refcount bug but has no Fixes: trailer.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the reworked
  error ladder keeps it.
- [High] This problem wasn't introduced by this patch, but it is in the
  error path the patch reworks.
- [Medium] The probe error ladder in lan966x_probe() never calls
  lan966x_mac_purge_entries(), unlike lan966x_remove() (line 1332).

--

net: lan966x: fix double fwnode_handle_put() and missing mdb deinit in probe error path

This commit reworks the error ladder in lan966x_probe(). Failures after
the port loop no longer reach cleanup_ports, so fwnode_handle_put(ports)
is not called a second time. A new cleanup_mdb label calls
lan966x_mdb_deinit(), and a new cleanup_switch label skips the fwnode
puts and goes straight to lan966x_cleanup_ports().

> In lan966x_probe(), fwnode_handle_put(ports) is called after the
> fwnode_for_each_available_child_node(ports, portnp) loop finishes.
> However, if lan966x_fdb_init(), lan966x_ptp_init(), lan966x_fdma_init(),
> or lan966x_vcap_init() subsequently fails, execution jumps to or falls
> through to cleanup_ports, calling fwnode_handle_put(ports) a second time
> and skipping lan966x_mdb_deinit().

[ ... ]

> Signed-off-by: Selby Mashiki <mashikiselby@gmail.com>

[Severity: Low]
Should this have a Fixes: tag?

Both the fwnode_handle_put(ports) after the port loop and the one under
cleanup_ports came in with commit 925f3deb45df ("net: lan966x: add
missing fwnode_handle_put() for ports node"). That commit has its own
Fixes: tag, so the double put is probably in stable trees as well.

Something like:

Fixes: 925f3deb45df ("net: lan966x: add missing fwnode_handle_put() for ports node")

and possibly, for the missing MDB deinit:

Fixes: 7aacb894b1ad ("net: lan966x: Extend switchdev with mdb support")

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index e4c9c5812ccab..887385833b4d6 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -1271,7 +1271,7 @@ static int lan966x_probe(struct platform_device *pdev)
>  	lan966x_mdb_init(lan966x);
>  	err = lan966x_fdb_init(lan966x);
>  	if (err)
> -		goto cleanup_ports;
> +		goto cleanup_mdb;
>  
>  	err = lan966x_ptp_init(lan966x);
>  	if (err)

[Severity: High]
This is a pre-existing issue, but it sits in the error path being
reworked here. What happens to PTP clocks that are already registered
when lan966x_ptp_init() fails partway through?

lan966x_ptp_init() registers LAN966X_PHC_COUNT clocks and returns on the
first failure:

drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c:lan966x_ptp_init() {
    ...
	for (i = 0; i < LAN966X_PHC_COUNT; ++i) {
		err = lan966x_ptp_phc_init(lan966x, i, &lan966x_ptp_clock_info);
		if (err)
			return err;
	}
    ...
}

Suppose ptp_clock_register() fails for PHC 1 or 2. The clocks registered
before it stay registered, and lan966x_probe() jumps to cleanup_fdb,
which skips lan966x_ptp_deinit().

Those clocks have ptp->info pointing at lan966x->phc[i].info. That is
inside the devm-allocated struct lan966x, which is freed when probe
fails.

After that, could reading /sys/class/ptp/ptpN/clock_name (clock_name_show()
dereferences ptp->info->name), or any operation on /dev/ptpN, touch freed
memory?

lan966x_ptp_deinit() unregisters every PHC unconditionally, so it can't
just be called here. lan966x_ptp_init() would need its own unwind for
the clocks it already registered.

[ ... ]

> @@ -1298,10 +1298,15 @@ static int lan966x_probe(struct platform_device *pdev)
>  cleanup_fdb:
>  	lan966x_fdb_deinit(lan966x);

[Severity: High]
This isn't a bug introduced by this patch, and the reworked ladder keeps
it. lan966x_fdb_deinit() destroys lan966x->fdb_work before
lan966x_cleanup_ports() calls unregister_netdev() on the ports.

Suppose a port is already in a bridge when the ptp, fdma or vcap init
fails. Would unregistering that port go through this chain on a
workqueue that has already been destroyed?

lan966x_cleanup_ports()
  unregister_netdev()
    bridge unlink -> NETDEV_PRECHANGEUPPER
      lan966x_port_prechangeupper()
        lan966x_fdb_flush_workqueue()
          flush_workqueue(lan966x->fdb_work)

FDB events on the still-registered ports in that window would also reach
lan966x_handle_fdb()->queue_work(lan966x->fdb_work, ...) on the freed
workqueue.

If lan966x_fdb_init() itself fails (now goto cleanup_mdb), fdb_work is
NULL. Would the same unregister path then end up in flush_workqueue(NULL)
or queue_work(NULL, ...)?

lan966x_remove() avoids this by calling lan966x_fdb_deinit() only after
lan966x_cleanup_ports().

>  
> +cleanup_mdb:
> +	lan966x_mdb_deinit(lan966x);
> +	goto cleanup_switch;

[Severity: High]
Does this free the MDB and PGID lists while the port netdevs are still
registered?

lan966x_probe_port() calls register_netdev() inside the port loop. The
matching unregister_netdev() only runs later, in lan966x_cleanup_ports()
under cleanup_switch.

lan966x_mdb_purge_mdb_entries() and lan966x_mdb_purge_pgid_entries() call
list_del() and kfree() without taking any lock. During this window, the
switchdev notifier can still deliver PORT_MDB/HOST_MDB add and del for
these ports under rtnl, for example on bridge join replay or a bridge mdb
add from userspace:

lan966x_mdb_ip_add()
  lan966x_mdb_entry_get() / lan966x_mdb_entry_add()
    list_add_tail(&mdb_entry->list, &lan966x->mdb_entries);

Could the unlocked purge race with these handlers, corrupt the list, or
free an entry a handler is still using? Entries added after the purge
but before unregister_netdev() would also leak.

lan966x_remove() does it the other way round:

	lan966x_cleanup_ports(lan966x);
	...
	lan966x_mdb_deinit(lan966x);

Should lan966x_mdb_deinit() also run after lan966x_cleanup_ports() in
the probe error path?

> +
>  cleanup_ports:
>  	fwnode_handle_put(ports);
>  	fwnode_handle_put(portnp);
>  
> +cleanup_switch:
>  	lan966x_cleanup_ports(lan966x);
>  
>  	cancel_delayed_work_sync(&lan966x->stats_work);

[Severity: Medium]
This is a pre-existing issue, but since the ladder is gaining an MDB
deinit: does the probe error path leak the lan966x_mac_entry objects on
lan966x->mac_entries?

lan966x_remove() calls lan966x_mac_purge_entries() after
lan966x_cleanup_ports(). None of the probe error labels do.

The list is initialized in lan966x_init()->lan966x_mac_init() before the
port loop. After that, the ANA learn IRQ can allocate entries:

lan966x_ana_irq_handler()
  lan966x_mac_irq_handler()
    lan966x_mac_irq_process()
      list_add_tail(&mac_entry->list, &lan966x->mac_entries);

lan966x_mac_add_entry() can also add entries from FDB work once the
ports are registered.

If lan966x_fdb_init(), lan966x_ptp_init(), fdma_init or
lan966x_vcap_init() fails after that, these entries are never freed. The
devm-allocated struct lan966x that holds the list head is then released.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025203155.455075-1-mashikiselby%40gmail.com