[PATCH v2] staging: gpib: Fix device reference leak in fmh_gpib driver

[PATCH v2] staging: gpib: Fix device reference leak in fmh_gpib driver

[Ma Ke]

The fmh_gpib driver contains a device reference count leak in
fmh_gpib_attach_impl() where driver_find_device() increases the
reference count of the device by get_device() when matching but this
reference is not properly decreased. Add put_device() in
fmh_gpib_attach_impl() and add put_device() in fmh_gpib_detach(),
which ensures that the reference count of the device is correctly
managed.

Found by code review.

Cc: stable@vger.kernel.org
Fixes: 8e4841a0888c ("staging: gpib: Add Frank Mori Hess FPGA PCI GPIB driver")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
Changes in v2:
- modified the free operations as suggestions. Thanks for dan carpenter's instructions.
---
 drivers/staging/gpib/fmh_gpib/fmh_gpib.c | 66 +++++++++++++++++++-----
 1 file changed, 54 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c
index 4138f3d2bae7..f5be24b44238 100644
--- a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c
+++ b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c
@@ -1396,7 +1396,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
 
 	retval = fmh_gpib_generic_attach(board);
 	if (retval)
-		return retval;
+		goto err_put_device;
 
 	e_priv = board->private_data;
 	nec_priv = &e_priv->nec7210_priv;
@@ -1404,14 +1404,16 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpib_control_status");
 	if (!res) {
 		dev_err(board->dev, "Unable to locate mmio resource\n");
-		return -ENODEV;
+		retval = -ENODEV;
+		goto err_generic_detach;
 	}
 
 	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_generic_detach;
 	}
 	e_priv->gpib_iomem_res = res;
 
@@ -1419,7 +1421,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_mmio_region;
 	}
 	dev_dbg(board->dev, "iobase %pr remapped to %p\n",
 		e_priv->gpib_iomem_res, nec_priv->mmiobase);
@@ -1427,34 +1430,41 @@ 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_mmio;
 	}
 	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_mmio;
 	}
 	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;
 
@@ -1462,9 +1472,11 @@ 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;
 		}
 	}
+
 	/*
 	 * in the future we might want to know the half-fifo size
 	 * (dma_burst_length) even when not using dma, so go ahead an
@@ -1473,7 +1485,32 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar
 	e_priv->dma_burst_length = fifos_read(e_priv, FIFO_MAX_BURST_LENGTH_REG) &
 		fifo_max_burst_length_mask;
 
-	return fmh_gpib_init(e_priv, board, handshake_mode);
+	retval = fmh_gpib_init(e_priv, board, handshake_mode);
+	if (retval)
+		goto err_release_dma;
+
+	return 0;
+
+err_release_dma:
+	if (acquire_dma && e_priv->dma_channel)
+		dma_release_channel(e_priv->dma_channel);
+err_free_irq:
+	free_irq(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_mmio:
+	iounmap(nec_priv->mmiobase);
+err_release_mmio_region:
+	release_mem_region(e_priv->gpib_iomem_res->start,
+			   resource_size(e_priv->gpib_iomem_res));
+err_generic_detach:
+	fmh_gpib_generic_detach(board);
+err_put_device:
+	put_device(board->dev);
+	return retval;
 }
 
 int fmh_gpib_attach_holdoff_all(struct gpib_board *board, const struct gpib_board_config *config)
@@ -1517,6 +1554,11 @@ void fmh_gpib_detach(struct gpib_board *board)
 					   resource_size(e_priv->gpib_iomem_res));
 	}
 	fmh_gpib_generic_detach(board);
+
+	if (board->dev) {
+		put_device(board->dev);
+		board->dev = NULL;
+	}
 }
 
 static int fmh_gpib_pci_attach_impl(struct gpib_board *board,
-- 
2.17.1

Re: [PATCH v2] staging: gpib: Fix device reference leak in fmh_gpib driver

[Dan Carpenter]

On Mon, Sep 22, 2025 at 04:45:12PM +0800, Ma Ke wrote:
> The fmh_gpib driver contains a device reference count leak in
> fmh_gpib_attach_impl() where driver_find_device() increases the
> reference count of the device by get_device() when matching but this
> reference is not properly decreased. Add put_device() in
> fmh_gpib_attach_impl() and add put_device() in fmh_gpib_detach(),
> which ensures that the reference count of the device is correctly
> managed.
> 
> Found by code review.
> 
> Cc: stable@vger.kernel.org
> Fixes: 8e4841a0888c ("staging: gpib: Add Frank Mori Hess FPGA PCI GPIB driver")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> ---
> Changes in v2:
> - modified the free operations as suggestions. Thanks for dan carpenter's instructions.
> ---

Actually, it turns out that this isn't the right approach.  Sorry.
This will introduce double frees.

The caller looks like this:

drivers/staging/gpib/common/iblib.c
   204  int ibonline(struct gpib_board *board)
   205  {
   206          int retval;
   207  
   208          if (board->online)
   209                  return -EBUSY;
   210          if (!board->interface)
   211                  return -ENODEV;
   212          retval = gpib_allocate_board(board);
   213          if (retval < 0)
   214                  return retval;
   215  
   216          board->dev = NULL;
   217          board->local_ppoll_mode = 0;
   218          retval = board->interface->attach(board, &board->config);
   219          if (retval < 0) {
   220                  board->interface->detach(board);

   So if the attach() fails, we call ->detach() which works.

   221                  return retval;
   222          }

It's weird because the fmh_gpib_pci_detach() function does have a
put_device() in it:

	if (board->dev)
		pci_dev_put(to_pci_dev(board->dev));
                ^^^^^^^^^^^

The detach functions are really similar...

regards,
dan carpenter

Re: [PATCH v2] staging: gpib: Fix device reference leak in fmh_gpib driver

[Ma Ke]

On 9/22/25 19:28, Dan Carpenter wrote:
> On Mon, Sep 22, 2025 at 04:45:12PM +0800, Ma Ke wrote:
> > The fmh_gpib driver contains a device reference count leak in
> > fmh_gpib_attach_impl() where driver_find_device() increases the
> > reference count of the device by get_device() when matching but this
> > reference is not properly decreased. Add put_device() in
> > fmh_gpib_attach_impl() and add put_device() in fmh_gpib_detach(),
> > which ensures that the reference count of the device is correctly
> > managed.
> > 
> > Found by code review.
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 8e4841a0888c ("staging: gpib: Add Frank Mori Hess FPGA PCI GPIB driver")
> > Signed-off-by: Ma Ke <make24@iscas.ac.cn>
> > ---
> > Changes in v2:
> > - modified the free operations as suggestions. Thanks for dan carpenter's instructions.
> > ---
> 
> Actually, it turns out that this isn't the right approach.  Sorry.
> This will introduce double frees.
> 
> The caller looks like this:
> 
> drivers/staging/gpib/common/iblib.c
>    204  int ibonline(struct gpib_board *board)
>    205  {
>    206          int retval;
>    207  
>    208          if (board->online)
>    209                  return -EBUSY;
>    210          if (!board->interface)
>    211                  return -ENODEV;
>    212          retval = gpib_allocate_board(board);
>    213          if (retval < 0)
>    214                  return retval;
>    215  
>    216          board->dev = NULL;
>    217          board->local_ppoll_mode = 0;
>    218          retval = board->interface->attach(board, &board->config);
>    219          if (retval < 0) {
>    220                  board->interface->detach(board);
> 
>    So if the attach() fails, we call ->detach() which works.
> 
>    221                  return retval;
>    222          }
> 
> It's weird because the fmh_gpib_pci_detach() function does have a
> put_device() in it:
> 
> 	if (board->dev)
> 		pci_dev_put(to_pci_dev(board->dev));
>                 ^^^^^^^^^^^
> 
> The detach functions are really similar...
> 
> regards,
> dan carpenter
Thank you very much for your patient reply. Indeed, I also carefully 
examined the implementations of the functions fmh_gpib_detach() and 
fmh_gpib_pci_detach(), and their code is indeed very similar. I also 
took note of the double free issue you mentioned. In patch v3, I will 
refer to the implementation of fmh_gpib_pci_detach() and only add 
put_device() in fmh_gpib_detach(). I will not make any modifications 
to other functions. Once again, thank you for your patient guidance, 
from which I have benefited greatly.

Best regards,

Ma Ke