[BUG][PATCH]dma-coherent.c: error path bug

[BUG][PATCH]dma-coherent.c: error path bug

[Marin Mitov]

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 <mitov@issp.bas.bg>

=======================================================================
--- a/drivers/base/dma-coherent.c	2010-06-06 12:47:17.000000000 +0300
+++ b/drivers/base/dma-coherent.c	2010-06-06 12:53:36.000000000 +0300
@@ -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);

Re: [BUG][PATCH]dma-coherent.c: error path bug

[FUJITA Tomonori]

On Sun, 6 Jun 2010 13:53:04 +0300
Marin Mitov <mitov@issp.bas.bg> 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.
> 
> Marin Mitov
> 
> Signed-off-by: Marin Mitov <mitov@issp.bas.bg>

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.

Re: [BUG][PATCH]dma-coherent.c: error path bug

[Marin Mitov]

On Monday, June 07, 2010 05:30:48 am FUJITA Tomonori wrote:
> On Sun, 6 Jun 2010 13:53:04 +0300
> Marin Mitov <mitov@issp.bas.bg> 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.
> > 
> > Marin Mitov
> > 
> > Signed-off-by: Marin Mitov <mitov@issp.bas.bg>
> 
> 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

Re: [BUG][PATCH]dma-coherent.c: error path bug

[FUJITA Tomonori]

On Mon, 7 Jun 2010 07:08:56 +0300
Marin Mitov <mitov@issp.bas.bg> wrote:

> On Monday, June 07, 2010 05:30:48 am FUJITA Tomonori wrote:
> > On Sun, 6 Jun 2010 13:53:04 +0300
> > Marin Mitov <mitov@issp.bas.bg> 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.
> > > 
> > > Marin Mitov
> > > 
> > > Signed-off-by: Marin Mitov <mitov@issp.bas.bg>
> > 
> > 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.

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

Re: [BUG][PATCH]dma-coherent.c: error path bug

[Marin Mitov]

On Monday, June 07, 2010 07:27:49 am FUJITA Tomonori wrote:
> On Mon, 7 Jun 2010 07:08:56 +0300
> Marin Mitov <mitov@issp.bas.bg> wrote:
> 
> > On Monday, June 07, 2010 05:30:48 am FUJITA Tomonori wrote:
> > > On Sun, 6 Jun 2010 13:53:04 +0300
> > > Marin Mitov <mitov@issp.bas.bg> 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.
> > > > 
> > > > Marin Mitov
> > > > 
> > > > Signed-off-by: Marin Mitov <mitov@issp.bas.bg>
> > > 
> > > 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.
> 
> 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

Re: [BUG][PATCH]dma-coherent.c: error path bug

[FUJITA Tomonori]

On Mon, 7 Jun 2010 07:43:19 +0300
Marin Mitov <mitov@issp.bas.bg> wrote:

> On Monday, June 07, 2010 07:27:49 am FUJITA Tomonori wrote:
> > On Mon, 7 Jun 2010 07:08:56 +0300
> > Marin Mitov <mitov@issp.bas.bg> wrote:
> > 
> > > On Monday, June 07, 2010 05:30:48 am FUJITA Tomonori wrote:
> > > > On Sun, 6 Jun 2010 13:53:04 +0300
> > > > Marin Mitov <mitov@issp.bas.bg> 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.
> > > > > 
> > > > > Marin Mitov
> > > > > 
> > > > > Signed-off-by: Marin Mitov <mitov@issp.bas.bg>
> > > > 
> > > > 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.
> > 
> > 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,

Re: [BUG][PATCH]dma-coherent.c: error path bug

[Paul Mundt]

On Mon, Jun 07, 2010 at 01:59:48PM +0900, FUJITA Tomonori wrote:
> On Mon, 7 Jun 2010 07:43:19 +0300
> Marin Mitov <mitov@issp.bas.bg> wrote:
> > On Monday, June 07, 2010 07:27:49 am FUJITA Tomonori wrote:
> > > On Mon, 7 Jun 2010 07:08:56 +0300
> > > Marin Mitov <mitov@issp.bas.bg> wrote:
> > > > On Monday, June 07, 2010 05:30:48 am FUJITA Tomonori wrote:
> > > > > 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.
> > > 
> > > 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.