[PATCH] serial: pch: Fix PCI device refcount leak in pch_request_dma()

[PATCH] serial: pch: Fix PCI device refcount leak in pch_request_dma()

[Xiongfeng Wang]

As comment of pci_get_slot() says, it returns a pci_device with its
refcount increased. The caller must decrement the reference count by
calling pci_dev_put().

Since 'dma_dev' is only used to filter the channel in filter(), we can
call pci_dev_put() before exiting from pch_request_dma(). Add the
missing pci_dev_put() for the normal and error path.

Fixes: 3c6a483275f4 ("Serial: EG20T: add PCH_UART driver")
Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 drivers/tty/serial/pch_uart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index c59ce7886579..b17788cf309b 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -694,6 +694,7 @@ static void pch_request_dma(struct uart_port *port)
 	if (!chan) {
 		dev_err(priv->port.dev, "%s:dma_request_channel FAILS(Tx)\n",
 			__func__);
+		pci_dev_put(dma_dev);
 		return;
 	}
 	priv->chan_tx = chan;
@@ -710,6 +711,7 @@ static void pch_request_dma(struct uart_port *port)
 			__func__);
 		dma_release_channel(priv->chan_tx);
 		priv->chan_tx = NULL;
+		pci_dev_put(dma_dev);
 		return;
 	}
 
@@ -717,6 +719,8 @@ static void pch_request_dma(struct uart_port *port)
 	priv->rx_buf_virt = dma_alloc_coherent(port->dev, port->fifosize,
 				    &priv->rx_buf_dma, GFP_KERNEL);
 	priv->chan_rx = chan;
+
+	pci_dev_put(dma_dev);
 }
 
 static void pch_dma_rx_complete(void *arg)
-- 
2.20.1

Re: [PATCH] serial: pch: Fix PCI device refcount leak in pch_request_dma()

[Andy Shevchenko]

On Tue, Nov 22, 2022 at 07:45:59PM +0800, Xiongfeng Wang wrote:
> As comment of pci_get_slot() says, it returns a pci_device with its
> refcount increased. The caller must decrement the reference count by
> calling pci_dev_put().

> Since 'dma_dev' is only used to filter the channel in filter(), we can
> call pci_dev_put() before exiting from pch_request_dma(). Add the
> missing pci_dev_put() for the normal and error path.

No, we can't do that. How is it supposed to work if DMA device disappears in
the middle?

Look at how it's done in the

5318f70da7e8 serial: 8250_lpss: Balance reference count for PCI DMA device
67ec6dd0b257 serial: 8250_mid: Balance reference count for PCI DMA device

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] serial: pch: Fix PCI device refcount leak in pch_request_dma()

[Xiongfeng Wang]

Hi Andy,

Thanks for your reply !

On 2022/11/23 3:03, Andy Shevchenko wrote:
> On Tue, Nov 22, 2022 at 07:45:59PM +0800, Xiongfeng Wang wrote:
>> As comment of pci_get_slot() says, it returns a pci_device with its
>> refcount increased. The caller must decrement the reference count by
>> calling pci_dev_put().
> 
>> Since 'dma_dev' is only used to filter the channel in filter(), we can
>> call pci_dev_put() before exiting from pch_request_dma(). Add the
>> missing pci_dev_put() for the normal and error path.
> 
> No, we can't do that. How is it supposed to work if DMA device disappears in
> the middle?

When the DMA device is registered into the system, its refcount is increased.
dma_request_channel() will increase the refcount for 'dma_chan'. So I think this
can gurantee the DMA device not removed in the middle.

Thanks,
Xiongfeng

> 
> Look at how it's done in the
> 
> 5318f70da7e8 serial: 8250_lpss: Balance reference count for PCI DMA device
> 67ec6dd0b257 serial: 8250_mid: Balance reference count for PCI DMA device
> 

Re: [PATCH] serial: pch: Fix PCI device refcount leak in pch_request_dma()

[Andy Shevchenko]

On Wed, Nov 23, 2022 at 10:41:08AM +0800, Xiongfeng Wang wrote:
> On 2022/11/23 3:03, Andy Shevchenko wrote:
> > On Tue, Nov 22, 2022 at 07:45:59PM +0800, Xiongfeng Wang wrote:
> >> As comment of pci_get_slot() says, it returns a pci_device with its
> >> refcount increased. The caller must decrement the reference count by
> >> calling pci_dev_put().
> > 
> >> Since 'dma_dev' is only used to filter the channel in filter(), we can
> >> call pci_dev_put() before exiting from pch_request_dma(). Add the
> >> missing pci_dev_put() for the normal and error path.
> > 
> > No, we can't do that. How is it supposed to work if DMA device disappears in
> > the middle?
> 
> When the DMA device is registered into the system, its refcount is increased.
> dma_request_channel() will increase the refcount for 'dma_chan'. So I think this
> can gurantee the DMA device not removed in the middle.

It might be the case in this particular driver but not in general, i.e. when
driver drops and request channels on demand, like with SPI case. By doing this
way you prevent (possible) modifications to serial core to use DMA on demand.

That's why I recommend to switch to more robust way of solving this.

-- 
With Best Regards,
Andy Shevchenko