This patch is the next logical step after the following two threads
http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.2/2505.html
http://www.ussg.iu.edu/hypermail/linux/kernel/0511.3/0036.html
When a symbol is exported from the kernel, and say, a module would
export the same symbol, there currently exists no mechanism to prevent
the module from exporting this symbol. The module would still go ahead
and export the symbol, the symbol table would now contain two copies
of the exported symbol, and hell would break loose.
This patch prevents that from happening, by checking the symbol table
before relocation for all occurences of the Exported Symbol. If the
symbol already exists, we branch out with -ENOEXEC. Currently, this
search is sequential.
Signed-off-by: Ashutosh Naik <[email protected]>
Signed-off-by: Anand Krishnan <[email protected]>
Updating the correct email id of Anand Krishnan
Signed-off-by: Anand Krishnan <[email protected]>
On 12/12/05, Ashutosh Naik <[email protected]> wrote:
> This patch is the next logical step after the following two threads
>
> http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.2/2505.html
> http://www.ussg.iu.edu/hypermail/linux/kernel/0511.3/0036.html
>
> When a symbol is exported from the kernel, and say, a module would
> export the same symbol, there currently exists no mechanism to prevent
> the module from exporting this symbol. The module would still go ahead
> and export the symbol, the symbol table would now contain two copies
> of the exported symbol, and hell would break loose.
>
> This patch prevents that from happening, by checking the symbol table
> before relocation for all occurences of the Exported Symbol. If the
> symbol already exists, we branch out with -ENOEXEC. Currently, this
> search is sequential.
>
>
> Signed-off-by: Ashutosh Naik <[email protected]>
> Signed-off-by: Anand Krishnan <[email protected]>
>
>
>
Ashutosh Naik <[email protected]> wrote:
>
> When a symbol is exported from the kernel, and say, a module would
> export the same symbol, there currently exists no mechanism to prevent
> the module from exporting this symbol. The module would still go ahead
> and export the symbol, the symbol table would now contain two copies
> of the exported symbol, and hell would break loose.
>
> This patch prevents that from happening, by checking the symbol table
> before relocation for all occurences of the Exported Symbol. If the
> symbol already exists, we branch out with -ENOEXEC. Currently, this
> search is sequential.
Do we really need to do this at runtime? We could check this when building
module depoendencies (for example). That'll be a 95% solution..
On Mon, Dec 12, 2005 at 11:13:22AM -0800, Andrew Morton wrote:
> Do we really need to do this at runtime?
Probably. One could consider this a security hole...
r~
On Mon, Dec 12, 2005 at 11:27:46AM -0800, Richard Henderson wrote:
> On Mon, Dec 12, 2005 at 11:13:22AM -0800, Andrew Morton wrote:
> > Do we really need to do this at runtime?
>
> Probably. One could consider this a security hole...
Huh? You are root and loading a kernel module. You can do much worse
things at this point in time than messing around with existing symbols
:)
I think it should be a build-time thing if possible.
thanks,
greg k-h
On 12/12/05, Greg KH <[email protected]> wrote:
> On Mon, Dec 12, 2005 at 11:27:46AM -0800, Richard Henderson wrote:
> > On Mon, Dec 12, 2005 at 11:13:22AM -0800, Andrew Morton wrote:
> > > Do we really need to do this at runtime?
> >
> > Probably. One could consider this a security hole...
>
> Huh? You are root and loading a kernel module. You can do much worse
> things at this point in time than messing around with existing symbols
> :)
>
Sure, but having insmod <foo> crash the kernel is bad.
In my oppinion the kernel should be robust against accidental loading
of the wrong module. If the symbols of the module clashes with
in-kernel symbols, the logical thing is to reject the module load.
Sure you can do a lot worse things as root, but being able to cause a
crash with a standard tool such as insmod is pretty nasty and
something you can easily get into by accident. You can't prevent root
from purposely screwing with the kernel, but accidental mis-loading of
a wrong module shouldn't cause a crash.
> I think it should be a build-time thing if possible.
>
If there's a way to revent it 100% at build time I'd agree, but if not
I'd say a runtime solution is required.
Just my 0.02 euro.
--
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
On Mon, 2005-12-12 at 18:09 +0530, Ashutosh Naik wrote:
> diff -Naurp linux-2.6.15-rc5-vanilla/kernel/module.c linux-2.6.15-rc5-mod/kernel/module.c
> --- linux-2.6.15-rc5-vanilla/kernel/module.c 2005-12-07 19:32:23.000000000 +0530
> +++ linux-2.6.15-rc5-mod/kernel/module.c 2005-12-12 17:47:28.000000000 +0530
> @@ -1204,6 +1204,63 @@ void *__symbol_get(const char *symbol)
> }
> EXPORT_SYMBOL_GPL(__symbol_get);
>
> +/*
> + * Ensure that an exported symbol [global namespace] does not already exist
> + * in the Kernel or in some other modules exported symbol table.
> + */
> +static int verify_export_symbols(Elf_Shdr *sechdrs,
> + const char *strtab,
> + struct module *mod)
> +{
> + struct kernel_symbol *exportsym, *gplsym;
> + unsigned long i,ret=0,value=0;
> + struct module *owner;
> + const unsigned long *crc;
> + unsigned long index=0;
> +
> + spin_lock_irq(&modlist_lock);
> +
> + exportsym = (struct kernel_symbol *)mod->syms;
> + gplsym = (struct kernel_symbol *)mod->gpl_syms;
> +
> + if (exportsym)
> + for (i = 0; i < mod->num_syms; exportsym++,i++) {
Hi,
The check for exportsym not being NULL is redundant, since
mod->num_syms will be 0 in that case. The cast is also redundant. You
have two identical failure cases at the bottom. And your use of index
is convoluted: do it after relocations.
How about something like:
const struct kernel_symbol *sym;
unsigned int i;
const unsigned long *crc;
struct module *owner;
spin_lock_irq(&modlist_lock);
for (i = 0; i < mod->num_syms; i++)
if (__find_symbol(mod->syms[i].name, &owner, &crc, 1))
goto dup;
for (i = 0; i < num->num_gpl_syms; i++)
if (__find_symbol(mod->gpl_syms[i].name,&owner,&crc,1))
goto dup;
spin_unlock_irq(&modlist_lock);
return 0;
dup:
printk("%s: exports duplicate symbol (owned by %s)\n",
mod->name, module_name(owner));
return -ENOEXEC;
}
Cheers,
Rusty.
--
ccontrol: http://ozlabs.org/~rusty/ccontrol
On 12/12/05, Ashutosh Naik <[email protected]> wrote:
> Updating the correct email id of Anand Krishnan
> Signed-off-by: Anand Krishnan <[email protected]>
>
> On 12/12/05, Ashutosh Naik <[email protected]> wrote:
> > This patch is the next logical step after the following two threads
> >
> > http://www.uwsg.iu.edu/hypermail/linux/kernel/0511.2/2505.html
> > http://www.ussg.iu.edu/hypermail/linux/kernel/0511.3/0036.html
> >
> > When a symbol is exported from the kernel, and say, a module would
> > export the same symbol, there currently exists no mechanism to prevent
> > the module from exporting this symbol. The module would still go ahead
> > and export the symbol, the symbol table would now contain two copies
> > of the exported symbol, and hell would break loose.
> >
> > This patch prevents that from happening, by checking the symbol table
> > before relocation for all occurences of the Exported Symbol. If the
> > symbol already exists, we branch out with -ENOEXEC. Currently, this
> > search is sequential.
> >
> >
> > Signed-off-by: Ashutosh Naik <[email protected]>
> > Signed-off-by: Anand Krishnan <[email protected]>
> >
<grumble>it would be a lot easier to comment on patches if you
included them inline instead of as attachments</grumble>
>
> diff -Naurp linux-2.6.15-rc5-vanilla/kernel/module.c linux-2.6.15-rc5-mod/kernel/module.c
> --- linux-2.6.15-rc5-vanilla/kernel/module.c 2005-12-07 19:32:23.000000000 +0530
> +++ linux-2.6.15-rc5-mod/kernel/module.c 2005-12-12 17:47:28.000000000 +0530
> @@ -1204,6 +1204,63 @@ void *__symbol_get(const char *symbol)
> }
> EXPORT_SYMBOL_GPL(__symbol_get);
>
> +/*
> + * Ensure that an exported symbol [global namespace] does not already exist
> + * in the Kernel or in some other modules exported symbol table.
> + */
> +static int verify_export_symbols(Elf_Shdr *sechdrs,
> + const char *strtab,
> + struct module *mod)
> +{
> + struct kernel_symbol *exportsym, *gplsym;
> + unsigned long i,ret=0,value=0;
spaces after "," and before/after "=" please :
unsigned long i, ret = 0, value = 0;
> + struct module *owner;
> + const unsigned long *crc;
> + unsigned long index=0;
unsigned long index = 0;
> +
> + spin_lock_irq(&modlist_lock);
> +
I'm wondering, doesn't this take quite a long time? Too long to hold
a spinlock for?
Of course we need locking to prevent other module loads to modify the
symbol table while we are checking this one, but to prevent the kernel
from stalling everything else for a long time, wouldn't it be better
to use a semaphore (we can sleep with those - right?) and an explicit
schedule() call in the loop(s)? Or am I completely out in outere
space with that thought?
> + exportsym = (struct kernel_symbol *)mod->syms;
> + gplsym = (struct kernel_symbol *)mod->gpl_syms;
> +
> + if (exportsym)
> + for (i = 0; i < mod->num_syms; exportsym++,i++) {
> + index = (unsigned long)(exportsym->name);
> + if (exportsym->name) {
> + value = __find_symbol(strtab + index, &owner, &crc,1);
> +
> + if (value != 0)
if (unlikely(value)) ?????
> + goto duplicate_sym;
> + }
> + }
> +
> + if (gplsym)
> + for (i = 0; i < mod->num_gpl_syms; gplsym++,i++) {
> + index = (unsigned long)(gplsym->name);
> + if (gplsym->name) {
> + value = __find_symbol(strtab + index, &owner, &crc,1);
> +
> + if (value != 0)
if (unlikely(value)) ?????
> + goto duplicate_gpl_sym;
> + }
> + }
> +
> + spin_unlock_irq(&modlist_lock);
> + /*Done*/
> + return ret;
> +
> +duplicate_sym:
> + spin_unlock_irq(&modlist_lock);
> + printk("%s: Duplicate Exported Symbol found in %s\n",
Shouldn't this printk() be using KERN_ERROR ?
printk(KERN_ERROR "%s: Duplicate Exported Symbol found in %s\n",
> + strtab + index, mod->name);
> + return -ENOEXEC;
> +duplicate_gpl_sym:
> + spin_unlock_irq(&modlist_lock);
> + printk("%s: Duplicate Exported Symbol found in %s\n",
> + strtab + index, mod->name);
> + return -ENOEXEC;
> +}
Why 3 different exit paths? and 2 of them are even identical. Why not
something like this instead? :
{
...
if (unlikely(value) {
ret = -ENOEXEC;
goto out;
}
...
out:
spin_unlock_irq();
if (ret)
printk();
return ret;
}
> +
> /* Change all symbols so that sh_value encodes the pointer directly. */
> static int simplify_symbols(Elf_Shdr *sechdrs,
> unsigned int symindex,
> @@ -1502,10 +1559,10 @@ static struct module *load_module(void _
> {
> Elf_Ehdr *hdr;
> Elf_Shdr *sechdrs;
> - char *secstrings, *args, *modmagic, *strtab = NULL;
> + char *secstrings, *args, *modmagic, *strtab = NULL, *exportstrtab = NULL;
> unsigned int i, symindex = 0, strindex = 0, setupindex, exindex,
> - exportindex, modindex, obsparmindex, infoindex, gplindex,
> - crcindex, gplcrcindex, versindex, pcpuindex;
> + exportindex, exportstringindex, modindex, obsparmindex, infoindex,
> + gplindex, crcindex, gplcrcindex, versindex, pcpuindex;
> long arglen;
> struct module *mod;
> long err = 0;
> @@ -1585,6 +1642,7 @@ static struct module *load_module(void _
>
> /* Optional sections */
> exportindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab");
> + exportstringindex = find_sec(hdr,sechdrs, secstrings, "__ksymtab_strings");
> gplindex = find_sec(hdr, sechdrs, secstrings, "__ksymtab_gpl");
> crcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab");
> gplcrcindex = find_sec(hdr, sechdrs, secstrings, "__kcrctab_gpl");
> @@ -1736,6 +1794,13 @@ static struct module *load_module(void _
> if (gplcrcindex)
> mod->gpl_crcs = (void *)sechdrs[gplcrcindex].sh_addr;
>
> + /* Find duplicate symbols */
> + exportstrtab = (void *)sechdrs[exportstringindex].sh_addr;
> + err = verify_export_symbols(sechdrs, exportstrtab, mod);
> +
> + if (err < 0)
> + goto cleanup;
> +
> #ifdef CONFIG_MODVERSIONS
> if ((mod->num_syms && !crcindex) ||
> (mod->num_gpl_syms && !gplcrcindex)) {
>
>
--
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
On Llu, 2005-12-12 at 11:13 -0800, Andrew Morton wrote:
> Do we really need to do this at runtime? We could check this when building
> module depoendencies (for example). That'll be a 95% solution..
Its almost the 0% solution. The kernel as shipped doesn't seem to have
any clashing symbols like this. The two sets of cases people report are
1. Out of tree modules
2. Reconfiguring, rebuilding something from kernel to module and not
cleaning up
A dep time solution might fix one of those but robustness here would be
good, especially as once the installation is incorrect end users can
often trigger hotplug loads that cause problems.
On Mon, 2005-12-12 at 22:48 +0000, Alan Cox wrote:
> On Llu, 2005-12-12 at 11:13 -0800, Andrew Morton wrote:
> > Do we really need to do this at runtime? We could check this when building
> > module depoendencies (for example). That'll be a 95% solution..
>
> Its almost the 0% solution. The kernel as shipped doesn't seem to have
> any clashing symbols like this. The two sets of cases people report are
>
> 1. Out of tree modules
these need to (re)run depmod anyway, and they do in general
> 2. Reconfiguring, rebuilding something from kernel to module and not
> cleaning up
which runs depmod as well
Hi,
> I'm wondering, doesn't this take quite a long time?
> Too long to hold
> a spinlock for?
>
> Of course we need locking to prevent other module
> loads to modify the
> symbol table while we are checking this one, but to
> prevent the kernel
> from stalling everything else for a long time,
> wouldn't it be better
> to use a semaphore (we can sleep with those -
> right?) and an explicit
> schedule() call in the loop(s)? Or am I completely
> out in outere
> space with that thought?
>
Both at the time of loading a module and while
unloading a
module module_mutex is acquired. In the first place we
wer
e not planning to take the spinlock at all. But saw
that r
esolve_symbol does that and hence wasn't sure whether
the
lock should be acquired.
> Shouldn't this printk() be using KERN_ERROR ?
>
> printk(KERN_ERROR "%s: Duplicate Exported Symbol
> found in %s\n",
>
>
> > + strtab + index, mod->name);
> > + return -ENOEXEC;
> > +duplicate_gpl_sym:
> > + spin_unlock_irq(&modlist_lock);
> > + printk("%s: Duplicate Exported Symbol found in
> %s\n",
> > + strtab + index, mod->name);
> > + return -ENOEXEC;
> > +}
>
> Why 3 different exit paths? and 2 of them are even
> identical. Why not
> something like this instead? :
>
> {
> ...
> if (unlikely(value) {
> ret = -ENOEXEC;
> goto out;
> }
> ...
> out:
> spin_unlock_irq();
> if (ret)
> printk();
> return ret;
> }
>
We will send an updated patch with the modifications.
Thanks,
Anand
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
On 12/13/05, Rusty Russell <[email protected]> wrote:
>
> How about something like:
>
> const struct kernel_symbol *sym;
> unsigned int i;
> const unsigned long *crc;
> struct module *owner;
>
> spin_lock_irq(&modlist_lock);
> for (i = 0; i < mod->num_syms; i++)
> if (__find_symbol(mod->syms[i].name, &owner, &crc, 1))
> goto dup;
> for (i = 0; i < num->num_gpl_syms; i++)
> if (__find_symbol(mod->gpl_syms[i].name,&owner,&crc,1))
> goto dup;
> spin_unlock_irq(&modlist_lock);
> return 0;
> dup:
> printk("%s: exports duplicate symbol (owned by %s)\n",
> mod->name, module_name(owner));
> return -ENOEXEC;
> }
Have tried that in the attached patch. However, mod->syms[i].name
would be valid only after a long relocation for loop has run through.
While this adds a wee bit extra overhead, that overhead is only in the
case where the module does actually export a Duplicate Symbol.
This its a question, whether we do the search before relocation ( A
little messier ) or after ( More straight forward)
Regards
Ashutosh
On 12/13/05, Alan Cox <[email protected]> wrote:
> Its almost the 0% solution. The kernel as shipped doesn't seem to have
> any clashing symbols like this. The two sets of cases people report are
>
> 1. Out of tree modules
> 2. Reconfiguring, rebuilding something from kernel to module and not
> cleaning up
>
> A dep time solution might fix one of those but robustness here would be
> good, especially as once the installation is incorrect end users can
> often trigger hotplug loads that cause problems.
I agree with this.
I also would like to add that, the exported symbol may not always be
in the same module. Imagine if Module A is loaded and Module B would
export one symbol with the same symbol name as a symbol in Module A,
then the symbol exported by Module B would still go through. Now
Imagine if that symbol does something like a kmem_cache_create of an
existing cache!!
I feel this is a security loophole and preventing duplicate *exported*
symbols in the kernel, might just solve it.
Regards
Ashutosh
On 12/13/05, Ashutosh Naik <[email protected]> wrote:
> On 12/13/05, Rusty Russell <[email protected]> wrote:
> >
> > How about something like:
> >
> > const struct kernel_symbol *sym;
> > unsigned int i;
> > const unsigned long *crc;
> > struct module *owner;
> >
> > spin_lock_irq(&modlist_lock);
> > for (i = 0; i < mod->num_syms; i++)
> > if (__find_symbol(mod->syms[i].name, &owner, &crc, 1))
> > goto dup;
> > for (i = 0; i < num->num_gpl_syms; i++)
> > if (__find_symbol(mod->gpl_syms[i].name,&owner,&crc,1))
> > goto dup;
> > spin_unlock_irq(&modlist_lock);
> > return 0;
> > dup:
> > printk("%s: exports duplicate symbol (owned by %s)\n",
> > mod->name, module_name(owner));
> > return -ENOEXEC;
> > }
>
> Have tried that in the attached patch. However, mod->syms[i].name
> would be valid only after a long relocation for loop has run through.
> While this adds a wee bit extra overhead, that overhead is only in the
> case where the module does actually export a Duplicate Symbol.
Forgot to add
Signed-off-by: Ashutosh Naik <[email protected]>
Signed-off-by: Anand Krishnan <[email protected]>
Cheers
On 12/13/05, Ashutosh Naik <[email protected]> wrote:
> On 12/13/05, Rusty Russell <[email protected]> wrote:
> >
> > How about something like:
> >
[snip imrovement suggestion]
>
> Have tried that in the attached patch. However, mod->syms[i].name
> would be valid only after a long relocation for loop has run through.
> While this adds a wee bit extra overhead, that overhead is only in the
> case where the module does actually export a Duplicate Symbol.
>
> This its a question, whether we do the search before relocation ( A
> little messier ) or after ( More straight forward)
>
hmm, patch still attached instead of inlined... :-(
Anyway, a few minor comments below.
>
> --- /usr/src/linux-2.6.15-rc5-vanilla/kernel/module.c 2005-12-07 19:32:23.000000000 +0530
> +++ /usr/src/linux-2.6.15-rc5/kernel/module.c 2005-12-13 19:44:43.000000000 +0530
> @@ -1204,6 +1204,42 @@ void *__symbol_get(const char *symbol)
> }
> EXPORT_SYMBOL_GPL(__symbol_get);
>
> +/*
> + * Ensure that an exported symbol [global namespace] does not already exist
> + * in the Kernel or in some other modules exported symbol table.
> + */
> +static int verify_export_symbols(struct module *mod)
> +{
> + const char *name=0;
CodingStyle issue :
const char *name = 0;
> + unsigned long i, ret = 0;
> + struct module *owner;
> + const unsigned long *crc;
> +
> + spin_lock_irq(&modlist_lock);
> + for (i = 0; i < mod->num_syms; i++)
> + if (unlikely(__find_symbol(mod->syms[i].name, &owner, &crc,1))) {
CodingStyle issue :
if (unlikely(__find_symbol(mod->syms[i].name, &owner, &crc, 1))) {
> + name = mod->syms[i].name;
> + ret = -ENOEXEC;
> + goto dup;
> + }
> +
> + for (i = 0; i < mod->num_gpl_syms; i++)
> + if (unlikely(__find_symbol(mod->gpl_syms[i].name, &owner, &crc,1))) {
CodingStyle issue :
if (unlikely(__find_symbol(mod->gpl_syms[i].name, &owner, &crc, 1))) {
> + name = mod->gpl_syms[i].name;
> + ret = -ENOEXEC;
> + goto dup;
> + }
> +
> +dup:
> + spin_unlock_irq(&modlist_lock);
> +
> + if (ret)
> + printk("%s: exports duplicate symbol %s (owned by %s)\n",
I still think this should be printk(KERN_ERROR ...) and not just a
warning, since the loading of the module will fail completely. Others
may disagree ofcourse, but that's my oppinion.
> + mod->name, name, module_name(owner));
> +
> + return ret;
> +}
> +
> /* Change all symbols so that sh_value encodes the pointer directly. */
> static int simplify_symbols(Elf_Shdr *sechdrs,
> unsigned int symindex,
> @@ -1767,6 +1804,12 @@ static struct module *load_module(void _
> goto cleanup;
> }
>
> + /* Find duplicate symbols */
> + err = verify_export_symbols(mod);
> +
> + if (err < 0)
> + goto cleanup;
> +
> /* Set up and sort exception table */
> mod->num_exentries = sechdrs[exindex].sh_size / sizeof(*mod->extable);
> mod->extable = extable = (void *)sechdrs[exindex].sh_addr;
>
A few general things:
I still worry a bit about the spinlock hold time, especially since you
are doing two linear searches through what could potentially be a
*lot* of symbols.. It may not be a problem (do you have any time
measurements?), but it still seems to me that using a lock type that
allows you to sleep + a call to schedule() would be a good thing for
those loops.
Also, what about the softlockup watchdog? Do we risk falsly triggering
that? Would a call to touch_softlockup_watchdog() between the two
loops, or maybe even inside each loop, be a good idea? again, depends
on how long this all takes.
All in all, looks a lot better to me than the previous version.
--
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
On Tue, 2005-12-13 at 17:49 +0100, Jesper Juhl wrote:
> On 12/13/05, Ashutosh Naik <[email protected]> wrote:
> > On 12/13/05, Rusty Russell <[email protected]> wrote:
> > >
> > > How about something like:
> > >
> [snip imrovement suggestion]
> >
> > Have tried that in the attached patch. However, mod->syms[i].name
> > would be valid only after a long relocation for loop has run through.
> > While this adds a wee bit extra overhead, that overhead is only in the
> > case where the module does actually export a Duplicate Symbol.
> >
> > This its a question, whether we do the search before relocation ( A
> > little messier ) or after ( More straight forward)
Hi Ashutosh, Jasper,
Patch looks good! A few nits still:
> > +static int verify_export_symbols(struct module *mod)
> > +{
> > + const char *name=0;
>
> CodingStyle issue :
> const char *name = 0;
More importantly:
const char *name = NULL; /* GCC 4.0 warns */
(I assume that's why you have the useless initialization).
> > + spin_lock_irq(&modlist_lock);
> > + for (i = 0; i < mod->num_syms; i++)
> > + if (unlikely(__find_symbol(mod->syms[i].name, &owner, &crc,1))) {
>
> CodingStyle issue :
> if (unlikely(__find_symbol(mod->syms[i].name, &owner, &crc, 1))) {
I would discard the unlikely() here; it's a completely wasted
micro-optimization in this context
> > + if (ret)
> > + printk("%s: exports duplicate symbol %s (owned by %s)\n",
>
> I still think this should be printk(KERN_ERROR ...) and not just a
> warning, since the loading of the module will fail completely. Others
> may disagree ofcourse, but that's my oppinion.
I agree, KERN_ERR is appropriate here.
> I still worry a bit about the spinlock hold time, especially since you
> are doing two linear searches through what could potentially be a
> *lot* of symbols.. It may not be a problem (do you have any time
> measurements?), but it still seems to me that using a lock type that
> allows you to sleep + a call to schedule() would be a good thing for
> those loops.
We already do this to resolve (more) symbols, so I don't see it as a
problem. However, I believe that lock is redundant here: we need both
locks to write the list, but either is sufficient for reading, and we
already hold the sem.
Cheers,
Rusty.
--
ccontrol: http://ozlabs.org/~rusty/ccontrol
On 12/14/05, Rusty Russell <[email protected]> wrote:
> Patch looks good! A few nits still:
Have resolved all the nits ( hopefully :) )
> We already do this to resolve (more) symbols, so I don't see it as a
> problem. However, I believe that lock is redundant here: we need both
> locks to write the list, but either is sufficient for reading, and we
> already hold the sem.
Ya, the lock is redundant here, as we are already inside a semaphore.
Signed-off-by: Ashutosh Naik <[email protected]>
Signed-off-by: Anand Krishnan <[email protected]>
--- linux-2.6.15-rc5/kernel/module.c.orig 2005-12-14
09:27:53.000000000 +0530
+++ linux-2.6.15-rc5/kernel/module.c 2005-12-14 09:18:31.000000000 +0530
@@ -1204,6 +1204,39 @@ void *__symbol_get(const char *symbol)
}
EXPORT_SYMBOL_GPL(__symbol_get);
+/*
+ * Ensure that an exported symbol [global namespace] does not already exist
+ * in the Kernel or in some other modules exported symbol table.
+ */
+static int verify_export_symbols(struct module *mod)
+{
+ const char *name = NULL;
+ unsigned long i, ret = 0;
+ struct module *owner;
+ const unsigned long *crc;
+
+ for (i = 0; i < mod->num_syms; i++)
+ if (!__find_symbol(mod->syms[i].name, &owner, &crc, 1)) {
+ name = mod->syms[i].name;
+ ret = -ENOEXEC;
+ goto dup;
+ }
+
+ for (i = 0; i < mod->num_gpl_syms; i++)
+ if (!__find_symbol(mod->gpl_syms[i].name, &owner, &crc, 1)) {
+ name = mod->gpl_syms[i].name;
+ ret = -ENOEXEC;
+ goto dup;
+ }
+
+dup:
+ if (ret)
+ printk(KERN_ERR "%s: exports duplicate symbol %s
(owned by %s)\n",
+ mod->name, name, module_name(owner));
+
+ return ret;
+}
+
/* Change all symbols so that sh_value encodes the pointer directly. */
static int simplify_symbols(Elf_Shdr *sechdrs,
unsigned int symindex,
@@ -1767,6 +1800,12 @@ static struct module *load_module(void _
goto cleanup;
}
+ /* Find duplicate symbols */
+ err = verify_export_symbols(mod);
+
+ if (err < 0)
+ goto cleanup;
+
/* Set up and sort exception table */
mod->num_exentries = sechdrs[exindex].sh_size / sizeof(*mod->extable);
mod->extable = extable = (void *)sechdrs[exindex].sh_addr;
My mail client is indeed broken.
Find the attached patch and here is the Changelog
This patch ensures that an exported symbol does not already exist in
the kernel or in some other module's exported symbol table. This is
done by checking the symbol tables for the exported symbol at the time
of loading the module. Currently this is done after the relocation of
the symbol.
Signed-off-by: Ashutosh Naik <[email protected]>
Signed-off-by: Anand Krishnan <[email protected]>
On 12/14/05, Rusty Russell <[email protected]> wrote:
> We already do this to resolve (more) symbols, so I don't see it as a
> problem. However, I believe that lock is redundant here: we need both
> locks to write the list, but either is sufficient for reading, and we
> already hold the sem.
Was just wondering, in that case, if we really need the spinlock in
resolve_symbol() function, where there exists a spinlock around the
__find_symbol() function
Cheers
Ashutosh
On Wed, 2005-12-14 at 11:16 +0530, Ashutosh Naik wrote:
> On 12/14/05, Rusty Russell <[email protected]> wrote:
> > We already do this to resolve (more) symbols, so I don't see it as a
> > problem. However, I believe that lock is redundant here: we need both
> > locks to write the list, but either is sufficient for reading, and we
> > already hold the sem.
>
> Was just wondering, in that case, if we really need the spinlock in
> resolve_symbol() function, where there exists a spinlock around the
> __find_symbol() function
Yes, I think that's redundant as well. We're not altering the module
list itself, so either of the two locks is sufficient, and we have the
semaphore.
Patch welcome!
Rusty.
--
ccontrol: http://ozlabs.org/~rusty/ccontrol
Ashutosh Naik <[email protected]> wrote:
>
> This patch ensures that an exported symbol does not already exist in
> the kernel or in some other module's exported symbol table. This is
> done by checking the symbol tables for the exported symbol at the time
> of loading the module. Currently this is done after the relocation of
> the symbol.
This patch causes weird things to happen on ppc64.
The only module which is loaded is autofs, and I assume this happens when
autofs is (successfully) loaded. But unloading autofs and reloading autofs
does not cause a repeat.
kjournald starting. Commit interval 5 seconds
EXT3 FS on sda6, internal journal
EXT3-fs: mounted filesystem with ordered data mode.
tg3: eth0: Link is up at 100 Mbps, full duplex.
tg3: eth0: Flow control is off for TX and off for RX.
sunrpc: exports duplicate symbol rpc_max_payload (owned by autofs)
lockd: Unknown symbol svc_recv
lockd: Unknown symbol svc_create
lockd: Unknown symbol rpciod_up
lockd: Unknown symbol rpc_destroy_client
lockd: Unknown symbol xdr_encode_netobj
lockd: Unknown symbol svc_destroy
lockd: Unknown symbol xprt_create_proto
lockd: Unknown symbol rpc_delay
lockd: Unknown symbol rpc_call_async
lockd: Unknown symbol rpc_create_client
lockd: Unknown symbol svc_makesock
lockd: Unknown symbol nlm_debug
lockd: Unknown symbol xdr_decode_netobj
lockd: Unknown symbol svc_wake_up
lockd: Unknown symbol rpciod_down
lockd: Unknown symbol svc_exit_thread
lockd: Unknown symbol xdr_encode_string
lockd: Unknown symbol rpc_call_sync
lockd: Unknown symbol xdr_decode_string_inplace
lockd: Unknown symbol svc_set_client
lockd: Unknown symbol svc_process
lockd: Unknown symbol xprt_set_timeout
lockd: Unknown symbol rpc_restart_call
lockd: Unknown symbol svc_create_thread
exportfs: exports duplicate symbol find_exported_dentry (owned by autofs)
sunrpc: exports duplicate symbol rpc_max_payload (owned by autofs)
lockd: Unknown symbol svc_recv
lockd: Unknown symbol svc_create
lockd: Unknown symbol rpciod_up
lockd: Unknown symbol rpc_destroy_client
lockd: Unknown symbol xdr_encode_netobj
lockd: Unknown symbol svc_destroy
lockd: Unknown symbol xprt_create_proto
lockd: Unknown symbol rpc_delay
lockd: Unknown symbol rpc_call_async
lockd: Unknown symbol rpc_create_client
lockd: Unknown symbol svc_makesock
lockd: Unknown symbol nlm_debug
lockd: Unknown symbol xdr_decode_netobj
lockd: Unknown symbol svc_wake_up
lockd: Unknown symbol rpciod_down
lockd: Unknown symbol svc_exit_thread
lockd: Unknown symbol xdr_encode_string
lockd: Unknown symbol rpc_call_sync
lockd: Unknown symbol xdr_decode_string_inplace
lockd: Unknown symbol svc_set_client
lockd: Unknown symbol svc_process
lockd: Unknown symbol xprt_set_timeout
lockd: Unknown symbol rpc_restart_call
lockd: Unknown symbol svc_create_thread
exportfs: exports duplicate symbol find_exported_dentry (owned by autofs)
nfsd: Unknown symbol cache_flush
nfsd: Unknown symbol find_exported_dentry
nfsd: Unknown symbol rpcauth_lookup_credcache
nfsd: Unknown symbol svc_proc_register
nfsd: Unknown symbol cache_register
nfsd: Unknown symbol svc_recv
nfsd: Unknown symbol svc_create
nfsd: Unknown symbol svcauth_unix_purge
nfsd: Unknown symbol rpciod_up
nfsd: Unknown symbol xdr_encode_opaque
nfsd: Unknown symbol xdr_reserve_space
nfsd: Unknown symbol svc_destroy
nfsd: Unknown symbol cache_fresh
nfsd: Unknown symbol auth_domain_put
nfsd: Unknown symbol xdr_inline_decode
nfsd: Unknown symbol xprt_create_proto
nfsd: Unknown symbol nfsd_debug
nfsd: Unknown symbol rpc_call_async
nfsd: Unknown symbol svc_makesock
nfsd: Unknown symbol auth_unix_lookup
nfsd: Unknown symbol svc_seq_show
nfsd: Unknown symbol unix_domain_find
nfsd: Unknown symbol auth_unix_forget_old
nfsd: Unknown symbol svc_proc_unregister
nfsd: Unknown symbol auth_unix_add_addr
nfsd: Unknown symbol cache_init
nfsd: Unknown symbol qword_get
nfsd: Unknown symbol rpc_shutdown_client
nfsd: Unknown symbol rpciod_down
nfsd: Unknown symbol put_rpccred
nfsd: Unknown symbol svc_exit_thread
nfsd: Unknown symbol svc_set_client
nfsd: Unknown symbol lockd_down
nfsd: Unknown symbol xdr_init_decode
nfsd: Unknown symbol cache_unregister
nfsd: Unknown symbol qword_add
nfsd: Unknown symbol lockd_up
nfsd: Unknown symbol nlmsvc_ops
nfsd: Unknown symbol xdr_init_encode
nfsd: Unknown symbol rpc_call_sync
nfsd: Unknown symbol export_op_default
nfsd: Unknown symbol xdr_decode_string_inplace
nfsd: Unknown symbol rpc_new_client
nfsd: Unknown symbol svc_process
nfsd: Unknown symbol svc_reserve
nfsd: Unknown symbol cache_purge
nfsd: Unknown symbol cache_check
nfsd: Unknown symbol qword_addhex
nfsd: Unknown symbol svc_create_thread
nfsd: Unknown symbol auth_domain_find
sunrpc: exports duplicate symbol rpc_max_payload (owned by autofs)
eth0: no IPv6 routers present
On Wed, 2005-12-14 at 20:40 -0800, Andrew Morton wrote:
> Ashutosh Naik <[email protected]> wrote:
> >
> > This patch ensures that an exported symbol does not already exist in
> > the kernel or in some other module's exported symbol table. This is
> > done by checking the symbol tables for the exported symbol at the time
> > of loading the module. Currently this is done after the relocation of
> > the symbol.
>
> This patch causes weird things to happen on ppc64.
And probably in general:
+ for (i = 0; i < mod->num_syms; i++)
+ if (!__find_symbol(mod->syms[i].name, &owner, &crc, 1)) {
+ name = mod->syms[i].name;
+ ret = -ENOEXEC;
+ goto dup;
__find_symbol returns the value, or 0 on failure. This test is
backwards, as is the one below it.
Rusty.
--
ccontrol: http://ozlabs.org/~rusty/ccontrol
On 12/15/05, Rusty Russell <[email protected]> wrote:
> On Wed, 2005-12-14 at 20:40 -0800, Andrew Morton wrote:
> + if (!__find_symbol(mod->syms[i].name, &owner, &crc, 1)) {
if (__find_symbol(mod->syms[i].name, &owner, &crc, 1)) {
>+ if (!__find_symbol(mod->gpl_syms[i].name, &owner, &crc, 1)) {
if (__find_symbol(mod->gpl_syms[i].name, &owner, &crc, 1)) {
Oops... I dunno how we missed it
This code is architecture independent.
Changelog -
This patch ensures that an exported symbol does not already exist in
the kernel or in some other module's exported symbol table. This is
done by checking the symbol tables for the exported symbol at the time
of loading the module. Currently this is done after the relocation of
the symbol.
Signed-off-by: Ashutosh Naik <[email protected]>
Signed-off-by: Anand Krishnan <[email protected]>