On Tue, 8 Jan 2008 22:50:06 +1100
Rusty Russell <[EMAIL PROTECTED]> wrote:
> On Tuesday 08 January 2008 22:33:23 Ingo Molnar wrote:
> > * Rusty Russell <[EMAIL PROTECTED]> wrote:
> > > +/* FIXME: Risky: returns a pointer into a module w/o lock */
> >
> > stupid question: since module unloads are so rare, why isnt this via the
> > same mechanism that CPU hotplug uses to securely unregister CPUs? I.e.
> > quiet all CPUs, disable irqs on all of them, then unlink the module.
>
> That's what we do. This old locking stuff is legacy.
>
> And here's the patch for the FIXME (which I put in to remind myself):
>
> Make module_address_lookup safe
>
> module_address_lookup releases preemption then returns a pointer into
> the module space. The only user (kallsyms) copies the result, so just
> do that under the preempt disable.
>
> ...
>
> -/* For kallsyms to ask for address resolution. NULL means not found.
> - We don't lock, as this is used for oops resolution and races are a
> - lesser concern. */
> -/* FIXME: Risky: returns a pointer into a module w/o lock */
> -const char *module_address_lookup(unsigned long addr,
> - unsigned long *size,
> - unsigned long *offset,
> - char **modname)
> +/* For kallsyms to ask for address resolution. NULL means not found.
> Careful
> + * not to lock to avoid deadlock on oopses, simply disable preemption. */
> +char *module_address_lookup(unsigned long addr,
> + unsigned long *size,
> + unsigned long *offset,
> + char **modname,
> + char *namebuf)
> {
> struct module *mod;
> const char *ret = NULL;
> @@ -2256,6 +2255,11 @@ const char *module_address_lookup(unsign
> ret = get_ksymbol(mod, addr, size, offset);
> break;
> }
> + }
> + /* Make a copy in here where it's safe */
> + if (ret) {
> + strncpy(namebuf, ret, KSYM_NAME_LEN - 1);
> + ret = namebuf;
> }
> preempt_enable();
> return ret;
The string handling in here has become a bit scruffy.
afacit the `namebuf[KSYM_NAME_LEN - 1] = 0;' would be unneeded if we were
to use strlcpy() and I suspect the `namebuf[0] = 0;' isn't needed either.
And the use of strlcpy() means we don't need to subtract 1 from
KSYM_NAME_LEN and we don't need to fret about weird strncpy semantics when
the input string is too large.
And the fact that incoming arg `namebuf' MUST point at a
KSYM_NAME_LEN-sized buffer could be better communicated by using a
dedicated struct for this, or by giving the arg a type of `char
namebuf[KSYM_NAME_LEN]'. Or by adding a comment. Or by just ignoring
me and doing something more useful.
--
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/
