dma-coherent.c: error path bug

PATCH In review Marin Mitov 3 participants 7 messages Jun 6, 2010 – Jun 7, 2010 Driver core

lore

Share & tools

lore

Marin Mitov

author

2010-06-06 10:55 UTC #0 lore raw

PATCH 1/1 dma-coherent.c: error path bug

Hi all,

The error path in dma_declare_coherent_memory() leaves the pointer dev->dma_mem non completely initialized.

If allocation of dev->dma_mem succeeds,
but allocation of dev->dma_mem->bitmap fails
dev->dma_mem is freed, but left non NULL
and non completely initialized.

Either zero it after being freed (one liner patch), or assign to dev->dma_mem only completely initialized structure (patch included).

Comments welcome.

Marin Mitov

Signed-off-by Marin Mitov <[email protected]>

=======================================================================

drivers/base/dma-coherent.c +11 -9

@@ -17,6 +17,7 @@ struct dma_coherent_mem {
 int dma_declare_coherent_memory(struct device *dev, dma_addr_t bus_addr,
 				dma_addr_t device_addr, size_t size, int flags)
 {
+	struct dma_coherent_mem *mem;
 	void __iomem *mem_base = NULL;
 	int pages = size >> PAGE_SHIFT;
 	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);

@@ -34,17 +35,18 @@ int dma_declare_coherent_memory(struct d
 	if (!mem_base)
 		goto out;
 
-	dev->dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL);
-	if (!dev->dma_mem)
+	mem = kzalloc(sizeof(*mem), GFP_KERNEL);
+	if (!mem)
 		goto out;
-	dev->dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
-	if (!dev->dma_mem->bitmap)
+	mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL);
+	if (!mem->bitmap)
 		goto free1_out;
 
-	dev->dma_mem->virt_base = mem_base;
-	dev->dma_mem->device_base = device_addr;
-	dev->dma_mem->size = pages;
-	dev->dma_mem->flags = flags;
+	mem->virt_base = mem_base;
+	mem->device_base = device_addr;
+	mem->size = pages;
+	mem->flags = flags;
+	dev->dma_mem = mem;
 
 	if (flags & DMA_MEMORY_MAP)
 		return DMA_MEMORY_MAP;

@@ -52,7 +54,7 @@ int dma_declare_coherent_memory(struct d
 	return DMA_MEMORY_IO;
 
  free1_out:
-	kfree(dev->dma_mem);
+	kfree(mem);
  out:
 	if (mem_base)
 		iounmap(mem_base);

FUJITA Tomonori

2010-06-07 02:31 UTC #1 lore raw

Quoted 10 lines · On Sun, 6 Jun 2010 13:53:04 +0300 Marin Mitov <[email protected]> wrote:

Hi all,
The error path in dma_declare_coherent_memory() leaves the pointer dev->dma_mem non completely initialized.
If allocation of dev->dma_mem succeeds,
but allocation of dev->dma_mem->bitmap fails
dev->dma_mem is freed, but left non NULL
and non completely initialized.

Quoted 8 lines

Either zero it after being freed (one liner patch), or assign to dev->dma_mem only completely initialized structure (patch included).
Comments welcome.
Marin Mitov
Signed-off-by Marin Mitov <[email protected]>

Hmm, if dma_declare_coherent_memory() fails, the driver doesn't use dev->dma_mem. So even if dev->dma_mem points to a freed memory, I'm not sure that it causes a real problem. We could call this patch a cleanup though.

Marin Mitov

author

2010-06-07 04:11 UTC #2 lore raw

Quoted 18 lines · FUJITA Tomonori

Quoted 15 lines · On Sun, 6 Jun 2010 13:53:04 +0300 Marin Mitov <[email protected]> wrote:
Hi all,
The error path in dma_declare_coherent_memory() leaves the pointer dev->dma_mem non completely initialized.
If allocation of dev->dma_mem succeeds,
but allocation of dev->dma_mem->bitmap fails
dev->dma_mem is freed, but left non NULL
and non completely initialized.
Either zero it after being freed (one liner patch), or assign to dev->dma_mem only completely initialized structure (patch included).
Comments welcome.

Quoted 9 lines

Quoted 3 lines
Marin Mitov
Signed-off-by Marin Mitov <[email protected]>
Hmm, if dma_declare_coherent_memory() fails, the driver doesn't use dev->dma_mem. So even if dev->dma_mem points to a freed memory, I'm not sure that it causes a real problem. We could call this patch a cleanup though.

My understanding of dma_alloc_coherent() is that we first try to allocate from per-device coherent memory and we do it using dma_alloc_from_coherent() (in drivers/base/dma-coherent.c) if dev->dma_mem is not NULL (and we have left it not NULL, here is the problem). If allocation of dev->dma_mem->bitmap fails dev->dma_mem must be NULL.

Marin Mitov

FUJITA Tomonori

2010-06-07 04:28 UTC #3 lore raw

Quoted 28 lines · On Mon, 7 Jun 2010 07:08:56 +0300 Marin Mitov <[email protected]> wrote:

Quoted 18 lines · FUJITA Tomonori
Quoted 15 lines · On Sun, 6 Jun 2010 13:53:04 +0300 Marin Mitov <[email protected]> wrote:
Hi all,
The error path in dma_declare_coherent_memory() leaves the pointer dev->dma_mem non completely initialized.
If allocation of dev->dma_mem succeeds,
but allocation of dev->dma_mem->bitmap fails
dev->dma_mem is freed, but left non NULL
and non completely initialized.
Either zero it after being freed (one liner patch), or assign to dev->dma_mem only completely initialized structure (patch included).
Comments welcome.
Quoted 9 lines
Quoted 3 lines
Marin Mitov
Signed-off-by Marin Mitov <[email protected]>
Hmm, if dma_declare_coherent_memory() fails, the driver doesn't use dev->dma_mem. So even if dev->dma_mem points to a freed memory, I'm not sure that it causes a real problem. We could call this patch a cleanup though.

Quoted 5 lines

My understanding of dma_alloc_coherent() is that we first try to allocate from per-device coherent memory and we do it using dma_alloc_from_coherent() (in drivers/base/dma-coherent.c) if dev->dma_mem is not NULL (and we have left it not NULL, here is the problem). If allocation of dev->dma_mem->bitmap fails dev->dma_mem must be NULL.

But are there any driver that can continue when dma_declare_coherent_memory() fails?

Marin Mitov

author

2010-06-07 04:45 UTC #4 lore raw

Quoted 31 lines · FUJITA Tomonori

Quoted 28 lines · On Mon, 7 Jun 2010 07:08:56 +0300 Marin Mitov <[email protected]> wrote:
Quoted 27 lines · FUJITA Tomonori
Quoted 10 lines · On Sun, 6 Jun 2010 13:53:04 +0300 Marin Mitov <[email protected]> wrote:
Hi all,
The error path in dma_declare_coherent_memory() leaves the pointer dev->dma_mem non completely initialized.
If allocation of dev->dma_mem succeeds,
but allocation of dev->dma_mem->bitmap fails
dev->dma_mem is freed, but left non NULL
and non completely initialized.
Quoted 8 lines
Either zero it after being freed (one liner patch), or assign to dev->dma_mem only completely initialized structure (patch included).
Comments welcome.
Marin Mitov
Signed-off-by Marin Mitov <[email protected]>
Hmm, if dma_declare_coherent_memory() fails, the driver doesn't use dev->dma_mem. So even if dev->dma_mem points to a freed memory, I'm not sure that it causes a real problem. We could call this patch a cleanup though.

Quoted 8 lines

Quoted 5 lines
My understanding of dma_alloc_coherent() is that we first try to allocate from per-device coherent memory and we do it using dma_alloc_from_coherent() (in drivers/base/dma-coherent.c) if dev->dma_mem is not NULL (and we have left it not NULL, here is the problem). If allocation of dev->dma_mem->bitmap fails dev->dma_mem must be NULL.
But are there any driver that can continue when dma_declare_coherent_memory() fails?

I do not know if such a real driver exists.

My understanding of drivers' use of dma_declare_coherent_memory() is to declare some (may be on board) memory as coherent, so when they call dma_alloc_coherent() the request is satisfied first from this area and when it is exhausted fall back to other pools of coherent memory. As far as a fall back exists the driver may continue even if dma_declare_coherent_memory() fails.

Marin Mitov

FUJITA Tomonori

2010-06-07 05:00 UTC #5 lore raw

Quoted 38 lines · On Mon, 7 Jun 2010 07:43:19 +0300 Marin Mitov <[email protected]> wrote:

Quoted 37 lines · FUJITA Tomonori
Quoted 33 lines · On Mon, 7 Jun 2010 07:08:56 +0300 Marin Mitov <[email protected]> wrote:
Quoted 18 lines · FUJITA Tomonori
Quoted 15 lines · On Sun, 6 Jun 2010 13:53:04 +0300 Marin Mitov <[email protected]> wrote:
Hi all,
The error path in dma_declare_coherent_memory() leaves the pointer dev->dma_mem non completely initialized.
If allocation of dev->dma_mem succeeds,
but allocation of dev->dma_mem->bitmap fails
dev->dma_mem is freed, but left non NULL
and non completely initialized.
Either zero it after being freed (one liner patch), or assign to dev->dma_mem only completely initialized structure (patch included).
Comments welcome.
Quoted 9 lines
Quoted 3 lines
Marin Mitov
Signed-off-by Marin Mitov <[email protected]>
Hmm, if dma_declare_coherent_memory() fails, the driver doesn't use dev->dma_mem. So even if dev->dma_mem points to a freed memory, I'm not sure that it causes a real problem. We could call this patch a cleanup though.
My understanding of dma_alloc_coherent() is that we first try to allocate from per-device coherent memory and we do it using dma_alloc_from_coherent() (in drivers/base/dma-coherent.c) if dev->dma_mem is not NULL (and we have left it not NULL, here is the problem). If allocation of dev->dma_mem->bitmap fails dev->dma_mem must be NULL.

Quoted 5 lines

Quoted 2 lines
But are there any driver that can continue when dma_declare_coherent_memory() fails?
I do not know if such a real driver exists.
From a quick look, seems no. There is no fallback for such hardware,

it's safe assumption, I guess.

As I wrote, I don't think that the patch is rc material since seems that this doesn't cause a real problem. However, it looks fine for the next merge window.

Thanks,

Paul Mundt

2010-06-07 06:28 UTC #6 lore raw

Quoted 17 lines · FUJITA Tomonori

Quoted 15 lines · On Mon, 7 Jun 2010 07:43:19 +0300 Marin Mitov <[email protected]> wrote:
Quoted 14 lines · FUJITA Tomonori
Quoted 11 lines · On Mon, 7 Jun 2010 07:08:56 +0300 Marin Mitov <[email protected]> wrote:
Quoted 5 lines · FUJITA Tomonori
Hmm, if dma_declare_coherent_memory() fails, the driver doesn't use dev->dma_mem. So even if dev->dma_mem points to a freed memory, I'm not sure that it causes a real problem. We could call this patch a cleanup though.
My understanding of dma_alloc_coherent() is that we first try to allocate from per-device coherent memory and we do it using dma_alloc_from_coherent() (in drivers/base/dma-coherent.c) if dev->dma_mem is not NULL (and we have left it not NULL, here is the problem). If allocation of dev->dma_mem->bitmap fails dev->dma_mem must be NULL.

Quoted 7 lines

Quoted 3 lines
Quoted 2 lines
But are there any driver that can continue when dma_declare_coherent_memory() fails?
I do not know if such a real driver exists.
From a quick look, seems no. There is no fallback for such hardware, it's safe assumption, I guess.

Most of the devices using dma_declare_coherent_memory() today do so out of necessity, usually due to addressing limitations and needing to have DMA buffers allocated from device memory. This doesn't apply to all devices though, and indeed didn't apply at all when the interface was used initially (presently this functionality is pushed down to the DMA flags, anything that needs to be fixed to the device-specific coherent region uses DMA_MEMORY_EXCLUSIVE).

drivers/scsi/NCR_Q720.c (which was the only in-tree user for the declare coherent API before we started using it on SH) is an example of one that supports fallback to non-exclusive memory, and we've largely managed to avoid usage clashes with the exception of the following:

	0609697eab9775564845d4c94f9e3780fb791ffd
	cdf57cab27aef72f13a19c86858c6cac9951dc24
	58c6d3dfe436eb8cfb451981d8fdc9044eaf42da

I suspect the generic memory fallback path gets a lot less testing than the exclusive memory one at least.