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

On Mon, Sep 22, 2025 at 10:38:31AM +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

---

drivers/staging/gpib/fmh_gpib/fmh_gpib.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c index 4138f3d2bae7..245c8fe87eaa 100644 --- a/drivers/staging/gpib/fmh_gpib/fmh_gpib.c +++ b/drivers/staging/gpib/fmh_gpib/fmh_gpib.c @@ -1395,14 +1395,17 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar pdev = to_platform_device(board->dev); retval = fmh_gpib_generic_attach(board);

• if (retval)

• if (retval) {

• put_device(board->dev);
  

  return retval;

Do this with an unwind goto.

if (reval) goto put_dev;

...

retval = fmh_gpib_init(...); /* this bug wasn't fixed btw */ if (retval) goto put_dev;

return 0;

put_dev: put_device(board->dev); return retval;

Actually, this function needs a bunch of other frees as well. See my blog for more details:

https://staticthinking.wordpress.com/2022/04/28/free-the-last-thing-style/

• }

e_priv = board->private_data; nec_priv = &e_priv->nec7210_priv; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gpib_control_status"); if (!res) {

• put_device(board->dev);
  

  dev_err(board->dev, "Unable to locate mmio resource\n"); return -ENODEV; }

@@ -1410,6 +1413,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar if (request_mem_region(res->start, resource_size(res), pdev->name) == NULL) {

• put_device(board->dev);
  

  dev_err(board->dev, "cannot claim registers\n"); return -ENXIO; }

request_mem_region() needs a release_region().

@@ -1418,6 +1422,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar nec_priv->mmiobase = ioremap(e_priv->gpib_iomem_res->start, resource_size(e_priv->gpib_iomem_res)); if (!nec_priv->mmiobase) {

• put_device(board->dev);
  

  dev_err(board->dev, "Could not map I/O memory\n"); return -ENOMEM; }

ioremap() needs an iounmap();

@@ -1426,12 +1431,14 @@ 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) {

• put_device(board->dev);
  

  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) {

• put_device(board->dev);
  

  dev_err(board->dev, "cannot claim registers\n"); return -ENXIO; }

release_region()

@@ -1439,6 +1446,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar e_priv->fifo_base = ioremap(e_priv->dma_port_res->start, resource_size(e_priv->dma_port_res)); if (!e_priv->fifo_base) {

• put_device(board->dev);
  

  dev_err(board->dev, "Could not map I/O memory for fifos\n"); return -ENOMEM; }

iounmap();

@@ -1447,10 +1455,14 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar (unsigned long)resource_size(e_priv->dma_port_res)); irq = platform_get_irq(pdev, 0);

• if (irq < 0)

• if (irq < 0) {

• put_device(board->dev);
  

  return -EBUSY;
• }
• retval = request_irq(irq, fmh_gpib_interrupt, IRQF_SHARED, pdev->name, board); if (retval) {

• put_device(board->dev);
  

  dev_err(board->dev, "cannot register interrupt handler err=%d\n", retval);

request_irq() needs a release_irq()

@@ -1461,6 +1473,7 @@ static int fmh_gpib_attach_impl(struct gpib_board *board, const struct gpib_boar if (acquire_dma) { e_priv->dma_channel = dma_request_slave_channel(board->dev, "rxtx"); if (!e_priv->dma_channel) {

• 	put_device(board->dev);
  dev_err(board->dev, "failed to acquire dma channel \"rxtx\".\n");
  return -EIO;
  

  }

This needs a free too. There may be other things outside of what I can see in this email.

@@ -1517,6 +1530,12 @@ void fmh_gpib_detach(struct gpib_board *board) resource_size(e_priv->gpib_iomem_res)); } fmh_gpib_generic_detach(board);

• if (board->dev) {

• dev_set_drvdata(board->dev, NULL);
  

• put_device(board->dev);
  

• board->dev = NULL;
  

I explain this in my blog, that the free function should be a cut and paste of the cleanup. So this stuff isn't done in the cleanup so one or the other of these is not correct.

The question is should this be done in one patch or several patches? Adding cleanup to one function is generally considered One Thing in terms of One Thing Per Path. If we were going to backport bits of cleanup to different stable kernels then we would want to break it up into the easiest way for backporting. But realistically we're not going to do that here because this doesn't affect real life users generally. It's just from review. It's not a security patch. And this is staging as well so the standards for backports are not necessarily as strict. (Staging drivers are often really really bad).

regards, dan carpenter