Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
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)0x1u) /* 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++) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On 03/08/07, Andrew Morton <[EMAIL PROTECTED]> wrote: > On Fri, 3 Aug 2007 01:10:02 +0200 > "Jesper Juhl" <[EMAIL PROTECTED]> 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 <[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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Fri, 3 Aug 2007 01:10:02 +0200 "Jesper Juhl" <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On 03/08/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
> On Fri, 3 Aug 2007 00:53:44 +0200
> Jesper Juhl <[EMAIL PROTECTED]> wrote:
>
> > On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
> > > On 02/08/07, Andrew Morton <[EMAIL PROTECTED]> 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
> > #include
> > #include
> > +#include
> >
> > /*
> > * 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] [] show_trace_log_lvl+0x1a/0x30
> > [ 165.128614] [] show_trace+0x12/0x20
> > [ 165.128616] [] dump_stack+0x16/0x20
> > [ 165.128619] [] kmem_cache_alloc+0xe3/0x110
> > [ 165.128622] [] mempool_alloc_slab+0xe/0x10
> > [ 165.128625] [] 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 <[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-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Fri, 3 Aug 2007 00:53:44 +0200
Jesper Juhl <[EMAIL PROTECTED]> wrote:
> On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
> > On 02/08/07, Andrew Morton <[EMAIL PROTECTED]> 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
> #include
> #include
> +#include
>
> /*
> * 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] [] show_trace_log_lvl+0x1a/0x30
> [ 165.128614] [] show_trace+0x12/0x20
> [ 165.128616] [] dump_stack+0x16/0x20
> [ 165.128619] [] kmem_cache_alloc+0xe3/0x110
> [ 165.128622] [] mempool_alloc_slab+0xe/0x10
> [ 165.128625] [] 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().
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
> On 02/08/07, Andrew Morton <[EMAIL PROTECTED]> 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
#include
#include
+#include
/*
* 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] [] show_trace_log_lvl+0x1a/0x30
[ 165.128614] [] show_trace+0x12/0x20
[ 165.128616] [] dump_stack+0x16/0x20
[ 165.128619] [] kmem_cache_alloc+0xe3/0x110
[ 165.128622] [] mempool_alloc_slab+0xe/0x10
[ 165.128625] [] mempool_alloc+0x31/0xf0
[ 165.128628] [] bio_alloc_bioset+0x73/0x140
[ 165.128631] [] bio_alloc+0xe/0x20
[ 165.128634] [] bio_map_kern+0x31/0x100
[ 165.128637] [] blk_rq_map_kern+0x52/0x90
[ 165.128640] [] scsi_execute+0x4b/0xe0
[ 165.128643] [] sr_do_ioctl+0xa8/0x230
[ 165.128646] [] sr_read_tochdr+0x76/0xb0
[ 165.128649] [] sr_disk_status+0x1b/0xa0
[ 165.128652] [] sr_cd_check+0x9b/0x1b0
[ 165.128655] [] sr_media_change+0x7d/0x250
[ 165.128659] [] media_changed+0x5e/0xa0
[ 165.128662] [] cdrom_media_changed+0x31/0x40
[ 165.128665] [] sr_block_media_changed+0xe/0x10
[ 165.128668] [] check_disk_change+0x20/0x80
[ 165.128671] [] cdrom_open+0x173/0xa10
[ 165.128674] [] sr_block_open+0x5e/0xa0
[ 165.128677] [] do_open+0x85/0x2c0
[ 165.128680] [] blkdev_open+0x33/0x80
[ 165.128683] [] __dentry_open+0xe4/0x200
[ 165.128686] [] nameidata_to_filp+0x35/0x40
[ 165.128689] [] do_filp_open+0x49/0x60
[ 165.128692] [] do_sys_open+0x49/0xe0
[ 165.128695] [] sys_open+0x1c/0x20
[ 165.128697] [] syscall_call+0x7/0xb
...
[ 165.134957] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
[ 165.134959] [] show_trace_log_lvl+0x1a/0x30
[ 165.134962] [] show_trace+0x12/0x20
[ 165.134965] [] dump_stack+0x16/0x20
[ 165.134969] [] kmem_cache_alloc+0xe3/0x110
[ 165.134971] [] mempool_alloc_slab+0xe/0x10
[ 165.134974] [] mempool_alloc+0x31/0xf0
[ 165.134977] [] get_request+0xac/0x260
[ 165.134981] [] get_request_wait+0x1c/0x100
[ 165.134983] [] blk_get_request+0x32/0x70
[ 165.134986] [] scsi_execute+0x22/0xe0
[ 165.134989] [] scsi_execute_req+0x6c/0xd0
[ 165.134991] [] ioctl_internal_command+0x40/0x100
[ 165.134996] [] scsi_set_medium_removal+0x5c/0x90
[ 165.134999] [] sr_lock_door+0x16/0x20
[ 165.135002] [] cdrom_release+0x104/0x250
[ 165.135005] [] sr_block_release+0x24/0x40
[ 165.135008] [] __blkdev_put+0x146/0x150
[ 165.135012] [] blkdev_put+0xa/0x10
[ 165.135015] [] blkdev_close+0x32/0x40
[ 165.135018] [] __fput+0xb6/0x180
[ 165.135021] [] fput+0x19/0x20
[ 165.135024] [] filp_close+0x47/0x80
[ 165.135027] [] sys_close+0x66/0xc0
[ 165.135030] [] syscall_call+0x7/0xb
[ 165.135032] ===
[ 166.564998] WARNING: at mm/slub.c:1573 kmem_cache_alloc()
[ 166.565006] [] show_trace_log_lvl+0x1a/0x30
[ 166.565013] [] show_trace+0x12/0x20
[ 166.565016] [] dump_stack+0x16/0x20
[ 166.565020] [] kmem_cache_alloc+0xe3/0x110
[ 166.565030] [] mempool_alloc_slab+0xe/0x10
[ 166.565039] [] mempool_alloc+0x31/0xf0
[ 166.565047] [] bio_alloc_bioset+0x1f/0x140
[ 166.565057] [] bio_alloc+0xe/0x20
[ 166.565066] [] submit_bh+0x63/0x100
[ 166.565075] [] journal_do_submit_data+0x28/0x40
[ 166.565085] [] journal_commit_transaction+0x658/0x1290
[ 166.565095] [] kjournald+0xb2/0x1e0
[ 166.565103] [] kthread+0x42/0x70
[ 166.565112] [] 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
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
On 02/08/07, Andrew Morton [EMAIL PROTECTED] 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]
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Fri, 3 Aug 2007 00:53:44 +0200
Jesper Juhl [EMAIL PROTECTED] wrote:
On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
On 02/08/07, Andrew Morton [EMAIL PROTECTED] 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().
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On 03/08/07, Andrew Morton [EMAIL PROTECTED] wrote:
On Fri, 3 Aug 2007 00:53:44 +0200
Jesper Juhl [EMAIL PROTECTED] wrote:
On Thursday 02 August 2007 10:20:47 Jesper Juhl wrote:
On 02/08/07, Andrew Morton [EMAIL PROTECTED] 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 [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-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On Fri, 3 Aug 2007 01:10:02 +0200 Jesper Juhl [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
On 03/08/07, Andrew Morton [EMAIL PROTECTED] wrote: On Fri, 3 Aug 2007 01:10:02 +0200 Jesper Juhl [EMAIL PROTECTED] 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 [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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Fix two potential mem leaks in MPT Fusion (mpt_attach())
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)0x1u) /* 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++) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[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-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
