[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