Re: [PATCH net v2 2/2] net: fman: add error cleanup path in fman_probe

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: fman: add error cleanup path in fman_probe

This commit introduces a new error cleanup path in fman_probe() to prevent
memory leaks when fman_init() or devm_request_irq() fail. It adds
fman_muram_finish() and fman_free_resources() to properly tear down
allocations and states.

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 6947f3bc7c87e..752c0df0e17c5 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c

[ ... ]

> +static void fman_free_resources(struct fman *fman, struct device *dev,
> +				bool irq_registered)
> +{
> +	/* Free IRQs first while fman is still valid */
> +	if (irq_registered) {
> +		if (fman->dts_params.err_irq != 0)
> +			devm_free_irq(dev, fman->dts_params.err_irq, fman);
> +		devm_free_irq(dev, fman->dts_params.irq, fman);
> +	}
> +
> +	kfree(fman->keygen);
> +	free_init_resources(fman);

[Severity: High]
If fman_init() fails internally (for example, in dma_init() or
keygen_init()), it calls free_init_resources(fman) on its own failure path.
However, it does not clear fman->fifo_offset or fman->cam_offset.

When fman_probe() catches this error, it jumps to err_no_irq and calls
fman_free_resources(), which will invoke free_init_resources(fman) a second
time.

Does this cause a double-free of the MURAM resources?

[Severity: High]
This is a pre-existing issue, but if fman_muram_alloc() fails during
fman_init(), it returns an error code (such as -ENOMEM) which is stored
directly in fman->fifo_offset.

Since the error code is non-zero, free_init_resources() will evaluate
if (fman->fifo_offset) as true and pass the error pointer to
fman_muram_free_mem().

Does attempting to free this pseudo-random error code cause corruption in
the gen_pool allocator?

> +	kfree(fman->cfg);
> +	fman_muram_finish(fman->muram);
> +	kfree(fman->state);
> +	kfree(fman);
> +}

[ ... ]

> @@ -2844,7 +2863,7 @@ static int fman_probe(struct platform_device *of_dev)
>  	if (err < 0) {
>  		dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
>  			__func__, fman->dts_params.irq, err);
> -		return err;
> +		goto err_no_irq;
>  	}
>  
>  	if (fman->dts_params.err_irq != 0) {

[Severity: High]
If devm_request_irq() fails here, fman_init() has already succeeded. At the
end of fman_init(), the FMan hardware is fully enabled via enable(fman, cfg).

Jumping to err_no_irq calls fman_free_resources(), which destroys the
structures and unmaps the MURAM memory. However, there doesn't appear to be
any call to stop or disable the active hardware.

Will the hardware continue to operate and perform DMA to the freed MURAM
addresses, potentially causing memory corruption?