Re: [PATCH net-next 5/8] ice: add TX clock (TXC) DPLL interface for E825 devices

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ice: add TX clock (TXC) DPLL interface for E825 devices

This commit introduces a TX clock DPLL instance and pins for E825-based
devices. The TXC domain exposes two reference pins: EXT_EREF0 (board
external electrical reference) and SYNCE (port-derived reference,
described via fwnode). A new pin type and pin ops are added to support
TX clock references.

> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
> index 62f75701d652..a1258f2e03a9 100644
> --- a/drivers/net/ethernet/intel/ice/ice_dpll.c
> +++ b/drivers/net/ethernet/intel/ice/ice_dpll.c

[ ... ]

> @@ -3552,6 +3686,39 @@ ice_dpll_init_fwnode_pins(struct ice_pf *pf, struct ice_dpll_pin *pins,
>  	return ret;
>  }
>
> +static int ice_dpll_init_txclk_pins(struct ice_pf *pf, int start_idx)
> +{
> +	struct ice_dpll_pin *ref_pin = pf->dplls.txclks;
> +	struct ice_dpll *txc = &pf->dplls.txc;
> +	int ret = 0;
> +
> +	/* configure EXT_EREF0 pin */
> +	ret = ice_dpll_get_pins(pf, ref_pin, start_idx, 1, pf->dplls.clock_id);
> +	if (ret)
> +		return ret;
> +	ret = dpll_pin_register(txc->dpll, ref_pin->pin, &ice_dpll_txclk_ops,
> +				ref_pin);
> +	if (ret)
> +		return ret;
> +
> +	/* configure EXT_SYNCE pin which is based on fwnode pin */
> +	ref_pin++;
> +	ret = ice_dpll_init_fwnode_pin(ref_pin, ice_dpll_fwnode_ext_synce);
> +	if (ret)
> +		return ret;
> +
> +	ret = dpll_pin_register(txc->dpll, ref_pin->pin, &ice_dpll_txclk_ops,
> +				ref_pin);
> +	if (ret)
> +		return ret;

Does ice_dpll_init_txclk_pins leak the EXT_EREF0 pin on error?

If dpll_pin_register for the EXT_SYNCE pin fails, the function returns
the error without cleaning up the EXT_EREF0 pin that was already
successfully registered. Looking at the call chain:

ice_dpll_init_pins_e825() calls:
    ice_dpll_init_txclk_pins()
        ice_dpll_get_pins() -> acquires pin reference for EXT_EREF0
        dpll_pin_register() -> registers EXT_EREF0 with TXC DPLL
        ice_dpll_init_fwnode_pin() -> may fail here
        dpll_pin_register() -> or may fail here for EXT_SYNCE

If either of the last two steps fail, the function returns the error
directly without calling dpll_pin_unregister or ice_dpll_release_pins
for the EXT_EREF0 pin.

The caller ice_dpll_init_pins_e825() also doesn't compensate - on
txclk init failure it only calls ice_dpll_deinit_rclk_pin() and
ice_dpll_deinit_fwnode_pins(), neither of which clean up txclk
resources.

Compare with ice_dpll_init_fwnode_pins() and ice_dpll_get_pins() which
both have proper error-path rollback using goto labels.

> +
> +	if (IS_ERR_OR_NULL(ref_pin->pin))
> +		dev_dbg(ice_pf_to_dev(pf),
> +			"Tx-clk mux pin not registered yet\n");

Can this cause a NULL pointer dereference?

The IS_ERR_OR_NULL check appears after calling dpll_pin_register with
ref_pin->pin. Looking at the code flow:

ice_dpll_init_fwnode_pin() returns 0 (success) even when
fwnode_dpll_pin_find() returns NULL and sets ref_pin->pin = NULL. This
is the expected case - the commit message states "The SYNCE pin is
registered when its fwnode becomes available" and the code prints "DPLL
pin for %pfwp not registered yet".

However, the code unconditionally calls:

    dpll_pin_register(txc->dpll, ref_pin->pin, ...)

where ref_pin->pin is NULL. Looking at dpll_pin_register in
dpll_core.c:

    int dpll_pin_register(struct dpll_device *dpll, struct dpll_pin *pin, ...)
    {
        ...
        ret = dpll_pin_fwnode_register(dpll, pin, &ice_dpll_txclk_ops, pin_priv);
        ...
    }

which then calls code that dereferences pin->fwnode without a NULL
check, causing a kernel oops.

The IS_ERR_OR_NULL guard at line 3716 was clearly intended to protect
the dpll_pin_register call but was placed after it instead of before it.

> +
> +	return 0;
> +}

[ ... ]