[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

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] 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