Re: [PATCH v2] gpib: fmh_gpib: Fix resource leaks in fmh_gpib_attach_impl

[Dominik Karol Piątkowski]

Hi Hongling Zeng,

On Friday, May 29th, 2026 at 07:57, Hongling Zeng <zhongling0719@xxxxxxx> wrote:

>    Hi Dominik Karol,
> 
>    Thanks for the analysis. You're right about the double-free issue.
> 
>    However, there's still a real leak : when dma_fifos resource
>    lookup fails, the previously allocated gpib_iomem_res leaks because
>    dma_port_res isn't assigned yet, so detach() won't clean it up.
> 
>    Minimal fix: cleanup only before field assignment:
> ```c
>    if (!res) {
>        dev_err(board->dev, "Unable to locate mmio resource for gpib dma
> port\n");
>        release_mem_region(e_priv->gpib_iomem_res->start,
>                          resource_size(e_priv->gpib_iomem_res));

I assume you're talking about the following code:

```
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma_fifos");
if (!res) {
	dev_err(board->dev, "Unable to locate mmio resource for gpib dma port\n");
	return -ENODEV;
}
if (request_mem_region(res->start,
		       resource_size(res),
		       pdev->name) == NULL) {
	dev_err(board->dev, "cannot claim registers\n");
	return -ENXIO;
}
e_priv->dma_port_res = res;
```

Earlier in the fmh_gpib_attach_impl we have the following memory region request:

```
if (request_mem_region(res->start,
		       resource_size(res),
		       pdev->name) == NULL) {
	dev_err(board->dev, "cannot claim registers\n");
	return -ENXIO;
}
e_priv->gpib_iomem_res = res;
```

I see that requested memory region is indeed saved in e_priv->gpib_iomem_res,
and cleaned up in fmh_gpib_detach that is called on failed attach:

```
if (e_priv->gpib_iomem_res)
	release_mem_region(e_priv->gpib_iomem_res->start,
			   resource_size(e_priv->gpib_iomem_res));
```

I don't really see a leak there.

Thanks,
Dominik Karol

>        return -ENODEV;
>    }
> 
>    This fixes the leak while avoiding double-free. Acceptable approach?
> 
>    Best regards,
>    Hongling Zeng
> 
> 
> 在 2026年05月29日 00:46, Dominik Karol Piątkowski 写道:
> > Hi Hongling Zeng,
> >
> > On Thursday, May 28th, 2026 at 03:50, Hongling Zeng <zenghongling@xxxxxxxxxx> wrote:
> >
> >> The fmh_gpib_attach_impl() function has multiple resource leaks in its
> >> error handling paths. When any initialization step fails, the function
> >> returns early without properly releasing previously acquired resources.
> >>
> >> Fix by adding proper error handling labels and cleanup code that releases
> >> resources in the reverse order they were acquired.
> >>
> >> Fixes: 8e4841a0888c7 ("staging: gpib: Add Frank Mori Hess FPGA PCI GPIB driver")
> >> Signed-off-by: Hongling Zeng <zenghongling@xxxxxxxxxx>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Suggested-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@xxxxxxxxxxxxxx>
> >>
> >> ---
> >> Changes in v2:
> >>   - Fixed unnecessary retval assignments in early error returns
> >>   - Removed extra newline
> >>   - Kept e_priv->irq assignment after request_irq() succeeds,as suggested by Dominik Karol
> >> ---
> >>   drivers/gpib/fmh_gpib/fmh_gpib.c | 37 +++++++++++++++++++++++++-------
> >>   1 file changed, 29 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpib/fmh_gpib/fmh_gpib.c b/drivers/gpib/fmh_gpib/fmh_gpib.c
> >> index fcafdc02ea2e..404379cd1565 100644
> >> --- a/drivers/gpib/fmh_gpib/fmh_gpib.c
> >> +++ b/drivers/gpib/fmh_gpib/fmh_gpib.c
> >> @@ -1418,7 +1418,8 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> >>   				     resource_size(e_priv->gpib_iomem_res));
> >>   	if (!nec_priv->mmiobase) {
> >>   		dev_err(board->dev, "Could not map I/O memory\n");
> >> -		return -ENOMEM;
> >> +		retval = -ENOMEM;
> >> +		goto err_release_gpib_region;
> >>   	}
> >>   	dev_dbg(board->dev, "iobase %pr remapped to %p\n",
> >>   		e_priv->gpib_iomem_res, nec_priv->mmiobase);
> >> @@ -1426,34 +1427,39 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> >>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma_fifos");
> >>   	if (!res) {
> >>   		dev_err(board->dev, "Unable to locate mmio resource for gpib dma port\n");
> >> -		return -ENODEV;
> >> +		retval = -ENODEV;
> >> +		goto err_iounmap_gpib;
> >>   	}
> >>   	if (request_mem_region(res->start,
> >>   			       resource_size(res),
> >>   			       pdev->name) == NULL) {
> >>   		dev_err(board->dev, "cannot claim registers\n");
> >> -		return -ENXIO;
> >> +		retval = -ENXIO;
> >> +		goto err_iounmap_gpib;
> >>   	}
> >>   	e_priv->dma_port_res = res;
> >>   	e_priv->fifo_base = ioremap(e_priv->dma_port_res->start,
> >>   				    resource_size(e_priv->dma_port_res));
> >>   	if (!e_priv->fifo_base) {
> >>   		dev_err(board->dev, "Could not map I/O memory for fifos\n");
> >> -		return -ENOMEM;
> >> +		retval = -ENOMEM;
> >> +		goto err_release_dma_region;
> >>   	}
> >>   	dev_dbg(board->dev, "dma fifos 0x%lx remapped to %p, length=%ld\n",
> >>   		(unsigned long)e_priv->dma_port_res->start, e_priv->fifo_base,
> >>   		(unsigned long)resource_size(e_priv->dma_port_res));
> >>
> >>   	irq = platform_get_irq(pdev, 0);
> >> -	if (irq < 0)
> >> -		return -EBUSY;
> >> +	if (irq < 0) {
> >> +		retval = -EBUSY;
> >> +		goto err_iounmap_fifo;
> >> +	}
> >>   	retval = request_irq(irq, fmh_gpib_interrupt, IRQF_SHARED, pdev->name, board);
> >>   	if (retval) {
> >>   		dev_err(board->dev,
> >>   			"cannot register interrupt handler err=%d\n",
> >>   			retval);
> >> -		return retval;
> >> +		goto err_iounmap_fifo;
> >>   	}
> >>   	e_priv->irq = irq;
> >>
> >> @@ -1461,7 +1467,8 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> >>   		e_priv->dma_channel = dma_request_slave_channel(board->dev, "rxtx");
> >>   		if (!e_priv->dma_channel) {
> >>   			dev_err(board->dev, "failed to acquire dma channel \"rxtx\".\n");
> >> -			return -EIO;
> >> +			retval = -EIO;
> >> +			goto err_free_irq;
> >>   		}
> >>   	}
> >>   	/*
> >> @@ -1473,6 +1480,20 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
> >>   		fifo_max_burst_length_mask;
> >>
> >>   	return fmh_gpib_init(e_priv, board, handshake_mode);
> >> +
> >> +err_free_irq:
> >> +	free_irq(e_priv->irq, board);
> >> +err_iounmap_fifo:
> >> +	iounmap(e_priv->fifo_base);
> >> +err_release_dma_region:
> >> +	release_mem_region(e_priv->dma_port_res->start,
> >> +				resource_size(e_priv->dma_port_res));
> >> +err_iounmap_gpib:
> >> +	iounmap(nec_priv->mmiobase);
> >> +err_release_gpib_region:
> >> +	release_mem_region(e_priv->gpib_iomem_res->start,
> >> +				resource_size(e_priv->gpib_iomem_res));
> >> +	return retval;
> > I see a problem with this patch.
> >
> > fmh_gpib_attach_impl is called from fmh_gpib_attach_holdoff_(all|end), and these
> > are passed as attach through fmh_gpib_(unaccel_)interface. The only place where
> > I see attach is called, is in common/iblib.c, in ibonline function. If attach
> > fails (that is, returns with -errno), detach is called (fmh_gpib_detach for
> > this case), where a proper cleanup is performed.
> >
> > If we release the resources in attach and not clear corresponding e_priv fields,
> > we will fail badly in detach, as we do double free/unmap and use after free.
> > Clearing e_priv fields will result in having two competing cleanup routines,
> > and I don't like that idea - let's have one proper cleanup in one place. The
> > only way to keep this approach would be to rewrite the core gpib logic for
> > attach/detach, and I don't know if it's worth the effort - probably not.
> >
> > Thanks,
> > Dominik Karol
> >
> >>   }
> >>
> >>   int fmh_gpib_attach_holdoff_all(struct gpib_board *board, const struct gpib_board_config *config)
> >> --
> >> 2.25.1
> >>
> >>
> 
>