[patch 1/4] fs, jfs: remove slab object constructor

[patch 1/4] fs, jfs: remove slab object constructor

[David Rientjes]

Mempools based on slab caches with object constructors are risky because
element allocation can happen either from the slab cache itself, meaning
the constructor is properly called before returning, or from the mempool
reserve pool, meaning the constructor is not called before returning,
depending on the allocation context.

For this reason, we should disallow creating mempools based on slab
caches that have object constructors.  Callers of mempool_alloc() will
be responsible for properly initializing the returned element.

Then, it doesn't matter if the element came from the slab cache or the
mempool reserved pool.

The only occurrence of a mempool being based on a slab cache with an
object constructor in the tree is in fs/jfs/jfs_metapage.c.  Remove it
and properly initialize the element in alloc_metapage().

At the same time, META_free is never used, so remove it as well.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 fs/jfs/jfs_metapage.c | 31 ++++++++++++-------------------
 fs/jfs/jfs_metapage.h |  1 -
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
--- a/fs/jfs/jfs_metapage.c
+++ b/fs/jfs/jfs_metapage.c
@@ -183,30 +183,23 @@ static inline void remove_metapage(struct page *page, struct metapage *mp)
 
 #endif
 
-static void init_once(void *foo)
-{
-	struct metapage *mp = (struct metapage *)foo;
-
-	mp->lid = 0;
-	mp->lsn = 0;
-	mp->flag = 0;
-	mp->data = NULL;
-	mp->clsn = 0;
-	mp->log = NULL;
-	set_bit(META_free, &mp->flag);
-	init_waitqueue_head(&mp->wait);
-}
-
 static inline struct metapage *alloc_metapage(gfp_t gfp_mask)
 {
-	return mempool_alloc(metapage_mempool, gfp_mask);
+	struct metapage *mp = mempool_alloc(metapage_mempool, gfp_mask);
+
+	if (mp) {
+		mp->lid = 0;
+		mp->lsn = 0;
+		mp->data = NULL;
+		mp->clsn = 0;
+		mp->log = NULL;
+		init_waitqueue_head(&mp->wait);
+	}
+	return mp;
 }
 
 static inline void free_metapage(struct metapage *mp)
 {
-	mp->flag = 0;
-	set_bit(META_free, &mp->flag);
-
 	mempool_free(mp, metapage_mempool);
 }
 
@@ -216,7 +209,7 @@ int __init metapage_init(void)
 	 * Allocate the metapage structures
 	 */
 	metapage_cache = kmem_cache_create("jfs_mp", sizeof(struct metapage),
-					   0, 0, init_once);
+					   0, 0, NULL);
 	if (metapage_cache == NULL)
 		return -ENOMEM;
 
diff --git a/fs/jfs/jfs_metapage.h b/fs/jfs/jfs_metapage.h
--- a/fs/jfs/jfs_metapage.h
+++ b/fs/jfs/jfs_metapage.h
@@ -48,7 +48,6 @@ struct metapage {
 
 /* metapage flag */
 #define META_locked	0
-#define META_free	1
 #define META_dirty	2
 #define META_sync	3
 #define META_discard	4

[patch 2/4] mm, mempool: disallow mempools based on slab caches with constructors

[David Rientjes]

All occurrences of mempools based on slab caches with object constructors
have been removed from the tree, so disallow creating them.

We can only dereference mem->ctor in mm/mempool.c without including
mm/slab.h in include/linux/mempool.h.  So simply note the restriction,
just like the comment restrictig usage of __GFP_ZERO, and warn on
kernels with CONFIG_DEBUG_VM() if such a mempool is allocated from.

We don't want to incur this check on every element allocation, so use
VM_BUG_ON().

Signed-off-by: David Rientjes <rientjes@google.com>
---
 include/linux/mempool.h | 3 ++-
 mm/mempool.c            | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/include/linux/mempool.h b/include/linux/mempool.h
--- a/include/linux/mempool.h
+++ b/include/linux/mempool.h
@@ -36,7 +36,8 @@ extern void mempool_free(void *element, mempool_t *pool);
 
 /*
  * A mempool_alloc_t and mempool_free_t that get the memory from
- * a slab that is passed in through pool_data.
+ * a slab cache that is passed in through pool_data.
+ * Note: the slab cache may not have a ctor function.
  */
 void *mempool_alloc_slab(gfp_t gfp_mask, void *pool_data);
 void mempool_free_slab(void *element, void *pool_data);
diff --git a/mm/mempool.c b/mm/mempool.c
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -15,6 +15,7 @@
 #include <linux/mempool.h>
 #include <linux/blkdev.h>
 #include <linux/writeback.h>
+#include "slab.h"
 
 static void add_element(mempool_t *pool, void *element)
 {
@@ -332,6 +333,7 @@ EXPORT_SYMBOL(mempool_free);
 void *mempool_alloc_slab(gfp_t gfp_mask, void *pool_data)
 {
 	struct kmem_cache *mem = pool_data;
+	VM_BUG_ON(mem->ctor);
 	return kmem_cache_alloc(mem, gfp_mask);
 }
 EXPORT_SYMBOL(mempool_alloc_slab);

[patch v2 3/4] mm, mempool: poison elements backed by slab allocator

[David Rientjes]

Mempools keep elements in a reserved pool for contexts in which
allocation may not be possible.  When an element is allocated from the
reserved pool, its memory contents is the same as when it was added to
the reserved pool.

Because of this, elements lack any free poisoning to detect
use-after-free errors.

This patch adds free poisoning for elements backed by the slab allocator.
This is possible because the mempool layer knows the object size of each
element.

When an element is added to the reserved pool, it is poisoned with
POISON_FREE.  When it is removed from the reserved pool, the contents are
checked for POISON_FREE.  If there is a mismatch, a warning is emitted to
the kernel log.

This is only effective for configs with CONFIG_DEBUG_SLAB or
CONFIG_SLUB_DEBUG_ON.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 v2: switch dependency to CONFIG_DEBUG_SLAB or CONFIG_SLUB_DEBUG_ON

 mm/mempool.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/mm/mempool.c b/mm/mempool.c
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -17,16 +17,77 @@
 #include <linux/writeback.h>
 #include "slab.h"
 
+#if defined(CONFIG_DEBUG_SLAB) || defined(CONFIG_SLUB_DEBUG_ON)
+static void poison_error(mempool_t *pool, void *element, size_t size,
+			 size_t byte)
+{
+	const int nr = pool->curr_nr;
+	const int start = max_t(int, byte - (BITS_PER_LONG / 8), 0);
+	const int end = min_t(int, byte + (BITS_PER_LONG / 8), size);
+	int i;
+
+	pr_err("BUG: mempool element poison mismatch\n");
+	pr_err("Mempool %p size %ld\n", pool, size);
+	pr_err(" nr=%d @ %p: %s0x", nr, element, start > 0 ? "... " : "");
+	for (i = start; i < end; i++)
+		pr_cont("%x ", *(u8 *)(element + i));
+	pr_cont("%s\n", end < size ? "..." : "");
+	dump_stack();
+}
+
+static void check_slab_element(mempool_t *pool, void *element)
+{
+	if (pool->free == mempool_free_slab || pool->free == mempool_kfree) {
+		size_t size = ksize(element);
+		u8 *obj = element;
+		size_t i;
+
+		for (i = 0; i < size; i++) {
+			u8 exp = (i < size - 1) ? POISON_FREE : POISON_END;
+
+			if (obj[i] != exp) {
+				poison_error(pool, element, size, i);
+				return;
+			}
+		}
+		memset(obj, POISON_INUSE, size);
+	}
+}
+
+static void poison_slab_element(mempool_t *pool, void *element)
+{
+	if (pool->alloc == mempool_alloc_slab ||
+	    pool->alloc == mempool_kmalloc) {
+		size_t size = ksize(element);
+		u8 *obj = element;
+
+		memset(obj, POISON_FREE, size - 1);
+		obj[size - 1] = POISON_END;
+	}
+}
+#else /* CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON */
+static inline void check_slab_element(mempool_t *pool, void *element)
+{
+}
+static inline void poison_slab_element(mempool_t *pool, void *element)
+{
+}
+#endif /* CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON */
+
 static void add_element(mempool_t *pool, void *element)
 {
 	BUG_ON(pool->curr_nr >= pool->min_nr);
+	poison_slab_element(pool, element);
 	pool->elements[pool->curr_nr++] = element;
 }
 
 static void *remove_element(mempool_t *pool)
 {
-	BUG_ON(pool->curr_nr <= 0);
-	return pool->elements[--pool->curr_nr];
+	void *element = pool->elements[--pool->curr_nr];
+
+	BUG_ON(pool->curr_nr < 0);
+	check_slab_element(pool, element);
+	return element;
 }
 
 /**

[patch v2 4/4] mm, mempool: poison elements backed by page allocator

[David Rientjes]

Elements backed by the slab allocator are poisoned when added to a
mempool's reserved pool.

It is also possible to poison elements backed by the page allocator
because the mempool layer knows the allocation order.

This patch extends mempool element poisoning to include memory backed by
the page allocator.

This is only effective for configs with CONFIG_DEBUG_SLAB or
CONFIG_SLUB_DEBUG_ON.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 v2: modify commit message for new dependencies

 mm/mempool.c | 74 ++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/mm/mempool.c b/mm/mempool.c
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -6,6 +6,7 @@
  *  extreme VM load.
  *
  *  started by Ingo Molnar, Copyright (C) 2001
+ *  debugging by David Rientjes, Copyright (C) 2015
  */
 
 #include <linux/mm.h>
@@ -35,41 +36,64 @@ static void poison_error(mempool_t *pool, void *element, size_t size,
 	dump_stack();
 }
 
-static void check_slab_element(mempool_t *pool, void *element)
+static void __check_element(mempool_t *pool, void *element, size_t size)
 {
-	if (pool->free == mempool_free_slab || pool->free == mempool_kfree) {
-		size_t size = ksize(element);
-		u8 *obj = element;
-		size_t i;
-
-		for (i = 0; i < size; i++) {
-			u8 exp = (i < size - 1) ? POISON_FREE : POISON_END;
-
-			if (obj[i] != exp) {
-				poison_error(pool, element, size, i);
-				return;
-			}
+	u8 *obj = element;
+	size_t i;
+
+	for (i = 0; i < size; i++) {
+		u8 exp = (i < size - 1) ? POISON_FREE : POISON_END;
+
+		if (obj[i] != exp) {
+			poison_error(pool, element, size, i);
+			return;
 		}
-		memset(obj, POISON_INUSE, size);
+	}
+	memset(obj, POISON_INUSE, size);
+}
+
+static void check_element(mempool_t *pool, void *element)
+{
+	/* Mempools backed by slab allocator */
+	if (pool->free == mempool_free_slab || pool->free == mempool_kfree)
+		__check_element(pool, element, ksize(element));
+
+	/* Mempools backed by page allocator */
+	if (pool->free == mempool_free_pages) {
+		int order = (int)(long)pool->pool_data;
+		void *addr = page_address(element);
+
+		__check_element(pool, addr, 1UL << (PAGE_SHIFT + order));
 	}
 }
 
-static void poison_slab_element(mempool_t *pool, void *element)
+static void __poison_element(void *element, size_t size)
 {
-	if (pool->alloc == mempool_alloc_slab ||
-	    pool->alloc == mempool_kmalloc) {
-		size_t size = ksize(element);
-		u8 *obj = element;
+	u8 *obj = element;
+
+	memset(obj, POISON_FREE, size - 1);
+	obj[size - 1] = POISON_END;
+}
+
+static void poison_element(mempool_t *pool, void *element)
+{
+	/* Mempools backed by slab allocator */
+	if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc)
+		__poison_element(element, ksize(element));
+
+	/* Mempools backed by page allocator */
+	if (pool->alloc == mempool_alloc_pages) {
+		int order = (int)(long)pool->pool_data;
+		void *addr = page_address(element);
 
-		memset(obj, POISON_FREE, size - 1);
-		obj[size - 1] = POISON_END;
+		__poison_element(addr, 1UL << (PAGE_SHIFT + order));
 	}
 }
 #else /* CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON */
-static inline void check_slab_element(mempool_t *pool, void *element)
+static inline void check_element(mempool_t *pool, void *element)
 {
 }
-static inline void poison_slab_element(mempool_t *pool, void *element)
+static inline void poison_element(mempool_t *pool, void *element)
 {
 }
 #endif /* CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON */
@@ -77,7 +101,7 @@ static inline void poison_slab_element(mempool_t *pool, void *element)
 static void add_element(mempool_t *pool, void *element)
 {
 	BUG_ON(pool->curr_nr >= pool->min_nr);
-	poison_slab_element(pool, element);
+	poison_element(pool, element);
 	pool->elements[pool->curr_nr++] = element;
 }
 
@@ -86,7 +110,7 @@ static void *remove_element(mempool_t *pool)
 	void *element = pool->elements[--pool->curr_nr];
 
 	BUG_ON(pool->curr_nr < 0);
-	check_slab_element(pool, element);
+	check_element(pool, element);
 	return element;
 }
 

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[Andrew Morton]

On Tue, 24 Mar 2015 16:10:01 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:

> Elements backed by the slab allocator are poisoned when added to a
> mempool's reserved pool.
> 
> It is also possible to poison elements backed by the page allocator
> because the mempool layer knows the allocation order.
> 
> This patch extends mempool element poisoning to include memory backed by
> the page allocator.
> 
> This is only effective for configs with CONFIG_DEBUG_SLAB or
> CONFIG_SLUB_DEBUG_ON.
> 

Maybe mempools should get KASAN treatment (as well as this)?

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[Andrey Ryabinin]

On 03/26/2015 12:55 AM, Andrew Morton wrote:
> On Tue, 24 Mar 2015 16:10:01 -0700 (PDT) David Rientjes <rientjes@google.com> wrote:
> 
>> Elements backed by the slab allocator are poisoned when added to a
>> mempool's reserved pool.
>>
>> It is also possible to poison elements backed by the page allocator
>> because the mempool layer knows the allocation order.
>>
>> This patch extends mempool element poisoning to include memory backed by
>> the page allocator.
>>
>> This is only effective for configs with CONFIG_DEBUG_SLAB or
>> CONFIG_SLUB_DEBUG_ON.
>>
> 
> Maybe mempools should get KASAN treatment (as well as this)?
> 

Certainly, I could cook a patch tomorrow.

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[Andrey Ryabinin]

2015-03-25 2:10 GMT+03:00 David Rientjes <rientjes@google.com>:

...
>
> +
> +static void check_element(mempool_t *pool, void *element)
> +{
> +       /* Mempools backed by slab allocator */
> +       if (pool->free == mempool_free_slab || pool->free == mempool_kfree)
> +               __check_element(pool, element, ksize(element));
> +
> +       /* Mempools backed by page allocator */
> +       if (pool->free == mempool_free_pages) {
> +               int order = (int)(long)pool->pool_data;
> +               void *addr = page_address(element);
> +
> +               __check_element(pool, addr, 1UL << (PAGE_SHIFT + order));
>         }
>  }
>
> -static void poison_slab_element(mempool_t *pool, void *element)
> +static void __poison_element(void *element, size_t size)
>  {
> -       if (pool->alloc == mempool_alloc_slab ||
> -           pool->alloc == mempool_kmalloc) {
> -               size_t size = ksize(element);
> -               u8 *obj = element;
> +       u8 *obj = element;
> +
> +       memset(obj, POISON_FREE, size - 1);
> +       obj[size - 1] = POISON_END;
> +}
> +
> +static void poison_element(mempool_t *pool, void *element)
> +{
> +       /* Mempools backed by slab allocator */
> +       if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc)
> +               __poison_element(element, ksize(element));
> +
> +       /* Mempools backed by page allocator */
> +       if (pool->alloc == mempool_alloc_pages) {
> +               int order = (int)(long)pool->pool_data;
> +               void *addr = page_address(element);
>
> -               memset(obj, POISON_FREE, size - 1);
> -               obj[size - 1] = POISON_END;
> +               __poison_element(addr, 1UL << (PAGE_SHIFT + order));

I think, it would be better to use kernel_map_pages() here and in
check_element().
This implies that poison_element()/check_element() has to be moved out of
CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON ifdef (keeping only slab
poisoning under this ifdef).
After these changes it might be a good idea to rename
poison_element()/check_element()
to something like debug_add_element()/debug_remove_element() respectively.

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[David Rientjes]

On Thu, 26 Mar 2015, Andrey Ryabinin wrote:

> > +static void check_element(mempool_t *pool, void *element)
> > +{
> > +       /* Mempools backed by slab allocator */
> > +       if (pool->free == mempool_free_slab || pool->free == mempool_kfree)
> > +               __check_element(pool, element, ksize(element));
> > +
> > +       /* Mempools backed by page allocator */
> > +       if (pool->free == mempool_free_pages) {
> > +               int order = (int)(long)pool->pool_data;
> > +               void *addr = page_address(element);
> > +
> > +               __check_element(pool, addr, 1UL << (PAGE_SHIFT + order));
> >         }
> >  }
> >
> > -static void poison_slab_element(mempool_t *pool, void *element)
> > +static void __poison_element(void *element, size_t size)
> >  {
> > -       if (pool->alloc == mempool_alloc_slab ||
> > -           pool->alloc == mempool_kmalloc) {
> > -               size_t size = ksize(element);
> > -               u8 *obj = element;
> > +       u8 *obj = element;
> > +
> > +       memset(obj, POISON_FREE, size - 1);
> > +       obj[size - 1] = POISON_END;
> > +}
> > +
> > +static void poison_element(mempool_t *pool, void *element)
> > +{
> > +       /* Mempools backed by slab allocator */
> > +       if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc)
> > +               __poison_element(element, ksize(element));
> > +
> > +       /* Mempools backed by page allocator */
> > +       if (pool->alloc == mempool_alloc_pages) {
> > +               int order = (int)(long)pool->pool_data;
> > +               void *addr = page_address(element);
> >
> > -               memset(obj, POISON_FREE, size - 1);
> > -               obj[size - 1] = POISON_END;
> > +               __poison_element(addr, 1UL << (PAGE_SHIFT + order));
> 
> I think, it would be better to use kernel_map_pages() here and in
> check_element().

Hmm, interesting suggestion.

> This implies that poison_element()/check_element() has to be moved out of
> CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON ifdef (keeping only slab
> poisoning under this ifdef).

The mempool poisoning introduced here is really its own poisoning built on 
top of whatever the mempool allocator is.  Otherwise, it would have called 
into the slab subsystem to do the poisoning and include any allocated 
space beyond the object size itself.  Mempool poisoning is agnostic to the 
underlying memory just like the chain of elements is, mempools don't even 
store size.

We don't have a need to set PAGE_EXT_DEBUG_POISON on these pages sitting 
in the reserved pool, nor do we have a need to do kmap_atomic() since it's 
already mapped and must be mapped to be on the reserved pool, which is 
handled by mempool_free().

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[Andrey Ryabinin]

On 03/27/2015 01:50 AM, David Rientjes wrote:
> On Thu, 26 Mar 2015, Andrey Ryabinin wrote:
> 
>>> +static void check_element(mempool_t *pool, void *element)
>>> +{
>>> +       /* Mempools backed by slab allocator */
>>> +       if (pool->free == mempool_free_slab || pool->free == mempool_kfree)
>>> +               __check_element(pool, element, ksize(element));
>>> +
>>> +       /* Mempools backed by page allocator */
>>> +       if (pool->free == mempool_free_pages) {
>>> +               int order = (int)(long)pool->pool_data;
>>> +               void *addr = page_address(element);
>>> +
>>> +               __check_element(pool, addr, 1UL << (PAGE_SHIFT + order));
>>>         }
>>>  }
>>>
>>> -static void poison_slab_element(mempool_t *pool, void *element)
>>> +static void __poison_element(void *element, size_t size)
>>>  {
>>> -       if (pool->alloc == mempool_alloc_slab ||
>>> -           pool->alloc == mempool_kmalloc) {
>>> -               size_t size = ksize(element);
>>> -               u8 *obj = element;
>>> +       u8 *obj = element;
>>> +
>>> +       memset(obj, POISON_FREE, size - 1);
>>> +       obj[size - 1] = POISON_END;
>>> +}
>>> +
>>> +static void poison_element(mempool_t *pool, void *element)
>>> +{
>>> +       /* Mempools backed by slab allocator */
>>> +       if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc)
>>> +               __poison_element(element, ksize(element));
>>> +
>>> +       /* Mempools backed by page allocator */
>>> +       if (pool->alloc == mempool_alloc_pages) {
>>> +               int order = (int)(long)pool->pool_data;
>>> +               void *addr = page_address(element);
>>>
>>> -               memset(obj, POISON_FREE, size - 1);
>>> -               obj[size - 1] = POISON_END;
>>> +               __poison_element(addr, 1UL << (PAGE_SHIFT + order));
>>
>> I think, it would be better to use kernel_map_pages() here and in
>> check_element().
> 
> Hmm, interesting suggestion.
> 
>> This implies that poison_element()/check_element() has to be moved out of
>> CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON ifdef (keeping only slab
>> poisoning under this ifdef).
> 
> The mempool poisoning introduced here is really its own poisoning built on 
> top of whatever the mempool allocator is.  Otherwise, it would have called 
> into the slab subsystem to do the poisoning and include any allocated 
> space beyond the object size itself. 

Perhaps, that would be a good thing to do. I mean it makes sense to check redzone
for corruption.

> Mempool poisoning is agnostic to the 
> underlying memory just like the chain of elements is, mempools don't even 
> store size.
> 
> We don't have a need to set PAGE_EXT_DEBUG_POISON on these pages sitting 
> in the reserved pool, nor do we have a need to do kmap_atomic() since it's 
> already mapped and must be mapped to be on the reserved pool, which is 
> handled by mempool_free().
> 

Well, yes. But this applies only to architectures that don't have ARCH_SUPPORTS_DEBUG_PAGEALLOC.
The rest of arches will only benefit from this as kernel_map_pages() potentially could find more bugs.

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[Andrey Ryabinin]

On 03/27/2015 01:50 AM, David Rientjes wrote:
> We don't have a need to set PAGE_EXT_DEBUG_POISON on these pages sitting 
> in the reserved pool, nor do we have a need to do kmap_atomic() since it's 
> already mapped and must be mapped to be on the reserved pool, which is 
> handled by mempool_free().
> 

Hmm.. I just realized that this statement might be wrong.
Why pages has to be mapped to be on reserved pool?
mempool could be used for highmem pages and there is no need to kmap()
until these pages will be used.

drbd (drivers/block/drbd) already uses mempool for highmem pages:

static int drbd_create_mempools(void)
{
....
	drbd_md_io_page_pool = mempool_create_page_pool(DRBD_MIN_POOL_PAGES, 0);
....
}



static void bm_page_io_async(struct drbd_bm_aio_ctx *ctx, int page_nr) __must_hold(local)
{
....
		page = mempool_alloc(drbd_md_io_page_pool, __GFP_HIGHMEM|__GFP_WAIT);
		copy_highpage(page, b->bm_pages[page_nr]);

Re: [patch v2 4/4] mm, mempool: poison elements backed by page allocator

[David Rientjes]

On Tue, 31 Mar 2015, Andrey Ryabinin wrote:

> > We don't have a need to set PAGE_EXT_DEBUG_POISON on these pages sitting 
> > in the reserved pool, nor do we have a need to do kmap_atomic() since it's 
> > already mapped and must be mapped to be on the reserved pool, which is 
> > handled by mempool_free().
> > 
> 
> Hmm.. I just realized that this statement might be wrong.
> Why pages has to be mapped to be on reserved pool?
> mempool could be used for highmem pages and there is no need to kmap()
> until these pages will be used.
> 
> drbd (drivers/block/drbd) already uses mempool for highmem pages:
> 

Yes, you're exactly right, I didn't see this because the mempool is 
created in one file and then solely used in another file, but regardless 
we still need protection from this usecase.

[patch -mm] mm, mempool: poison elements backed by page allocator fix fix

[David Rientjes]

Elements backed by the page allocator might not be directly mapped into 
lowmem, so do k{,un}map_atomic() before poisoning and verifying contents 
to map into lowmem and return the virtual adddress.

Reported-by: Andrey Ryabinin <a.ryabinin@samsung.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/mempool.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/mempool.c b/mm/mempool.c
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -61,9 +61,10 @@ static void check_element(mempool_t *pool, void *element)
 	/* Mempools backed by page allocator */
 	if (pool->free == mempool_free_pages) {
 		int order = (int)(long)pool->pool_data;
-		void *addr = page_address(element);
+		void *addr = kmap_atomic((struct page *)element);
 
 		__check_element(pool, addr, 1UL << (PAGE_SHIFT + order));
+		kunmap_atomic(addr);
 	}
 }
 
@@ -84,9 +85,10 @@ static void poison_element(mempool_t *pool, void *element)
 	/* Mempools backed by page allocator */
 	if (pool->alloc == mempool_alloc_pages) {
 		int order = (int)(long)pool->pool_data;
-		void *addr = page_address(element);
+		void *addr = kmap_atomic((struct page *)element);
 
 		__poison_element(addr, 1UL << (PAGE_SHIFT + order));
+		kunmap_atomic(addr);
 	}
 }
 #else /* CONFIG_DEBUG_SLAB || CONFIG_SLUB_DEBUG_ON */

Re: [patch 1/4] fs, jfs: remove slab object constructor

[Dave Kleikamp]

On 03/24/2015 06:08 PM, David Rientjes wrote:
> Mempools based on slab caches with object constructors are risky because
> element allocation can happen either from the slab cache itself, meaning
> the constructor is properly called before returning, or from the mempool
> reserve pool, meaning the constructor is not called before returning,
> depending on the allocation context.
> 
> For this reason, we should disallow creating mempools based on slab
> caches that have object constructors.  Callers of mempool_alloc() will
> be responsible for properly initializing the returned element.
> 
> Then, it doesn't matter if the element came from the slab cache or the
> mempool reserved pool.
> 
> The only occurrence of a mempool being based on a slab cache with an
> object constructor in the tree is in fs/jfs/jfs_metapage.c.  Remove it
> and properly initialize the element in alloc_metapage().
> 
> At the same time, META_free is never used, so remove it as well.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>
Acked-by: Dave Kleikamp <dave.kleikamp@oracle.com>

> ---
>  fs/jfs/jfs_metapage.c | 31 ++++++++++++-------------------
>  fs/jfs/jfs_metapage.h |  1 -
>  2 files changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/jfs/jfs_metapage.c b/fs/jfs/jfs_metapage.c
> --- a/fs/jfs/jfs_metapage.c
> +++ b/fs/jfs/jfs_metapage.c
> @@ -183,30 +183,23 @@ static inline void remove_metapage(struct page *page, struct metapage *mp)
>  
>  #endif
>  
> -static void init_once(void *foo)
> -{
> -	struct metapage *mp = (struct metapage *)foo;
> -
> -	mp->lid = 0;
> -	mp->lsn = 0;
> -	mp->flag = 0;
> -	mp->data = NULL;
> -	mp->clsn = 0;
> -	mp->log = NULL;
> -	set_bit(META_free, &mp->flag);
> -	init_waitqueue_head(&mp->wait);
> -}
> -
>  static inline struct metapage *alloc_metapage(gfp_t gfp_mask)
>  {
> -	return mempool_alloc(metapage_mempool, gfp_mask);
> +	struct metapage *mp = mempool_alloc(metapage_mempool, gfp_mask);
> +
> +	if (mp) {
> +		mp->lid = 0;
> +		mp->lsn = 0;
> +		mp->data = NULL;
> +		mp->clsn = 0;
> +		mp->log = NULL;
> +		init_waitqueue_head(&mp->wait);
> +	}
> +	return mp;
>  }
>  
>  static inline void free_metapage(struct metapage *mp)
>  {
> -	mp->flag = 0;
> -	set_bit(META_free, &mp->flag);
> -
>  	mempool_free(mp, metapage_mempool);
>  }
>  
> @@ -216,7 +209,7 @@ int __init metapage_init(void)
>  	 * Allocate the metapage structures
>  	 */
>  	metapage_cache = kmem_cache_create("jfs_mp", sizeof(struct metapage),
> -					   0, 0, init_once);
> +					   0, 0, NULL);
>  	if (metapage_cache == NULL)
>  		return -ENOMEM;
>  
> diff --git a/fs/jfs/jfs_metapage.h b/fs/jfs/jfs_metapage.h
> --- a/fs/jfs/jfs_metapage.h
> +++ b/fs/jfs/jfs_metapage.h
> @@ -48,7 +48,6 @@ struct metapage {
>  
>  /* metapage flag */
>  #define META_locked	0
> -#define META_free	1
>  #define META_dirty	2
>  #define META_sync	3
>  #define META_discard	4
> 

Re: [patch 1/4] fs, jfs: remove slab object constructor

[Mikulas Patocka]

On Tue, 24 Mar 2015, David Rientjes wrote:

> Mempools based on slab caches with object constructors are risky because
> element allocation can happen either from the slab cache itself, meaning
> the constructor is properly called before returning, or from the mempool
> reserve pool, meaning the constructor is not called before returning,
> depending on the allocation context.

I don't think there is any problem. If the allocation is hapenning from 
the slab cache, the constructor is called from the slab sybsystem.

If the allocation is hapenning from the mempool reserve, the constructor 
was called in the past (when the mempool reserve was refilled from the 
cache). So, in both cases, the object allocated frmo the mempool is 
constructed.

Mikulas

> For this reason, we should disallow creating mempools based on slab
> caches that have object constructors.  Callers of mempool_alloc() will
> be responsible for properly initializing the returned element.
> 
> Then, it doesn't matter if the element came from the slab cache or the
> mempool reserved pool.
> 
> The only occurrence of a mempool being based on a slab cache with an
> object constructor in the tree is in fs/jfs/jfs_metapage.c.  Remove it
> and properly initialize the element in alloc_metapage().
> 
> At the same time, META_free is never used, so remove it as well.

Re: [patch 1/4] fs, jfs: remove slab object constructor

[David Rientjes]

On Wed, 25 Mar 2015, Mikulas Patocka wrote:

> > Mempools based on slab caches with object constructors are risky because
> > element allocation can happen either from the slab cache itself, meaning
> > the constructor is properly called before returning, or from the mempool
> > reserve pool, meaning the constructor is not called before returning,
> > depending on the allocation context.
> 
> I don't think there is any problem. If the allocation is hapenning from 
> the slab cache, the constructor is called from the slab sybsystem.
> 
> If the allocation is hapenning from the mempool reserve, the constructor 
> was called in the past (when the mempool reserve was refilled from the 
> cache). So, in both cases, the object allocated frmo the mempool is 
> constructed.
> 

That would be true only for

	ptr = mempool_alloc(gfp, pool);
	mempool_free(ptr, pool);

and nothing in between, and that's pretty pointless.  Typically, callers 
allocate memory, modify it, and then free it.  When that happens with 
mempools, and we can't allocate slab because of the gfp context, mempools 
will return elements in the state in which they were freed (modified, not 
as constructed).

Re: [patch 1/4] fs, jfs: remove slab object constructor

[Christoph Hellwig]

On Wed, Mar 25, 2015 at 07:37:40PM -0700, David Rientjes wrote:
> That would be true only for
> 
> 	ptr = mempool_alloc(gfp, pool);
> 	mempool_free(ptr, pool);
> 
> and nothing in between, and that's pretty pointless.  Typically, callers 
> allocate memory, modify it, and then free it.  When that happens with 
> mempools, and we can't allocate slab because of the gfp context, mempools 
> will return elements in the state in which they were freed (modified, not 
> as constructed).

The historic slab allocator (Solaris and early Linux) expects objects
to be returned in the same / similar enough form as the constructor
returned it, and the constructor is only called when allocating pages
from the page pool.

I have to admit that I haven't used this feature forever, and I have no idea if
people changed how the allocator works in the meantime.

Re: [patch 1/4] fs, jfs: remove slab object constructor

[Dave Kleikamp]

On 03/26/2015 02:28 AM, Christoph Hellwig wrote:
> On Wed, Mar 25, 2015 at 07:37:40PM -0700, David Rientjes wrote:
>> That would be true only for
>>
>> 	ptr = mempool_alloc(gfp, pool);
>> 	mempool_free(ptr, pool);
>>
>> and nothing in between, and that's pretty pointless.  Typically, callers 
>> allocate memory, modify it, and then free it.  When that happens with 
>> mempools, and we can't allocate slab because of the gfp context, mempools 
>> will return elements in the state in which they were freed (modified, not 
>> as constructed).
> 
> The historic slab allocator (Solaris and early Linux) expects objects
> to be returned in the same / similar enough form as the constructor
> returned it, and the constructor is only called when allocating pages
> from the page pool.

I'm pretty sure that this was the intention of the jfs code. Returned
objects should have these fields returned to their initial values. It
does seem error-prone, though. If jfs is in fact the last user of the
constructor, it's probably time for it to die.

> 
> I have to admit that I haven't used this feature forever, and I have no idea if
> people changed how the allocator works in the meantime.