[PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Jesper Juhl]

Greetings & Salutations,

The Coverity checker spotted two potential memory leaks in 
drivers/message/fusion/mptbase.c::mpt_attach().

There are two returns that may leak the storage allocated for 
'ioc' (sizeof(MPT_ADAPTER) bytes).
A simple fix would be to simply add two kfree() calls before 
the return statements, but a better fix (that this patch 
implements) is to reorder the code so that if we hit the first 
return condition we don't have to do the allocation at all and 
then just add a kfree() call for the second case.

Please consider applying.  Patch has been compile tested only.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 drivers/message/fusion/mptbase.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index e866dac..f9bb705 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1393,18 +1393,18 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 	struct proc_dir_entry *dent, *ent;
 #endif
 
+	if (mpt_debug_level)
+		printk(KERN_INFO MYNAM ": mpt_debug_level=%xh\n", mpt_debug_level);
+
+	if (pci_enable_device(pdev))
+		return r;
+
 	ioc = kzalloc(sizeof(MPT_ADAPTER), GFP_ATOMIC);
 	if (ioc == NULL) {
 		printk(KERN_ERR MYNAM ": ERROR - Insufficient memory to add adapter!\n");
 		return -ENOMEM;
 	}
-
 	ioc->debug_level = mpt_debug_level;
-	if (mpt_debug_level)
-		printk(KERN_INFO MYNAM ": mpt_debug_level=%xh\n", mpt_debug_level);
-
-	if (pci_enable_device(pdev))
-		return r;
 
 	dinitprintk(ioc, printk(KERN_WARNING MYNAM ": mpt_adapter_install\n"));
 
@@ -1413,6 +1413,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 			": 64 BIT PCI BUS DMA ADDRESSING SUPPORTED\n"));
 	} else if (pci_set_dma_mask(pdev, DMA_32BIT_MASK)) {
 		printk(KERN_WARNING MYNAM ": 32 BIT PCI BUS DMA ADDRESSING NOT SUPPORTED\n");
+		kfree(ioc);
 		return r;
 	}
 

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Andrew Morton]

On Thu, 2 Aug 2007 01:55:33 +0200
Jesper Juhl <jesper.juhl@gmail.com> wrote:

> Greetings & Salutations,
> 
> The Coverity checker spotted two potential memory leaks in 
> drivers/message/fusion/mptbase.c::mpt_attach().
> 
> There are two returns that may leak the storage allocated for 
> 'ioc' (sizeof(MPT_ADAPTER) bytes).
> A simple fix would be to simply add two kfree() calls before 
> the return statements, but a better fix (that this patch 
> implements) is to reorder the code so that if we hit the first 
> return condition we don't have to do the allocation at all and 
> then just add a kfree() call for the second case.
> 
> Please consider applying.  Patch has been compile tested only.
> 
> 

umm,

> ---
> 
>  drivers/message/fusion/mptbase.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index e866dac..f9bb705 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1393,18 +1393,18 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  	struct proc_dir_entry *dent, *ent;
>  #endif
>  
> +	if (mpt_debug_level)
> +		printk(KERN_INFO MYNAM ": mpt_debug_level=%xh\n", mpt_debug_level);
> +
> +	if (pci_enable_device(pdev))
> +		return r;
> +
>  	ioc = kzalloc(sizeof(MPT_ADAPTER), GFP_ATOMIC);

Why on earth is that using GFP_ATOMIC?  This function later goes on to
create procfs files and such things.




y'know, we could have a debug option which will spit warnings if someone
does a !__GFP_WAIT allocation while !in_atomic() (only works if
CONFIG_PREEMPT).  

But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
the stuff it would pick up.

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Matthew Wilcox]

On Wed, Aug 01, 2007 at 05:26:53PM -0700, Andrew Morton wrote:
> Why on earth is that using GFP_ATOMIC?  This function later goes on to
> create procfs files and such things.

Seems fairly common in driver initialisation code.  I removed three
instances of this in the advansys driver.

> y'know, we could have a debug option which will spit warnings if someone
> does a !__GFP_WAIT allocation while !in_atomic() (only works if
> CONFIG_PREEMPT).  
> 
> But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
> the stuff it would pick up.

Seems like you'd get a lot of false positives.  How about a call:

slab_warn_about_atomic_allocs();

right before calling the initcalls, and then

slab_stop_warning_about_atomic_allocs();

after calling them?  That should give people a lot to chew on for a few
months.  Obviously, you would need to not warn about allocations from
interrupt context, as you say above.

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Andrew Morton]

On Wed, 1 Aug 2007 21:03:50 -0600 Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Aug 01, 2007 at 05:26:53PM -0700, Andrew Morton wrote:
> > Why on earth is that using GFP_ATOMIC?  This function later goes on to
> > create procfs files and such things.
> 
> Seems fairly common in driver initialisation code.  I removed three
> instances of this in the advansys driver.

hrm.  People reach for GFP_ATOMIC so often that it becomes a habit, I guess.

It makes one wonder how much that lovely fault-injection framework is being
used.


> > y'know, we could have a debug option which will spit warnings if someone
> > does a !__GFP_WAIT allocation while !in_atomic() (only works if
> > CONFIG_PREEMPT).  
> > 
> > But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
> > the stuff it would pick up.
> 
> Seems like you'd get a lot of false positives.

There would be a few.  mempool does a non-__GFP_WAIT allocation
deliberately, for example (I still think that's fishy btw).

But I don't expect there would be a large number of falsies.  We could add
a __GFP_I_REALLY_MEANT_ATOMIC flag to shut those up.

>  How about a call:
> 
> slab_warn_about_atomic_allocs();
> 
> right before calling the initcalls, and then
> 
> slab_stop_warning_about_atomic_allocs();
> 
> after calling them?  That should give people a lot to chew on for a few
> months.  Obviously, you would need to not warn about allocations from
> interrupt context, as you say above.

Could.  But GFP_ATOMIC at initcall-time really isn't a problem (except that
it can probably also happen at modprobe-time).

What is the major concern is needlessly atomic allocations at regular
runtime.

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Jesper Juhl]

On 02/08/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 2 Aug 2007 01:55:33 +0200
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
[snip]
> > +++ b/drivers/message/fusion/mptbase.c
> > @@ -1393,18 +1393,18 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
> >       struct proc_dir_entry *dent, *ent;
> >  #endif
> >
> > +     if (mpt_debug_level)
> > +             printk(KERN_INFO MYNAM ": mpt_debug_level=%xh\n", mpt_debug_level);
> > +
> > +     if (pci_enable_device(pdev))
> > +             return r;
> > +
> >       ioc = kzalloc(sizeof(MPT_ADAPTER), GFP_ATOMIC);
>
> Why on earth is that using GFP_ATOMIC?  This function later goes on to
> create procfs files and such things.
>
Dunno. But you are right, that does seem a bit odd, but I assumed
there was a reason for it.

>
> y'know, we could have a debug option which will spit warnings if someone
> does a !__GFP_WAIT allocation while !in_atomic() (only works if
> CONFIG_PREEMPT).
>
> But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
> the stuff it would pick up.
>

I can try to cook up something like that tonight...

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Jesper Juhl]

On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
> On 02/08/07, Andrew Morton <akpm@linux-foundation.org> wrote:
[snip]
> > y'know, we could have a debug option which will spit warnings if someone
> > does a !__GFP_WAIT allocation while !in_atomic() (only works if
> > CONFIG_PREEMPT).
> >
> > But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
> > the stuff it would pick up.
> >
> 
> I can try to cook up something like that tonight...
> 

Ok, so I did a quick hack and I'm drowning in dmesg WARN_ON() traces 
with my usual config.

This is what I added : 

diff --git a/mm/slub.c b/mm/slub.c
index 6c6d74f..e60dd9e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -20,6 +20,7 @@
 #include <linux/mempolicy.h>
 #include <linux/ctype.h>
 #include <linux/kallsyms.h>
+#include <linux/hardirq.h>
 
 /*
  * Lock order:
@@ -1568,6 +1569,10 @@ static void __always_inline *slab_alloc(struct kmem_cache *s,
 
 void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
 {
+#ifdef CONFIG_PREEMPT
+	WARN_ON( !in_atomic() && !(gfpflags & __GFP_WAIT) );
+#endif
+
 	return slab_alloc(s, gfpflags, -1, __builtin_return_address(0));
 }
 EXPORT_SYMBOL(kmem_cache_alloc);
@@ -2370,6 +2375,10 @@ void *__kmalloc(size_t size, gfp_t flags)
 {
 	struct kmem_cache *s = get_slab(size, flags);
 
+#ifdef CONFIG_PREEMPT
+	WARN_ON( !in_atomic() && !(flags & __GFP_WAIT) );
+#endif
+
 	if (ZERO_OR_NULL_PTR(s))
 		return s;
 


And this is what I'm getting heaps of : 

...
[  165.128607]  =======================
[  165.128609] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
[  165.128611]  [<c010400a>] show_trace_log_lvl+0x1a/0x30
[  165.128614]  [<c0104cd2>] show_trace+0x12/0x20
[  165.128616]  [<c0104cf6>] dump_stack+0x16/0x20
[  165.128619]  [<c0175ad3>] kmem_cache_alloc+0xe3/0x110
[  165.128622]  [<c015b10e>] mempool_alloc_slab+0xe/0x10
[  165.128625]  [<c015b211>] mempool_alloc+0x31/0xf0
[  165.128628]  [<c019d033>] bio_alloc_bioset+0x73/0x140
[  165.128631]  [<c019d10e>] bio_alloc+0xe/0x20
[  165.128634]  [<c019d6e1>] bio_map_kern+0x31/0x100
[  165.128637]  [<c02207b2>] blk_rq_map_kern+0x52/0x90
[  165.128640]  [<c02c418b>] scsi_execute+0x4b/0xe0
[  165.128643]  [<c02e5f28>] sr_do_ioctl+0xa8/0x230
[  165.128646]  [<c02e64f6>] sr_read_tochdr+0x76/0xb0
[  165.128649]  [<c02e654b>] sr_disk_status+0x1b/0xa0
[  165.128652]  [<c02e69db>] sr_cd_check+0x9b/0x1b0
[  165.128655]  [<c02e4fbd>] sr_media_change+0x7d/0x250
[  165.128659]  [<c02e6b8e>] media_changed+0x5e/0xa0
[  165.128662]  [<c02e6c01>] cdrom_media_changed+0x31/0x40
[  165.128665]  [<c02e51be>] sr_block_media_changed+0xe/0x10
[  165.128668]  [<c019e5a0>] check_disk_change+0x20/0x80
[  165.128671]  [<c02eaec3>] cdrom_open+0x173/0xa10
[  165.128674]  [<c02e526e>] sr_block_open+0x5e/0xa0
[  165.128677]  [<c019ed55>] do_open+0x85/0x2c0
[  165.128680]  [<c019f1b3>] blkdev_open+0x33/0x80
[  165.128683]  [<c0177c34>] __dentry_open+0xe4/0x200
[  165.128686]  [<c0177df5>] nameidata_to_filp+0x35/0x40
[  165.128689]  [<c0177e49>] do_filp_open+0x49/0x60
[  165.128692]  [<c0177ea9>] do_sys_open+0x49/0xe0
[  165.128695]  [<c0177f7c>] sys_open+0x1c/0x20
[  165.128697]  [<c0102fba>] syscall_call+0x7/0xb
...
[  165.134957] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
[  165.134959]  [<c010400a>] show_trace_log_lvl+0x1a/0x30
[  165.134962]  [<c0104cd2>] show_trace+0x12/0x20
[  165.134965]  [<c0104cf6>] dump_stack+0x16/0x20
[  165.134969]  [<c0175ad3>] kmem_cache_alloc+0xe3/0x110
[  165.134971]  [<c015b10e>] mempool_alloc_slab+0xe/0x10
[  165.134974]  [<c015b211>] mempool_alloc+0x31/0xf0
[  165.134977]  [<c0220b3c>] get_request+0xac/0x260
[  165.134981]  [<c022155c>] get_request_wait+0x1c/0x100
[  165.134983]  [<c0221672>] blk_get_request+0x32/0x70
[  165.134986]  [<c02c4162>] scsi_execute+0x22/0xe0
[  165.134989]  [<c02c428c>] scsi_execute_req+0x6c/0xd0
[  165.134991]  [<c02bff70>] ioctl_internal_command+0x40/0x100
[  165.134996]  [<c02c008c>] scsi_set_medium_removal+0x5c/0x90
[  165.134999]  [<c02e5e76>] sr_lock_door+0x16/0x20
[  165.135002]  [<c02e83d4>] cdrom_release+0x104/0x250
[  165.135005]  [<c02e5d74>] sr_block_release+0x24/0x40
[  165.135008]  [<c019eb96>] __blkdev_put+0x146/0x150
[  165.135012]  [<c019ebaa>] blkdev_put+0xa/0x10
[  165.135015]  [<c019f5e2>] blkdev_close+0x32/0x40
[  165.135018]  [<c017a586>] __fput+0xb6/0x180
[  165.135021]  [<c017a6b9>] fput+0x19/0x20
[  165.135024]  [<c0177a37>] filp_close+0x47/0x80
[  165.135027]  [<c0178e46>] sys_close+0x66/0xc0
[  165.135030]  [<c0102fba>] syscall_call+0x7/0xb
[  165.135032]  =======================
[  166.564998] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
[  166.565006]  [<c010400a>] show_trace_log_lvl+0x1a/0x30
[  166.565013]  [<c0104cd2>] show_trace+0x12/0x20
[  166.565016]  [<c0104cf6>] dump_stack+0x16/0x20
[  166.565020]  [<c0175ad3>] kmem_cache_alloc+0xe3/0x110
[  166.565030]  [<c015b10e>] mempool_alloc_slab+0xe/0x10
[  166.565039]  [<c015b211>] mempool_alloc+0x31/0xf0
[  166.565047]  [<c019cfdf>] bio_alloc_bioset+0x1f/0x140
[  166.565057]  [<c019d10e>] bio_alloc+0xe/0x20
[  166.565066]  [<c01997b3>] submit_bh+0x63/0x100
[  166.565075]  [<c01c96f8>] journal_do_submit_data+0x28/0x40
[  166.565085]  [<c01c9e18>] journal_commit_transaction+0x658/0x1290
[  166.565095]  [<c01ce5f2>] kjournald+0xb2/0x1e0
[  166.565103]  [<c013b9a2>] kthread+0x42/0x70
[  166.565112]  [<c0103bff>] kernel_thread_helper+0x7/0x18
[  166.565121]  =======================
...

etc...

So, where do we go from here?

Obviously my patch above is nothing but a quick hack.  
Should I turn that into a proper debug config option?  
Do we even want to clean up this stuff?
Am I even looking at the right thing?

I'm more than willing to try and create a proper debug option patch 
as well as clean up some of these allocations if wanted... What say 
"the powers that be" ?


Kind regards,

  Jesper Juhl <jesper.juhl@gmail.com>


   PS. Please keep me on Cc when replying.

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Andrew Morton]

On Fri, 3 Aug 2007 00:53:44 +0200
Jesper Juhl <jesper.juhl@gmail.com> wrote:

> On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
> > On 02/08/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> [snip]
> > > y'know, we could have a debug option which will spit warnings if someone
> > > does a !__GFP_WAIT allocation while !in_atomic() (only works if
> > > CONFIG_PREEMPT).
> > >
> > > But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
> > > the stuff it would pick up.
> > >
> > 
> > I can try to cook up something like that tonight...
> > 
> 
> Ok, so I did a quick hack and I'm drowning in dmesg WARN_ON() traces 
> with my usual config.
> 
> This is what I added : 
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 6c6d74f..e60dd9e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -20,6 +20,7 @@
>  #include <linux/mempolicy.h>
>  #include <linux/ctype.h>
>  #include <linux/kallsyms.h>
> +#include <linux/hardirq.h>
>  
>  /*
>   * Lock order:
> @@ -1568,6 +1569,10 @@ static void __always_inline *slab_alloc(struct kmem_cache *s,
>  
>  void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
>  {
> +#ifdef CONFIG_PREEMPT
> +	WARN_ON( !in_atomic() && !(gfpflags & __GFP_WAIT) );
> +#endif
> +
>  	return slab_alloc(s, gfpflags, -1, __builtin_return_address(0));
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc);
> @@ -2370,6 +2375,10 @@ void *__kmalloc(size_t size, gfp_t flags)
>  {
>  	struct kmem_cache *s = get_slab(size, flags);
>  
> +#ifdef CONFIG_PREEMPT
> +	WARN_ON( !in_atomic() && !(flags & __GFP_WAIT) );
> +#endif
> +
>  	if (ZERO_OR_NULL_PTR(s))
>  		return s;
>  
> 
> 
> And this is what I'm getting heaps of : 
> 
> ...
> [  165.128607]  =======================
> [  165.128609] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
> [  165.128611]  [<c010400a>] show_trace_log_lvl+0x1a/0x30
> [  165.128614]  [<c0104cd2>] show_trace+0x12/0x20
> [  165.128616]  [<c0104cf6>] dump_stack+0x16/0x20
> [  165.128619]  [<c0175ad3>] kmem_cache_alloc+0xe3/0x110
> [  165.128622]  [<c015b10e>] mempool_alloc_slab+0xe/0x10
> [  165.128625]  [<c015b211>] mempool_alloc+0x31/0xf0

I said you would.

> So, where do we go from here?

Where I said ;) Add a new __GFP_ flag which suppresses the warning, add
that flag to known-to-be-OK callsites, such as mempool_alloc().

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Jesper Juhl]

On 03/08/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 3 Aug 2007 00:53:44 +0200
> Jesper Juhl <jesper.juhl@gmail.com> wrote:
>
> > On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
> > > On 02/08/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > [snip]
> > > > y'know, we could have a debug option which will spit warnings if someone
> > > > does a !__GFP_WAIT allocation while !in_atomic() (only works if
> > > > CONFIG_PREEMPT).
> > > >
> > > > But please, make it depend on !CONFIG_AKPM.  I shudder to think about all
> > > > the stuff it would pick up.
> > > >
> > >
> > > I can try to cook up something like that tonight...
> > >
> >
> > Ok, so I did a quick hack and I'm drowning in dmesg WARN_ON() traces
> > with my usual config.
> >
> > This is what I added :
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 6c6d74f..e60dd9e 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/mempolicy.h>
> >  #include <linux/ctype.h>
> >  #include <linux/kallsyms.h>
> > +#include <linux/hardirq.h>
> >
> >  /*
> >   * Lock order:
> > @@ -1568,6 +1569,10 @@ static void __always_inline *slab_alloc(struct kmem_cache *s,
> >
> >  void *kmem_cache_alloc(struct kmem_cache *s, gfp_t gfpflags)
> >  {
> > +#ifdef CONFIG_PREEMPT
> > +     WARN_ON( !in_atomic() && !(gfpflags & __GFP_WAIT) );
> > +#endif
> > +
> >       return slab_alloc(s, gfpflags, -1, __builtin_return_address(0));
> >  }
> >  EXPORT_SYMBOL(kmem_cache_alloc);
> > @@ -2370,6 +2375,10 @@ void *__kmalloc(size_t size, gfp_t flags)
> >  {
> >       struct kmem_cache *s = get_slab(size, flags);
> >
> > +#ifdef CONFIG_PREEMPT
> > +     WARN_ON( !in_atomic() && !(flags & __GFP_WAIT) );
> > +#endif
> > +
> >       if (ZERO_OR_NULL_PTR(s))
> >               return s;
> >
> >
> >
> > And this is what I'm getting heaps of :
> >
> > ...
> > [  165.128607]  =======================
> > [  165.128609] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
> > [  165.128611]  [<c010400a>] show_trace_log_lvl+0x1a/0x30
> > [  165.128614]  [<c0104cd2>] show_trace+0x12/0x20
> > [  165.128616]  [<c0104cf6>] dump_stack+0x16/0x20
> > [  165.128619]  [<c0175ad3>] kmem_cache_alloc+0xe3/0x110
> > [  165.128622]  [<c015b10e>] mempool_alloc_slab+0xe/0x10
> > [  165.128625]  [<c015b211>] mempool_alloc+0x31/0xf0
>
> I said you would.
>
Hehe, I know you did. I'm not complaining, simply stating facts
(confirming what you said actually).

> > So, where do we go from here?
>
> Where I said ;) Add a new __GFP_ flag which suppresses the warning, add
> that flag to known-to-be-OK callsites, such as mempool_alloc().
>
Ok, I'll try to play around with this some more, try to filter out
false positives and see what I'm left with (if anything - I'm pretty
limited hardware-wise, so I can only test a small subset of drivers,
archs etc) - I'll keep you informed, but expect a few days to pass
before I have any news...


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Andrew Morton]

On Fri, 3 Aug 2007 01:10:02 +0200
"Jesper Juhl" <jesper.juhl@gmail.com> wrote:

> > > So, where do we go from here?
> >
> > Where I said ;) Add a new __GFP_ flag which suppresses the warning, add
> > that flag to known-to-be-OK callsites, such as mempool_alloc().
> >
> Ok, I'll try to play around with this some more, try to filter out
> false positives and see what I'm left with (if anything - I'm pretty
> limited hardware-wise, so I can only test a small subset of drivers,
> archs etc) - I'll keep you informed, but expect a few days to pass
> before I have any news...

Make it a once-off thing for now, so the warning will disable itself after
it has triggered once.  That will prevent the debug feature from making
anyone's kernel unusable.

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Jesper Juhl]

On 03/08/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 3 Aug 2007 01:10:02 +0200
> "Jesper Juhl" <jesper.juhl@gmail.com> wrote:
>
> > > > So, where do we go from here?
> > >
> > > Where I said ;) Add a new __GFP_ flag which suppresses the warning, add
> > > that flag to known-to-be-OK callsites, such as mempool_alloc().
> > >
> > Ok, I'll try to play around with this some more, try to filter out
> > false positives and see what I'm left with (if anything - I'm pretty
> > limited hardware-wise, so I can only test a small subset of drivers,
> > archs etc) - I'll keep you informed, but expect a few days to pass
> > before I have any news...
>
> Make it a once-off thing for now, so the warning will disable itself after
> it has triggered once.  That will prevent the debug feature from making
> anyone's kernel unusable.
>
Ok, I'll do that :-)

Just be a little patient. I'm doing this in my spare time and I do
have a real job/life to attend to, so I'll be playing with this in the
little free time I have over the next couple of days.  I'll get
something done, but don't expect it until a few days have passed :-)

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())

[Christoph Lameter]

Mempools do not want to wait if there is an allocation failure. Its like 
GFP_THISNODE in that we want a failure.

I had to add a

if (NUMA_BUILD && (gfp_mask & GFP_THISNODE) == GFP_THISNODE)
                goto nopage;

in page_alloc.c to make GFP_THISNODE fail.

Maybe add a GFP_FAIL and check for that?


diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index bc68dd9..41b6aa3 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -43,6 +43,7 @@ struct vm_area_struct;
 #define __GFP_REPEAT	((__force gfp_t)0x400u)	/* Retry the allocation.  Might fail */
 #define __GFP_NOFAIL	((__force gfp_t)0x800u)	/* Retry for ever.  Cannot fail */
 #define __GFP_NORETRY	((__force gfp_t)0x1000u)/* Do not retry.  Might fail */
+#define __GFP_FAIL	((__force gfp_t)0x2000u)/* Fail immediately if there is a problem */
 #define __GFP_COMP	((__force gfp_t)0x4000u)/* Add compound page metadata */
 #define __GFP_ZERO	((__force gfp_t)0x8000u)/* Return zeroed page on success */
 #define __GFP_NOMEMALLOC ((__force gfp_t)0x10000u) /* Don't use emergency reserves */
@@ -81,7 +82,8 @@ struct vm_area_struct;
 				 __GFP_MOVABLE)
 
 #ifdef CONFIG_NUMA
-#define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
+#define GFP_THISNODE	(__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY |\
+				__GFP_FAIL)
 #else
 #define GFP_THISNODE	((__force gfp_t)0)
 #endif
diff --git a/mm/mempool.c b/mm/mempool.c
index 02d5ec3..c1ac622 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -211,8 +211,9 @@ void * mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
 	gfp_mask |= __GFP_NOMEMALLOC;	/* don't allocate emergency reserves */
 	gfp_mask |= __GFP_NORETRY;	/* don't loop in __alloc_pages */
 	gfp_mask |= __GFP_NOWARN;	/* failures are OK */
+	gfp_mask |= __GFP_FAIL;
 
-	gfp_temp = gfp_mask & ~(__GFP_WAIT|__GFP_IO);
+	gfp_temp = gfp_mask & ~__GFP_IO;
 
 repeat_alloc:
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3da85b8..58c1a4d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1250,15 +1250,7 @@ restart:
 	if (page)
 		goto got_pg;
 
-	/*
-	 * GFP_THISNODE (meaning __GFP_THISNODE, __GFP_NORETRY and
-	 * __GFP_NOWARN set) should not cause reclaim since the subsystem
-	 * (f.e. slab) using GFP_THISNODE may choose to trigger reclaim
-	 * using a larger set of nodes after it has established that the
-	 * allowed per node queues are empty and that nodes are
-	 * over allocated.
-	 */
-	if (NUMA_BUILD && (gfp_mask & GFP_THISNODE) == GFP_THISNODE)
+	if (gfp_mask & __GFP_FAIL)
 		goto nopage;
 
 	for (z = zonelist->zones; *z; z++)