drivers/staging/gpib/fmh_gpib/fmh_gpib.c | 66 +++++++++++++++++++----- 1 file changed, 54 insertions(+), 12 deletions(-)
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.1On 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
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
© 2016 - 2025 Red Hat, Inc.