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
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
