[PATCH] md: avoid use of broken kzalloc mempool

[PATCH] md: avoid use of broken kzalloc mempool

[Sage Weil]

The kzalloc mempool does not re-zero items that have been used and then
returned to the pool.  Manually zero the allocated multipath_bh instead.

CC: linux-raid@vger.kernel.org
CC: Neil Brown <neilb@suse.de>
CC: <stable@kernel.org>
Signed-off-by: Sage Weil <sage@newdream.net>
---
 drivers/md/multipath.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
index 237fe3f..151ce69 100644
--- a/drivers/md/multipath.c
+++ b/drivers/md/multipath.c
@@ -150,6 +150,7 @@ static int multipath_make_request (struct request_queue *q, struct bio * bio)
 	}
 
 	mp_bh = mempool_alloc(conf->pool, GFP_NOIO);
+	memset(mp_bh, 0, sizeof(*mp_bh));
 
 	mp_bh->master_bio = bio;
 	mp_bh->mddev = mddev;
@@ -490,7 +491,7 @@ static int multipath_run (mddev_t *mddev)
 	}
 	mddev->degraded = conf->raid_disks - conf->working_disks;
 
-	conf->pool = mempool_create_kzalloc_pool(NR_RESERVED_BUFS,
+	conf->pool = mempool_create_kmalloc_pool(NR_RESERVED_BUFS,
 						 sizeof(struct multipath_bh));
 	if (conf->pool == NULL) {
 		printk(KERN_ERR 
-- 
1.5.6.5

[PATCH] ibmvscsi: avoid unnecessary use of kzalloc_pool

[Sage Weil]

The allocated struct is manually zeroed after allocation, so avoid using
the (broken) kzalloc mempool (which does not re-zero previously used items
when they are returned to the pool).

CC: linux-scsi@vger.kernel.org
CC: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Sage Weil <sage@newdream.net>
---
 drivers/scsi/ibmvscsi/ibmvfc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
index 166d964..bb2c696 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc.c
@@ -4217,7 +4217,7 @@ static int ibmvfc_alloc_mem(struct ibmvfc_host *vhost)
 	if (!vhost->trace)
 		goto free_disc_buffer;
 
-	vhost->tgt_pool = mempool_create_kzalloc_pool(IBMVFC_TGT_MEMPOOL_SZ,
+	vhost->tgt_pool = mempool_create_kmalloc_pool(IBMVFC_TGT_MEMPOOL_SZ,
 						      sizeof(struct ibmvfc_target));
 
 	if (!vhost->tgt_pool) {
-- 
1.5.6.5

[PATCH] mm: remove broken 'kzalloc' mempool

[Sage Weil]

The kzalloc mempool zeros items when they are initially allocated, but
does not rezero used items that are returned to the pool.  Consequently
mempool_alloc()s may return non-zeroed memory.

Since there are/were only two in-tree users for mempool_create_kzalloc_pool(),
and 'fixing' this in a way that will re-zero used (but not new) items
before first use is non-trivial, just remove it.

CC: <linux-mm@kvack.org>
Signed-off-by: Sage Weil <sage@newdream.net>
---
 include/linux/mempool.h |   10 ++--------
 mm/mempool.c            |    7 -------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/include/linux/mempool.h b/include/linux/mempool.h
index 9be484d..7c08052 100644
--- a/include/linux/mempool.h
+++ b/include/linux/mempool.h
@@ -47,22 +47,16 @@ mempool_create_slab_pool(int min_nr, struct kmem_cache *kc)
 }
 
 /*
- * 2 mempool_alloc_t's and a mempool_free_t to kmalloc/kzalloc and kfree
- * the amount of memory specified by pool_data
+ * a mempool_alloc_t and a mempool_free_t to kmalloc and kfree the
+ * amount of memory specified by pool_data
  */
 void *mempool_kmalloc(gfp_t gfp_mask, void *pool_data);
-void *mempool_kzalloc(gfp_t gfp_mask, void *pool_data);
 void mempool_kfree(void *element, void *pool_data);
 static inline mempool_t *mempool_create_kmalloc_pool(int min_nr, size_t size)
 {
 	return mempool_create(min_nr, mempool_kmalloc, mempool_kfree,
 			      (void *) size);
 }
-static inline mempool_t *mempool_create_kzalloc_pool(int min_nr, size_t size)
-{
-	return mempool_create(min_nr, mempool_kzalloc, mempool_kfree,
-			      (void *) size);
-}
 
 /*
  * A mempool_alloc_t and mempool_free_t for a simple page allocator that
diff --git a/mm/mempool.c b/mm/mempool.c
index a46eb1b..eea4f7d 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -308,13 +308,6 @@ void *mempool_kmalloc(gfp_t gfp_mask, void *pool_data)
 }
 EXPORT_SYMBOL(mempool_kmalloc);
 
-void *mempool_kzalloc(gfp_t gfp_mask, void *pool_data)
-{
-	size_t size = (size_t) pool_data;
-	return kzalloc(size, gfp_mask);
-}
-EXPORT_SYMBOL(mempool_kzalloc);
-
 void mempool_kfree(void *element, void *pool_data)
 {
 	kfree(element);
-- 
1.5.6.5

Re: [PATCH] mm: remove broken 'kzalloc' mempool

[Sage Weil]

On Mon, 3 Aug 2009, Sage Weil wrote:
> The kzalloc mempool zeros items when they are initially allocated, but
> does not rezero used items that are returned to the pool.  Consequently
> mempool_alloc()s may return non-zeroed memory.
> 
> Since there are/were only two in-tree users for mempool_create_kzalloc_pool(),
> and 'fixing' this in a way that will re-zero used (but not new) items
> before first use is non-trivial, just remove it.

This should of course only be applied after the fixes for the two callers 
(dm multipath and ibmvscsi), whatever the protocol for order that may 
be...

sage


> 
> CC: <linux-mm@kvack.org>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  include/linux/mempool.h |   10 ++--------
>  mm/mempool.c            |    7 -------
>  2 files changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/mempool.h b/include/linux/mempool.h
> index 9be484d..7c08052 100644
> --- a/include/linux/mempool.h
> +++ b/include/linux/mempool.h
> @@ -47,22 +47,16 @@ mempool_create_slab_pool(int min_nr, struct kmem_cache *kc)
>  }
>  
>  /*
> - * 2 mempool_alloc_t's and a mempool_free_t to kmalloc/kzalloc and kfree
> - * the amount of memory specified by pool_data
> + * a mempool_alloc_t and a mempool_free_t to kmalloc and kfree the
> + * amount of memory specified by pool_data
>   */
>  void *mempool_kmalloc(gfp_t gfp_mask, void *pool_data);
> -void *mempool_kzalloc(gfp_t gfp_mask, void *pool_data);
>  void mempool_kfree(void *element, void *pool_data);
>  static inline mempool_t *mempool_create_kmalloc_pool(int min_nr, size_t size)
>  {
>  	return mempool_create(min_nr, mempool_kmalloc, mempool_kfree,
>  			      (void *) size);
>  }
> -static inline mempool_t *mempool_create_kzalloc_pool(int min_nr, size_t size)
> -{
> -	return mempool_create(min_nr, mempool_kzalloc, mempool_kfree,
> -			      (void *) size);
> -}
>  
>  /*
>   * A mempool_alloc_t and mempool_free_t for a simple page allocator that
> diff --git a/mm/mempool.c b/mm/mempool.c
> index a46eb1b..eea4f7d 100644
> --- a/mm/mempool.c
> +++ b/mm/mempool.c
> @@ -308,13 +308,6 @@ void *mempool_kmalloc(gfp_t gfp_mask, void *pool_data)
>  }
>  EXPORT_SYMBOL(mempool_kmalloc);
>  
> -void *mempool_kzalloc(gfp_t gfp_mask, void *pool_data)
> -{
> -	size_t size = (size_t) pool_data;
> -	return kzalloc(size, gfp_mask);
> -}
> -EXPORT_SYMBOL(mempool_kzalloc);
> -
>  void mempool_kfree(void *element, void *pool_data)
>  {
>  	kfree(element);
> -- 
> 1.5.6.5
> 
> 

Re: [PATCH] md: avoid use of broken kzalloc mempool

[Andi Kleen]

Sage Weil <sage@newdream.net> writes:

> The kzalloc mempool does not re-zero items that have been used and then
> returned to the pool.  Manually zero the allocated multipath_bh instead.

Why not fix that in mempool instead? That would seem like the right place
to fix such a bug, instead of adding workarounds to all callers.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] ibmvscsi: avoid unnecessary use of kzalloc_pool

[Brian King]

Acked-by: Brian King <brking@linux.vnet.ibm.com>

Sage Weil wrote:
> The allocated struct is manually zeroed after allocation, so avoid using
> the (broken) kzalloc mempool (which does not re-zero previously used items
> when they are returned to the pool).
> 
> CC: linux-scsi@vger.kernel.org
> CC: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  drivers/scsi/ibmvscsi/ibmvfc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 166d964..bb2c696 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -4217,7 +4217,7 @@ static int ibmvfc_alloc_mem(struct ibmvfc_host *vhost)
>  	if (!vhost->trace)
>  		goto free_disc_buffer;
> 
> -	vhost->tgt_pool = mempool_create_kzalloc_pool(IBMVFC_TGT_MEMPOOL_SZ,
> +	vhost->tgt_pool = mempool_create_kmalloc_pool(IBMVFC_TGT_MEMPOOL_SZ,
>  						      sizeof(struct ibmvfc_target));
> 
>  	if (!vhost->tgt_pool) {


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH] md: avoid use of broken kzalloc mempool

[Sage Weil]

On Mon, 3 Aug 2009, Andi Kleen wrote:
> Sage Weil <sage@newdream.net> writes:
> 
> > The kzalloc mempool does not re-zero items that have been used and then
> > returned to the pool.  Manually zero the allocated multipath_bh instead.
> 
> Why not fix that in mempool instead? That would seem like the right place
> to fix such a bug, instead of adding workarounds to all callers.

That was my original proposal, but Andrew suggested it wasn't worth the 
added complexity for only 2 users (or actually, just 1).  See this thread:

http://marc.info/?l=linux-mm&m=124933189913260&w=2

sage

Re: [PATCH] md: avoid use of broken kzalloc mempool

[NeilBrown]

On Tue, August 4, 2009 6:54 am, Sage Weil wrote:
> The kzalloc mempool does not re-zero items that have been used and then
> returned to the pool.  Manually zero the allocated multipath_bh instead.
>
> CC: linux-raid@vger.kernel.org
> CC: Neil Brown <neilb@suse.de>

Acked-by: NeilBrown <neilb@suse.de>

Thanks.

In fact, you don't even need the memset.  After allocation
no assumptions are made about the content of the structure.
Every field is immediately initialised except retry_list, which
is never accessed until after the object is placed on a list.h list.

So this code never needed kzalloc_pool.

Thanks,
NeilBrown

> CC: <stable@kernel.org>
> Signed-off-by: Sage Weil <sage@newdream.net>
> ---
>  drivers/md/multipath.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
> index 237fe3f..151ce69 100644
> --- a/drivers/md/multipath.c
> +++ b/drivers/md/multipath.c
> @@ -150,6 +150,7 @@ static int multipath_make_request (struct
> request_queue *q, struct bio * bio)
>  	}
>
>  	mp_bh = mempool_alloc(conf->pool, GFP_NOIO);
> +	memset(mp_bh, 0, sizeof(*mp_bh));
>
>  	mp_bh->master_bio = bio;
>  	mp_bh->mddev = mddev;
> @@ -490,7 +491,7 @@ static int multipath_run (mddev_t *mddev)
>  	}
>  	mddev->degraded = conf->raid_disks - conf->working_disks;
>
> -	conf->pool = mempool_create_kzalloc_pool(NR_RESERVED_BUFS,
> +	conf->pool = mempool_create_kmalloc_pool(NR_RESERVED_BUFS,
>  						 sizeof(struct multipath_bh));
>  	if (conf->pool == NULL) {
>  		printk(KERN_ERR
> --
> 1.5.6.5
>

Re: [PATCH] md: avoid use of broken kzalloc mempool

[Andrew Morton]

On Tue, 4 Aug 2009 08:03:52 +1000 (EST)
"NeilBrown" <neilb@suse.de> wrote:

> On Tue, August 4, 2009 6:54 am, Sage Weil wrote:
> > The kzalloc mempool does not re-zero items that have been used and then
> > returned to the pool.  Manually zero the allocated multipath_bh instead.
> >
> > CC: linux-raid@vger.kernel.org
> > CC: Neil Brown <neilb@suse.de>
> > CC: <stable@kernel.org>

Sage, why did you cc stable@kernel.org?  The patch fixes some bug? 
What is it?  Why is the fix sufficiently important to warrant
backporting?

> Acked-by: NeilBrown <neilb@suse.de>
> 

Thanks, I'll merge it once the above is understood.

> 
> In fact, you don't even need the memset.  After allocation
> no assumptions are made about the content of the structure.
> Every field is immediately initialised except retry_list, which
> is never accessed until after the object is placed on a list.h list.
> 
> So this code never needed kzalloc_pool.

Well, that's a bit of a functional change so let us (ie: you ;)) do
that separately?

Re: [PATCH] md: avoid use of broken kzalloc mempool

[Sage Weil]

On Tue, 11 Aug 2009, Andrew Morton wrote:

> On Tue, 4 Aug 2009 08:03:52 +1000 (EST)
> "NeilBrown" <neilb@suse.de> wrote:
> 
> > On Tue, August 4, 2009 6:54 am, Sage Weil wrote:
> > > The kzalloc mempool does not re-zero items that have been used and then
> > > returned to the pool.  Manually zero the allocated multipath_bh instead.
> > >
> > > CC: linux-raid@vger.kernel.org
> > > CC: Neil Brown <neilb@suse.de>
> > > CC: <stable@kernel.org>
> 
> Sage, why did you cc stable@kernel.org?  The patch fixes some bug? 
> What is it?  Why is the fix sufficiently important to warrant
> backporting?

I thought it fixed a bug in multipath (though as Neil points out it turns 
out that code wasn't relying on the structure being zeroed, so there's no 
need to backport).  It would have been pretty rare, only affecting 
multipath after low-memory conditions.  I'm not sure what the criteria are 
for stable, so I thought I'd be on the safe side.

sage


> > Acked-by: NeilBrown <neilb@suse.de>
> > 
> 
> Thanks, I'll merge it once the above is understood.
> 
> > 
> > In fact, you don't even need the memset.  After allocation
> > no assumptions are made about the content of the structure.
> > Every field is immediately initialised except retry_list, which
> > is never accessed until after the object is placed on a list.h list.
> > 
> > So this code never needed kzalloc_pool.
> 
> Well, that's a bit of a functional change so let us (ie: you ;)) do
> that separately?
> 
> 

Re: [PATCH] md: avoid use of broken kzalloc mempool

[Neil Brown]

On Tuesday August 11, akpm@linux-foundation.org wrote:
> On Tue, 4 Aug 2009 08:03:52 +1000 (EST)
> "NeilBrown" <neilb@suse.de> wrote:
> 
> > On Tue, August 4, 2009 6:54 am, Sage Weil wrote:
> > > The kzalloc mempool does not re-zero items that have been used and then
> > > returned to the pool.  Manually zero the allocated multipath_bh instead.
> > >
> > > CC: linux-raid@vger.kernel.org
> > > CC: Neil Brown <neilb@suse.de>
> > > CC: <stable@kernel.org>
> 
> Sage, why did you cc stable@kernel.org?  The patch fixes some bug? 
> What is it?  Why is the fix sufficiently important to warrant
> backporting?
> 
> > Acked-by: NeilBrown <neilb@suse.de>
> > 
> 
> Thanks, I'll merge it once the above is understood.
> 
> > 
> > In fact, you don't even need the memset.  After allocation
> > no assumptions are made about the content of the structure.
> > Every field is immediately initialised except retry_list, which
> > is never accessed until after the object is placed on a list.h list.
> > 
> > So this code never needed kzalloc_pool.
> 
> Well, that's a bit of a functional change so let us (ie: you ;)) do
> that separately?

Yes, I'll keep an eye out for your change to be merged, then I'll
apply mine.

NeilBrown