Re: [PATCH] ptp: ocp: Fix dpll device reference leak in ptp_ocp_probe()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (1) · 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] The sibling error path `out_dpll:` in ptp_ocp_probe()
  (drivers/ptp/ptp_ocp.c:5150-5155) drops only the driver's own DPLL…

Pre-existing issues:
- [Medium] ptp_ocp_probe() (drivers/ptp/ptp_ocp.c) registers the devlink
  instance at line 5115 (`devlink_register(devlink)`) before the DPLL…

--

ptp: ocp: Fix dpll device reference leak in ptp_ocp_probe()

This adds a dpll_device_put() call on the error path taken when
dpll_device_register() fails in ptp_ocp_probe(), so the reference taken
earlier by dpll_device_get() is dropped before the shared unwind path
runs.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 3d26ec1f7b9ec..0739adb69a130 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
> @@ -5123,8 +5123,10 @@ ptp_ocp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  	}
>  
>  	err = dpll_device_register(bp->dpll, DPLL_TYPE_PPS, &dpll_ops, bp);
> -	if (err)
> +	if (err) {
> +		dpll_device_put(bp->dpll, &bp->tracker);
>  		goto out;
> +	}
>  
>  	for (i = 0; i < OCP_SMA_NUM; i++) {
>  		bp->sma[i].dpll_pin = dpll_pin_get(clkid, i, THIS_MODULE,

[Severity: High]
Once dpll_device_register() succeeds, does the sibling out_dpll path still
leak the same dpll device this patch is trying to protect?  The loop right
below the changed hunk jumps there on failure:

	for (i = 0; i < OCP_SMA_NUM; i++) {
		bp->sma[i].dpll_pin = dpll_pin_get(...);
		if (IS_ERR(bp->sma[i].dpll_pin)) {
			err = PTR_ERR(bp->sma[i].dpll_pin);
			goto out_dpll;
		}

		err = dpll_pin_register(bp->dpll, bp->sma[i].dpll_pin, ...);
		if (err) {
			dpll_pin_put(bp->sma[i].dpll_pin, &bp->sma[i].tracker);
			goto out_dpll;
		}
	}

and out_dpll only drops the driver's own reference:

out_dpll:
	while (i--) {
		dpll_pin_unregister(bp->dpll, bp->sma[i].dpll_pin, &dpll_pins_ops, &bp->sma[i]);
		dpll_pin_put(bp->sma[i].dpll_pin, &bp->sma[i].tracker);
	}
	dpll_device_put(bp->dpll, &bp->tracker);
out:
	ptp_ocp_detach(bp);
out_disable:
	pci_disable_device(pdev);
out_free:
	devlink_free(devlink);

dpll_device_register() in drivers/dpll/dpll_core.c allocates a registration
that stores priv and takes a second reference of its own:

	reg->ops = ops;
	reg->priv = priv;
	dpll->type = type;
	__dpll_device_hold(dpll, &reg->tracker);
	...
	xa_set_mark(&dpll_device_xa, dpll->id, DPLL_REGISTERED);

Only dpll_device_unregister() removes that entry and puts that reference,
which is what ptp_ocp_remove() does:

	dpll_device_unregister(bp->dpll, &dpll_ops, bp);
	dpll_device_put(bp->dpll, &bp->tracker);

Since ptp_ocp_remove() is not called after a failed probe, does the dpll
device stay marked DPLL_REGISTERED with reg->priv pointing at bp, while
devlink_free() releases the allocation whose private area is bp
(bp = devlink_priv(devlink))?  If so, can a later DPLL_CMD_DEVICE_GET reach
ptp_ocp_dpll_lock_status_get() with the freed bp:

	struct ptp_ocp *bp = priv;

	*status = bp->sync ? DPLL_LOCK_STATUS_LOCKED : DPLL_LOCK_STATUS_UNLOCKED;

and would the still-registered device keep ops pointers into the module
text after the module is unloaded?

Triggering conditions are dpll_pin_get() returning ERR_PTR on allocation
failure, or dpll_pin_register() failing for any SMA index.  Would it make
sense for this patch to also add dpll_device_unregister(bp->dpll,
&dpll_ops, bp) to out_dpll, so the reference the newly handled
dpll_device_register() call takes is released on all error paths?

[Severity: Medium]
This isn't a bug introduced by this patch, but the unwind path the patch
extends also looks asymmetric with respect to devlink.  ptp_ocp_probe()
registers the devlink instance before the dpll setup block:

	ptp_ocp_info(bp);
	devlink_register(devlink);

	clkid = pci_get_dsn(pdev);
	bp->dpll = dpll_device_get(clkid, 0, THIS_MODULE, &bp->tracker);

but the out/out_dpll paths reach devlink_free() without any
devlink_unregister():

out:
	ptp_ocp_detach(bp);
out_disable:
	pci_disable_device(pdev);
out_free:
	devlink_free(devlink);
	return err;

ptp_ocp_detach() contains no devlink call, and only ptp_ocp_remove() does:

	devlink_unregister(devlink);
	ptp_ocp_detach(bp);
	pci_disable_device(pdev);

	devlink_free(devlink);

devlink_free() in net/devlink/core.c opens with:

void devlink_free(struct devlink *devlink)
{
	ASSERT_DEVLINK_NOT_REGISTERED(devlink);

Does a dpll failure in probe therefore hit that WARN_ON_ONCE and destroy
the instance while it is still marked DEVLINK_REGISTERED, so userspace
listeners see the NEW notification but never the matching DEL, and the
instance stays findable while ptp_ocp_detach() tears the device down?  The
triggering conditions are the same ones handled here: dpll_device_get()
returning ERR_PTR, dpll_device_register() returning an error, or a
dpll_pin_get()/dpll_pin_register() failure.

Would adding devlink_unregister(devlink) to the out path be appropriate,
either in this patch or as a separate one?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917142337.2156652-1-vulab%40iscas.ac.cn