Re: [PATCH net v2 2/2] net: fman: add error cleanup path in fman_probe
From: Simon Horman <horms@kernel.org>
Date: 2026-07-09 14:58:23
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
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); +}
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?