[PATCH 0/3] dmaengine: ioatdma: Fix mem leakage series

[PATCH 0/3] dmaengine: ioatdma: Fix mem leakage series

[Nikita Shubin via B4 Relay]

Started with observing leakage in patch 3, ivestigating revealed much
more problems in probing error path.

Andy you are always welcome to review if you have a spare time.

Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
Nikita Shubin (3):
      dmaengine: ioatdma: Fix leaking on version mismatch
      dmaengine: ioatdma: Fix error path in ioat3_dma_probe()
      dmaengine: ioatdma: Fix kmemleak in ioat_pci_probe()

 drivers/dma/ioat/init.c | 55 ++++++++++++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 26 deletions(-)
---
base-commit: 6d69b6c12fce479fde7bc06f686212451688a102
change-id: 20240524-ioatdma-fixes-a8fccda9bd79

Best regards,
-- 
Nikita Shubin <n.shubin@yadro.com>

[PATCH 1/3] dmaengine: ioatdma: Fix leaking on version mismatch

[Nikita Shubin via B4 Relay]

From: Nikita Shubin <n.shubin@yadro.com>

Fix leaking ioatdma_device if I/OAT version is less than IOAT_VER_3_0.

Fixes: bf453a0a18b2 ("dmaengine: ioat: Support in-use unbind")
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 drivers/dma/ioat/init.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 9c364e92cb82..e76e507ae898 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1350,6 +1350,7 @@ static int ioat_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	void __iomem * const *iomap;
 	struct device *dev = &pdev->dev;
 	struct ioatdma_device *device;
+	u8 version;
 	int err;
 
 	err = pcim_enable_device(pdev);
@@ -1363,6 +1364,10 @@ static int ioat_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!iomap)
 		return -ENOMEM;
 
+	version = readb(iomap[IOAT_MMIO_BAR] + IOAT_VER_OFFSET);
+	if (version < IOAT_VER_3_0)
+		return -ENODEV;
+
 	err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
 	if (err)
 		return err;
@@ -1373,16 +1378,14 @@ static int ioat_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	pci_set_master(pdev);
 	pci_set_drvdata(pdev, device);
 
-	device->version = readb(device->reg_base + IOAT_VER_OFFSET);
+	device->version = version;
 	if (device->version >= IOAT_VER_3_4)
 		ioat_dca_enabled = 0;
-	if (device->version >= IOAT_VER_3_0) {
-		if (is_skx_ioat(pdev))
-			device->version = IOAT_VER_3_2;
-		err = ioat3_dma_probe(device, ioat_dca_enabled);
-	} else
-		return -ENODEV;
 
+	if (is_skx_ioat(pdev))
+		device->version = IOAT_VER_3_2;
+
+	err = ioat3_dma_probe(device, ioat_dca_enabled);
 	if (err) {
 		dev_err(dev, "Intel(R) I/OAT DMA Engine init failed\n");
 		return -ENODEV;

-- 
2.43.2

[PATCH 2/3] dmaengine: ioatdma: Fix error path in ioat3_dma_probe()

[Nikita Shubin via B4 Relay]

From: Nikita Shubin <n.shubin@yadro.com>

Make sure we are disabling interrupts and destroying DMA pool if
pcie_capability_read/write_word() failes.

Fixes: 511deae0261c ("dmaengine: ioatdma: disable relaxed ordering for ioatdma")
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 drivers/dma/ioat/init.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index e76e507ae898..26964b7c8cf1 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -534,18 +534,6 @@ static int ioat_probe(struct ioatdma_device *ioat_dma)
 	return err;
 }
 
-static int ioat_register(struct ioatdma_device *ioat_dma)
-{
-	int err = dma_async_device_register(&ioat_dma->dma_dev);
-
-	if (err) {
-		ioat_disable_interrupts(ioat_dma);
-		dma_pool_destroy(ioat_dma->completion_pool);
-	}
-
-	return err;
-}
-
 static void ioat_dma_remove(struct ioatdma_device *ioat_dma)
 {
 	struct dma_device *dma = &ioat_dma->dma_dev;
@@ -1181,9 +1169,9 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
 		       ioat_chan->reg_base + IOAT_DCACTRL_OFFSET);
 	}
 
-	err = ioat_register(ioat_dma);
+	err = dma_async_device_register(&ioat_dma->dma_dev);
 	if (err)
-		return err;
+		goto err_disable_interrupts;
 
 	ioat_kobject_add(ioat_dma, &ioat_ktype);
 
@@ -1192,20 +1180,29 @@ static int ioat3_dma_probe(struct ioatdma_device *ioat_dma, int dca)
 
 	/* disable relaxed ordering */
 	err = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &val16);
-	if (err)
-		return pcibios_err_to_errno(err);
+	if (err) {
+		err = pcibios_err_to_errno(err);
+		goto err_disable_interrupts;
+	}
 
 	/* clear relaxed ordering enable */
 	val16 &= ~PCI_EXP_DEVCTL_RELAX_EN;
 	err = pcie_capability_write_word(pdev, PCI_EXP_DEVCTL, val16);
-	if (err)
-		return pcibios_err_to_errno(err);
+	if (err) {
+		err = pcibios_err_to_errno(err);
+		goto err_disable_interrupts;
+	}
 
 	if (ioat_dma->cap & IOAT_CAP_DPS)
 		writeb(ioat_pending_level + 1,
 		       ioat_dma->reg_base + IOAT_PREFETCH_LIMIT_OFFSET);
 
 	return 0;
+
+err_disable_interrupts:
+	ioat_disable_interrupts(ioat_dma);
+	dma_pool_destroy(ioat_dma->completion_pool);
+	return err;
 }
 
 static void ioat_shutdown(struct pci_dev *pdev)

-- 
2.43.2

[PATCH 3/3] dmaengine: ioatdma: Fix kmemleak in ioat_pci_probe()

[Nikita Shubin via B4 Relay]

From: Nikita Shubin <n.shubin@yadro.com>

If probing fails we end up with leaking ioatdma_device and each
allocated channel.

Following kmemleak is easy to be reproduced by injecting error in
ioat_alloc_chan_resources() when doing ioat_dma_self_test().

unreferenced object 0xffff888014ad5800 (size 1024):
  comm "modprobe", pid 73, jiffies 4294681749
  hex dump (first 32 bytes):
    00 10 00 13 80 88 ff ff 00 c0 3f 00 00 c9 ff ff  ..........?.....
    00 ce 76 13 80 88 ff ff 00 00 00 00 00 00 00 00  ..v.............
  backtrace (crc 1f353f55):
    [<ffffffff827692ca>] kmemleak_alloc+0x4a/0x80
    [<ffffffff81430600>] kmalloc_trace+0x270/0x2f0
    [<ffffffffa000b7d1>] ioat_pci_probe+0xc1/0x1c0 [ioatdma]
    [<ffffffff8199376a>] local_pci_probe+0x7a/0xe0
    [<ffffffff81995189>] pci_call_probe+0xd9/0x2c0
    [<ffffffff81995975>] pci_device_probe+0xa5/0x170
    [<ffffffff81f5f89b>] really_probe+0x14b/0x510
    [<ffffffff81f5fd4a>] __driver_probe_device+0xda/0x1f0
    [<ffffffff81f5febf>] driver_probe_device+0x4f/0x120
    [<ffffffff81f6028a>] __driver_attach+0x14a/0x2b0
    [<ffffffff81f5c56c>] bus_for_each_dev+0xec/0x160
    [<ffffffff81f5ee1b>] driver_attach+0x2b/0x40
    [<ffffffff81f5e0d3>] bus_add_driver+0x1a3/0x300
    [<ffffffff81f61db3>] driver_register+0xa3/0x1d0
    [<ffffffff8199325b>] __pci_register_driver+0xeb/0x100
    [<ffffffffa003009c>] 0xffffffffa003009c

repeated for each ioatdma channel:

unreferenced object 0xffff8880148e5c00 (size 512):
  comm "modprobe", pid 73, jiffies 4294681751
  hex dump (first 32 bytes):
    40 58 ad 14 80 88 ff ff 00 00 00 00 00 00 00 00  @X..............
    01 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace (crc fbc62789):
    [<ffffffff827692ca>] kmemleak_alloc+0x4a/0x80
    [<ffffffff81430600>] kmalloc_trace+0x270/0x2f0
    [<ffffffffa0009641>] ioat_enumerate_channels+0x101/0x2d0 [ioatdma]
    [<ffffffffa000b266>] ioat3_dma_probe+0x4d6/0x970 [ioatdma]
    [<ffffffffa000b891>] ioat_pci_probe+0x181/0x1c0 [ioatdma]
    [<ffffffff8199376a>] local_pci_probe+0x7a/0xe0
    [<ffffffff81995189>] pci_call_probe+0xd9/0x2c0
    [<ffffffff81995975>] pci_device_probe+0xa5/0x170
    [<ffffffff81f5f89b>] really_probe+0x14b/0x510
    [<ffffffff81f5fd4a>] __driver_probe_device+0xda/0x1f0
    [<ffffffff81f5febf>] driver_probe_device+0x4f/0x120
    [<ffffffff81f6028a>] __driver_attach+0x14a/0x2b0
    [<ffffffff81f5c56c>] bus_for_each_dev+0xec/0x160
    [<ffffffff81f5ee1b>] driver_attach+0x2b/0x40
    [<ffffffff81f5e0d3>] bus_add_driver+0x1a3/0x300
    [<ffffffff81f61db3>] driver_register+0xa3/0x1d0

Fixes: bf453a0a18b2 ("dmaengine: ioat: Support in-use unbind")
Signed-off-by: Nikita Shubin <n.shubin@yadro.com>
---
 drivers/dma/ioat/init.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index 26964b7c8cf1..d0d787cfd0e0 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -1348,7 +1348,7 @@ static int ioat_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct device *dev = &pdev->dev;
 	struct ioatdma_device *device;
 	u8 version;
-	int err;
+	int err, i;
 
 	err = pcim_enable_device(pdev);
 	if (err)
@@ -1384,6 +1384,9 @@ static int ioat_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	err = ioat3_dma_probe(device, ioat_dca_enabled);
 	if (err) {
+		for (i = 0; i < IOAT_MAX_CHANS; i++)
+			kfree(device->idx[i]);
+		kfree(device);
 		dev_err(dev, "Intel(R) I/OAT DMA Engine init failed\n");
 		return -ENODEV;
 	}

-- 
2.43.2

Re: [PATCH 2/3] dmaengine: ioatdma: Fix error path in ioat3_dma_probe()

[Markus Elfring]

…
> pcie_capability_read/write_word() failes.

                                    call failed?

Regards,
Markus

Re: [PATCH 2/3] dmaengine: ioatdma: Fix error path in ioat3_dma_probe()

[Nikita Shubin]

>> pcie_capability_read/write_word() failes.
>                                    call failed?

Indeed - thank you.

Re: [PATCH 3/3] dmaengine: ioatdma: Fix kmemleak in ioat_pci_probe()

[Andy Shevchenko]

On Fri, May 24, 2024 at 1:24 PM Nikita Shubin via B4 Relay
<devnull+n.shubin.yadro.com@kernel.org> wrote:
>
> From: Nikita Shubin <n.shubin@yadro.com>
>
> If probing fails we end up with leaking ioatdma_device and each
> allocated channel.
>
> Following kmemleak is easy to be reproduced by injecting error in

easy to reproduce

an error

> ioat_alloc_chan_resources() when doing ioat_dma_self_test().
>
> unreferenced object 0xffff888014ad5800 (size 1024):
>   comm "modprobe", pid 73, jiffies 4294681749
>   hex dump (first 32 bytes):
>     00 10 00 13 80 88 ff ff 00 c0 3f 00 00 c9 ff ff  ..........?.....
>     00 ce 76 13 80 88 ff ff 00 00 00 00 00 00 00 00  ..v.............
>   backtrace (crc 1f353f55):
>     [<ffffffff827692ca>] kmemleak_alloc+0x4a/0x80
>     [<ffffffff81430600>] kmalloc_trace+0x270/0x2f0
>     [<ffffffffa000b7d1>] ioat_pci_probe+0xc1/0x1c0 [ioatdma]
>     [<ffffffff8199376a>] local_pci_probe+0x7a/0xe0
>     [<ffffffff81995189>] pci_call_probe+0xd9/0x2c0
>     [<ffffffff81995975>] pci_device_probe+0xa5/0x170
>     [<ffffffff81f5f89b>] really_probe+0x14b/0x510
>     [<ffffffff81f5fd4a>] __driver_probe_device+0xda/0x1f0
>     [<ffffffff81f5febf>] driver_probe_device+0x4f/0x120
>     [<ffffffff81f6028a>] __driver_attach+0x14a/0x2b0
>     [<ffffffff81f5c56c>] bus_for_each_dev+0xec/0x160
>     [<ffffffff81f5ee1b>] driver_attach+0x2b/0x40
>     [<ffffffff81f5e0d3>] bus_add_driver+0x1a3/0x300
>     [<ffffffff81f61db3>] driver_register+0xa3/0x1d0
>     [<ffffffff8199325b>] __pci_register_driver+0xeb/0x100
>     [<ffffffffa003009c>] 0xffffffffa003009c
>
> repeated for each ioatdma channel:
>
> unreferenced object 0xffff8880148e5c00 (size 512):
>   comm "modprobe", pid 73, jiffies 4294681751
>   hex dump (first 32 bytes):
>     40 58 ad 14 80 88 ff ff 00 00 00 00 00 00 00 00  @X..............
>     01 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace (crc fbc62789):
>     [<ffffffff827692ca>] kmemleak_alloc+0x4a/0x80
>     [<ffffffff81430600>] kmalloc_trace+0x270/0x2f0
>     [<ffffffffa0009641>] ioat_enumerate_channels+0x101/0x2d0 [ioatdma]
>     [<ffffffffa000b266>] ioat3_dma_probe+0x4d6/0x970 [ioatdma]
>     [<ffffffffa000b891>] ioat_pci_probe+0x181/0x1c0 [ioatdma]
>     [<ffffffff8199376a>] local_pci_probe+0x7a/0xe0
>     [<ffffffff81995189>] pci_call_probe+0xd9/0x2c0
>     [<ffffffff81995975>] pci_device_probe+0xa5/0x170
>     [<ffffffff81f5f89b>] really_probe+0x14b/0x510
>     [<ffffffff81f5fd4a>] __driver_probe_device+0xda/0x1f0
>     [<ffffffff81f5febf>] driver_probe_device+0x4f/0x120
>     [<ffffffff81f6028a>] __driver_attach+0x14a/0x2b0
>     [<ffffffff81f5c56c>] bus_for_each_dev+0xec/0x160
>     [<ffffffff81f5ee1b>] driver_attach+0x2b/0x40
>     [<ffffffff81f5e0d3>] bus_add_driver+0x1a3/0x300
>     [<ffffffff81f61db3>] driver_register+0xa3/0x1d0

Please, read
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages
and follow the advice given there.

...

> +       int err, i;

Why signed?

-- 
With Best Regards,
Andy Shevchenko