Bug in kmem_cache_create with duplicate names

Bug in kmem_cache_create with duplicate names

[Steven Rostedt]

Is it really necessary to BUG on creating a cache with a duplicate name?
Wouldn't it just be better to fail the create. The reason I mentioned
this is that I was writing some modules and after doing a cut and paste,
I forgot to change a name of a cache that was created by one module and
I used it in another existing module.  So you can say that it was indeed
a bug, but did it really need to crash my machine?  I aways check the
return codes in my modules, and I would have figured it out why it
failed, but I didn't expect a simple module to crash the machine the way
it did.  Well anyway it did definitely show me where my bug was.

Thanks,

-- Steve

Re: Bug in kmem_cache_create with duplicate names

[Randy.Dunlap]

Steven Rostedt wrote:
> Is it really necessary to BUG on creating a cache with a duplicate name?
> Wouldn't it just be better to fail the create. The reason I mentioned
> this is that I was writing some modules and after doing a cut and paste,
> I forgot to change a name of a cache that was created by one module and
> I used it in another existing module.  So you can say that it was indeed
> a bug, but did it really need to crash my machine?  I aways check the
> return codes in my modules, and I would have figured it out why it
> failed, but I didn't expect a simple module to crash the machine the way
> it did.  Well anyway it did definitely show me where my bug was.

Yes, it does that.

However, I agree with you.  I don't see a good reason for it.

Duplicate name can just return NULL.  NOTE:  Such a change most
likely requires an audit of all callers of kmem_cache_create()
to be sure that they check its return value.  There's a gcc
attribute that can be added to the function prototype to
warn if the function is called without looking at its
return value, although just doing
	x = kmem_cache_create(...);
and ignoring x probably evades the warning.


include/linux/compiler-gcc+.h:
#define __must_check 		__attribute__((warn_unused_result))

-- 
~Randy

Re: Bug in kmem_cache_create with duplicate names

[Arjan van de Ven]

On Tue, 2004-12-07 at 07:33 -0800, Randy.Dunlap wrote:
> Steven Rostedt wrote:
> > Is it really necessary to BUG on creating a cache with a duplicate name?
> > Wouldn't it just be better to fail the create. The reason I mentioned
> > this is that I was writing some modules and after doing a cut and paste,
> > I forgot to change a name of a cache that was created by one module and
> > I used it in another existing module.  So you can say that it was indeed
> > a bug, but did it really need to crash my machine?  I aways check the
> > return codes in my modules, and I would have figured it out why it
> > failed, but I didn't expect a simple module to crash the machine the way
> > it did.  Well anyway it did definitely show me where my bug was.
> 
> Yes, it does that.
> 
> However, I agree with you.  I don't see a good reason for it.

I do...
because if the registration gives success..... then you unregister it
later during module unload and the INITIAL user goes bang.
It's a bad bug. Don't do it. Fix your code ;)

Re: Bug in kmem_cache_create with duplicate names

[Randy.Dunlap]

Arjan van de Ven wrote:
> On Tue, 2004-12-07 at 07:33 -0800, Randy.Dunlap wrote:
> 
>>Steven Rostedt wrote:
>>
>>>Is it really necessary to BUG on creating a cache with a duplicate name?
>>>Wouldn't it just be better to fail the create. The reason I mentioned
>>>this is that I was writing some modules and after doing a cut and paste,
>>>I forgot to change a name of a cache that was created by one module and
>>>I used it in another existing module.  So you can say that it was indeed
>>>a bug, but did it really need to crash my machine?  I aways check the
>>>return codes in my modules, and I would have figured it out why it
>>>failed, but I didn't expect a simple module to crash the machine the way
>>>it did.  Well anyway it did definitely show me where my bug was.
>>
>>Yes, it does that.
>>
>>However, I agree with you.  I don't see a good reason for it.
> 
> 
> I do...
> because if the registration gives success..... then you unregister it
> later during module unload and the INITIAL user goes bang.
> It's a bad bug. Don't do it. Fix your code ;)

Who said anything about the (second) registration giving success?
and how is it destroyed without a valid ptr to it?

-- 
~Randy

Re: Bug in kmem_cache_create with duplicate names

[Steven Rostedt]

On Tue, 2004-12-07 at 11:15 -0500, Arjan van de Ven wrote:

> > However, I agree with you.  I don't see a good reason for it.
> 
> I do...
> because if the registration gives success..... then you unregister it
> later during module unload and the INITIAL user goes bang.
> It's a bad bug. Don't do it. Fix your code ;)
> 

Your module should fail to load if you can't register a cache. If you
are a good boy and check your return codes from the kmem_cache_create,
you would know that the cache failed and not load the module.
Otherwise, if it failed for other reasons, then you can be causing bugs
later when you go to use it. 

Now this raises the issue of name space, this will bug if two modules
use the same cache name. If this happens with two different vendors,
than the poor user will have to figure out who to blame.

-- Steve

(Arjan, Sorry for the duplicate, I forgot to do a reply all)

Re: Bug in kmem_cache_create with duplicate names

[Peter W. Morreale]

Steven Rostedt wrote:

>On Tue, 2004-12-07 at 11:15 -0500, Arjan van de Ven wrote:
>
>  
>
>>>However, I agree with you.  I don't see a good reason for it.
>>>      
>>>
>>I do...
>>because if the registration gives success..... then you unregister it
>>later during module unload and the INITIAL user goes bang.
>>It's a bad bug. Don't do it. Fix your code ;)
>>
>>    
>>
>
>Your module should fail to load if you can't register a cache. If you
>are a good boy and check your return codes from the kmem_cache_create,
>you would know that the cache failed and not load the module.
>Otherwise, if it failed for other reasons, then you can be causing bugs
>later when you go to use it. 
>

This would preclude the use of a dynamic cache, not all initializations 
are performed during module
insertion.    It also breaks since there is no relationship between the 
size of the objects in the cache
and the cache name (which is causing the BUG).  

This BUG specifically means that you (or somebody else) allocated 
something and did not free it.  

That is broken.  FYC (Fix Yer Code ;-) is the answer.

>
>Now this raises the issue of name space, this will bug if two modules
>use the same cache name. If this happens with two different vendors,
>than the poor user will have to figure out who to blame.
>

No different than any other global namespace issue.

-PWM

-- 
Peter W. Morreale                            email: morreale@radiantdata.com
Director of Engineering                      Niwot, Colorado, USA
Radiant Data Corporation                     voice: (303) 652-0870 x108 
-----------------------------------------------------------------------------
This transmission may contain information that is privileged, confidential
and/or exempt from disclosure under applicable law. If you are not the
intended recipient, you are hereby notified that any disclosure, copying,
distribution, or use of the information contained herein (including any
reliance thereon) is STRICTLY PROHIBITED. If you received this transmission
in error, please immediately contact the sender and destroy the material in
its entirety, whether in electronic or hard copy format. Thank you.

Re: Bug in kmem_cache_create with duplicate names

[Steven Rostedt]

On Tue, 2004-12-07 at 10:57 -0700, Peter W. Morreale wrote:

> >
> >Now this raises the issue of name space, this will bug if two modules
> >use the same cache name. If this happens with two different vendors,
> >than the poor user will have to figure out who to blame.
> >
> 
> No different than any other global namespace issue.

I beg to differ. If I have two modules that export the same name, do I
get a bug when I load the second module? No. Actually, I just tried it
and this raises another issue. There is no test to see if there is a
conflict of name spaces. Here's what I did,  I made two modules that
have a function named "abc" and exported them. The third module calls
function "abc".  The result was that both mod1 and mod2 were loaded with
no problem, and mod3 called mod2's abc.  

Below are the simple modules that did this test. Should this be a
problem, or issue? It has a little bit of a polymorphism effect. If I
unload mod3 and then mod2, then reload mod3, it calls mod1's abc (as
expected).  If I unload mod3 again, reload mod2, then reload mod3, it
calls mod2's abc again. 

Well, anyway, I don't think that the kernel should crash due to a
namespace problem with caches. But that's just my opinion. And for all
of you that were so concerned... Yes I did fix my code ;-)



mod1.c:
---------------------------------
#include <linux/config.h>
#include <linux/module.h>
#include <linux/init.h>

void abc(void)
{
	printk("hello from mod1\n");
}

EXPORT_SYMBOL(abc);

static int __init mod1_init(void)
{
	printk("loaded mod1\n");
	return 0;
}

static void __exit mod1_exit(void)
{
	printk("unloaded mod1\n");
}

module_init(mod1_init);
module_exit(mod1_exit);

MODULE_LICENSE("GPL");
----------------------------------------


mod2.c:
----------------------------------------
#include <linux/config.h>
#include <linux/module.h>
#include <linux/init.h>

void abc(void)
{
	printk("hello from mod2\n");
}

EXPORT_SYMBOL(abc);

static int __init mod2_init(void)
{
	printk("loaded mod2\n");
	return 0;
}

static void __exit mod2_exit(void)
{
	printk("unloaded mod2\n");
}

module_init(mod2_init);
module_exit(mod2_exit);

MODULE_LICENSE("GPL");
-------------------------------

mod3.c:
-------------------------------
#include <linux/config.h>
#include <linux/module.h>
#include <linux/init.h>

extern void abc(void);

static int __init mod3_init(void)
{
	printk("loaded mod3\n");

	printk("running abc\n");
	abc();

	return 0;
}

static void __exit mod3_exit(void)
{
	printk("unloaded mod3\n");
}

module_init(mod3_init);
module_exit(mod3_exit);

MODULE_LICENSE("GPL");
-------------------------------------

Re: Bug in kmem_cache_create with duplicate names

[Steven Rostedt]

On Tue, 2004-12-07 at 07:33 -0800, Randy.Dunlap wrote:

> Duplicate name can just return NULL.  NOTE:  Such a change most
> likely requires an audit of all callers of kmem_cache_create()
> to be sure that they check its return value.  There's a gcc
> attribute that can be added to the function prototype to
> warn if the function is called without looking at its
> return value, although just doing
> 	x = kmem_cache_create(...);
> and ignoring x probably evades the warning.
> 

I would hope that the kernel does check the return, since it still can
fail for other reasons an return a NULL, and it is more likely to fail
for other reasons, since this will already BUG on the duplicate case.
But your suggestion is probably a good one, and I'm sure there are
probably other calls that should have that same check.

-- Steve