2012-11-05 13:40:26

by Matthieu CASTET

Subject: [PATCH] dmapool : make DMAPOOL_DEBUG detect corruption of free marker

This can help to catch case where hardware is writting after dma free.

Signed-off-by: Matthieu Castet <[email protected]>
---
mm/dmapool.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index c5ab33b..e10898a 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -346,6 +346,29 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
retval = offset + page->vaddr;
*handle = offset + page->dma;
#ifdef DMAPOOL_DEBUG
+ {
+ int i;
+ u8 *data = retval;
+ /* page->offset is stored in first 4 bytes */
+ for (i = sizeof(int); i < pool->size; i++) {
+ if (data[i] != POOL_POISON_FREED) {
+ if (pool->dev)
+ dev_err(pool->dev,
+ "dma_pool_alloc %s, %p (corruped)\n",
+ pool->name, retval);
+ else
+ printk(KERN_ERR
+ "dma_pool_alloc %s, %p (corruped)\n",
+ pool->name, retval);
+
+ /* we dump the first 4 bytes even if there are not
+ POOL_POISON_FREED */
+ print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
+ data, pool->size, 1);
+ break;
+ }
+ }
+ }
memset(retval, POOL_POISON_ALLOCATED, pool->size);
#endif
done:
--
1.7.10.4

2012-11-05 20:05:09

by Andrew Morton

Subject: Re: [PATCH] dmapool : make DMAPOOL_DEBUG detect corruption of free marker

On Mon, 5 Nov 2012 14:40:05 +0100
Matthieu CASTET <[email protected]> wrote:

> This can help to catch case where hardware is writting after dma free.
>
> ...
>
> --- a/mm/dmapool.c
> +++ b/mm/dmapool.c
> @@ -346,6 +346,29 @@ void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
> retval = offset + page->vaddr;
> *handle = offset + page->dma;
> #ifdef DMAPOOL_DEBUG
> + {
> + int i;
> + u8 *data = retval;
> + /* page->offset is stored in first 4 bytes */
> + for (i = sizeof(int); i < pool->size; i++) {
> + if (data[i] != POOL_POISON_FREED) {
> + if (pool->dev)
> + dev_err(pool->dev,
> + "dma_pool_alloc %s, %p (corruped)\n",
> + pool->name, retval);
> + else
> + printk(KERN_ERR
> + "dma_pool_alloc %s, %p (corruped)\n",
> + pool->name, retval);
> +
> + /* we dump the first 4 bytes even if there are not
> + POOL_POISON_FREED */
> + print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
> + data, pool->size, 1);
> + break;
> + }
> + }
> + }
> memset(retval, POOL_POISON_ALLOCATED, pool->size);
> #endif

Seems reasonable. I fidded wth a few things.

- The code is quite painful to look at in an 80-col display, and
there's no real reason it has to be that way.

- The comment layout is strange, it misses capitalisation and needed
a grammatical fix

- Use sizeof(page->offset), not sizeof(int). Just in case someone
changes the type of dma_page.offset.

- Use pr_err(), as suggested by checkpatch. Please use checkpatch.

--- a/mm/dmapool.c~dmapool-make-dmapool_debug-detect-corruption-of-free-marker-fix
+++ a/mm/dmapool.c
@@ -350,23 +350,24 @@ void *dma_pool_alloc(struct dma_pool *po
int i;
u8 *data = retval;
/* page->offset is stored in first 4 bytes */
- for (i = sizeof(int); i < pool->size; i++) {
- if (data[i] != POOL_POISON_FREED) {
- if (pool->dev)
- dev_err(pool->dev,
- "dma_pool_alloc %s, %p (corruped)\n",
- pool->name, retval);
- else
- printk(KERN_ERR
- "dma_pool_alloc %s, %p (corruped)\n",
- pool->name, retval);
-
- /* we dump the first 4 bytes even if there are not
- POOL_POISON_FREED */
- print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
- data, pool->size, 1);
- break;
- }
+ for (i = sizeof(page->offset); i < pool->size; i++) {
+ if (data[i] == POOL_POISON_FREED)
+ continue;
+ if (pool->dev)
+ dev_err(pool->dev,
+ "dma_pool_alloc %s, %p (corruped)\n",
+ pool->name, retval);
+ else
+ pr_err("dma_pool_alloc %s, %p (corruped)\n",
+ pool->name, retval);
+
+ /*
+ * Dump the first 4 bytes even if they are not
+ * POOL_POISON_FREED
+ */
+ print_hex_dump(KERN_ERR, "", DUMP_PREFIX_OFFSET, 16, 1,
+ data, pool->size, 1);
+ break;
}
}
memset(retval, POOL_POISON_ALLOCATED, pool->size);
_