Re: [PATCH net 02/10] net: pse-pd: fix module refcount leak in pse_control_get_internal()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message names two leaking error paths in
  pse_control_get_internal() (drivers/net/pse-pd/pse_core.c): a…

--

net: pse-pd: fix module refcount leak in pse_control_get_internal()

This patch changes two error paths in pse_control_get_internal() to jump
to put_module instead of free_psec. Without it, the module reference
taken by try_module_get() is not dropped. One path is a controller
without pi_get_admin_state(). The other is a failing
pse_pi_is_hw_enabled().

> pse_control_get_internal() takes a module reference and then has two
> error paths that skip module_put(): a controller without
> pi_get_admin_state(), and a failing pse_pi_is_hw_enabled(). The second
> one happens on any I2C or UART error. Each time, one more reference
> leaks, and the PSE driver can no longer be unloaded.

[Severity: Low]
The code change looks correct. Is the first path in this description
actually reachable, though?

pse_controller_register() rejects a controller that has no
pi_get_admin_state(), and it does so before the controller is added to
pse_controller_list:

drivers/net/pse-pd/pse_core.c:pse_controller_register() {
    ...
	if (!pcdev->ops->pi_get_admin_state ||
	    !pcdev->ops->pi_get_pw_status) {
		dev_err(pcdev->dev,
			"Mandatory status report callbacks are missing");
		return -EINVAL;
	}
    ...
	mutex_lock(&pse_list_mutex);
	list_add(&pcdev->list, &pse_controller_list);
	mutex_unlock(&pse_list_mutex);
    ...
}

The only caller of pse_control_get_internal() is of_pse_control_get().
It takes pcdev from that list:

	list_for_each_entry(r, &pse_controller_list, list) {

So every pcdev that reaches pse_control_get_internal() has
pi_get_admin_state set. That means this check cannot fire:

	if (!pcdev->ops->pi_get_admin_state) {
		ret = -EOPNOTSUPP;
		goto put_module;
	}

Changing its goto keeps the unwind consistent, but it does not fix a
leak that can be hit. The leak that can be hit is the
pse_pi_is_hw_enabled() failure path. It returns the driver's
pi_get_admin_state() error, for example an I2C or UART failure.

Could the commit message say that the first branch is a defensive check
and cannot be reached today? Then anyone judging the fix for stable
would see that only the pse_pi_is_hw_enabled() path really leaks.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018082314.48950-1-github%40szelinsky.de