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

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

[Hongling Zeng]

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@kylinos.cn>
Cc: stable@vger.kernel.org
Suggested-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@protonmail.com>

---
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;
 }
 
 int fmh_gpib_attach_holdoff_all(struct gpib_board *board, const struct gpib_board_config *config)
-- 
2.25.1

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

[Dominik Karol Piątkowski]

Hi Hongling Zeng,

On Thursday, May 28th, 2026 at 03:50, Hongling Zeng <zenghongling@kylinos.cn> 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@kylinos.cn>
> Cc: stable@vger.kernel.org
> Suggested-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@protonmail.com>
> 
> ---
> 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
> 
>

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

[Hongling Zeng]

   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));
       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@kylinos.cn> 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@kylinos.cn>
>> Cc: stable@vger.kernel.org
>> Suggested-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@protonmail.com>
>>
>> ---
>> 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
>>
>>

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@126.com> 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@kylinos.cn> 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@kylinos.cn>
> >> Cc: stable@vger.kernel.org
> >> Suggested-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@protonmail.com>
> >>
> >> ---
> >> 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
> >>
> >>
> 
>