linux-kernel - Re: [PATCH 18/25] dynamic_debug: Introduce global fake module param $module.dyndbg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJfuBxyJe4NbN7GDk4Si5A0mN2C6NFSPrbOsFt-pGs84DFZ1nA@mail.gmail.com>
Date:	Wed, 7 Dec 2011 01:33:45 -0700
From:	Jim Cromie <jim.cromie@...il.com>
To:	Rusty Russell <rusty@...tcorp.com.au>
Cc:	jbaron@...hat.com, greg@...ah.com, joe@...ches.com,
	bart.vanassche@...il.com, linux-kernel@...r.kernel.org,
	Thomas Renninger <trenn@...e.de>
Subject: Re: [PATCH 18/25] dynamic_debug: Introduce global fake module param $module.dyndbg

On Tue, Dec 6, 2011 at 6:01 PM, Rusty Russell <rusty@...tcorp.com.au> wrote:
> On Tue,  6 Dec 2011 12:11:28 -0700, jim.cromie@...il.com wrote:
>> From: Jim Cromie <jim.cromie@...il.com>
>>
>> Rework Thomas Renninger's $module.ddebug boot-time debugging feature,
>> from https://lkml.org/lkml/2010/9/15/397
>
> This worked out pretty neatly.  Thanks!
>

It did.  Almost like you planned it.

> A few questions from your patches:
>
>> diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
>> index 7e3c53a..b77f43b 100644
>> --- a/include/linux/dynamic_debug.h
>> +++ b/include/linux/dynamic_debug.h
>> @@ -39,11 +39,16 @@ struct _ddebug {
>>  int ddebug_add_module(struct _ddebug *tab, unsigned int n,
>>                               const char *modname);
>>
>> +struct kernel_param;
>> +
>>  #if defined(CONFIG_DYNAMIC_DEBUG)
>>  extern int ddebug_remove_module(const char *mod_name);
>> +
>>  extern __printf(2, 3)
>>  int __dynamic_pr_debug(struct _ddebug *descriptor, const char *fmt, ...);
>
> struct kernel_param def belongs in future patch?  And whitespace change
> is kind of annoying.

kernel-param not needed - was part of old way.
whitespace cleaned out too.


>
>> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>> index 7939f63..91bd56a 100644
>> --- a/include/linux/moduleparam.h
>> +++ b/include/linux/moduleparam.h
>> @@ -292,7 +292,7 @@ extern int parse_args(const char *name,
>>                     char *args,
>>                     const struct kernel_param *params,
>>                     unsigned num,
>> -                   int (*unknown)(char *param, char *val));
>> +                   int (*unknown)(char *param, char *val, const char *modname));
>
> Do you really want the modname here, or a struct module?  I wonder if we
> should make this a standard opaque void *.  Of course, we'd have to
> modify parse_args callers, but you're abusing "name" arg here a bit
> anyway.
>

I thought about the possible abuse, but then saw existing uses:

linux-2.6]$ grep parse_args init/main.c
	parse_args("early options", cmdline, NULL, 0, do_early_param);
	parse_args("Booting kernel", static_command_line, __start___param,

"Booting kernel" isnt quite a name either.

wrt using a struct module arg, Id prefer not.
what I need is the name, so that the ddebug-queries being passed
can leave out the module name and thus be shorter and less redundant

forex:
foo.dyndbg=" func bar +p ; func buz +p ; func bang +p "

would otherwize need "module foo" in each of the 4 rules, which would
be tedious.
Since its been given already by user, and already available in load_module()'s
existing call to parse_args(), it seems straightforward to alter the
cb sig, and pass it down.



>> @@ -2893,7 +2893,8 @@ static struct module *load_module(void __user *umod,
>>         mutex_unlock(&module_mutex);
>>
>>         /* Module is ready to execute: parsing args may do that. */
>> -     err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp, NULL);
>> +     err = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
>> +                      ddebug_dyndbg_param_cb);
>>         if (err < 0)
>>                 goto unlink;
>
> You have to
>

?? You left this thought unfinished.


>
>
>> @@ -510,8 +510,7 @@ asmlinkage void __init start_kernel(void)
>>       printk(KERN_NOTICE "Kernel command line: %s\n", boot_command_line);
>>       parse_early_param();
>>       parse_args("Booting kernel", static_command_line, __start___param,
>> -                __stop___param - __start___param,
>> -                &unknown_bootoption);
>> +                __stop___param - __start___param, &unknown_bootoption);
>>
>>       jump_label_init();
>
> Gratuitous whitespace change.
>

undone.

>> +int ddebug_dyndbg_param_cb(char *param, char* val, const char* modname)
>> +{
>> +     if (strcmp(param, "dyndbg")) {
>> +             pr_warn("bogus param %s=%s received for %s\n",
>> +                     param, val, modname);
>> +             return -EINVAL;
>> +     }
>
> This is already done in parse_args(); don't add a pr_warn here.
>

OK yes.  I'll change rc to -ENOENT.


> Thanks,
> Rusty.


thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/