Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On 02/08/07, Andrew Morton <[EMAIL PROTECTED]> wrote: > On Thu, 2 Aug 2007 01:55:33 +0200 > Jesper Juhl <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Wed, 1 Aug 2007 21:03:50 -0600 Matthew Wilcox <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
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." - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Thu, 2 Aug 2007 01:55:33 +0200 Jesper Juhl <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
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 <[EMAIL PROTECTED]>
---
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;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
