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
| ||
|
Date: Wed, 07 Mar 2012 11:00:56 +1030 From: Rusty Russell <rusty@...tcorp.com.au> To: Jason Baron <jbaron@...hat.com>, Thomas Renninger <trenn@...e.de> Cc: Jim Cromie <jim.cromie@...il.com>, linux-kernel@...r.kernel.org Subject: Re: [PATCH 18/25] dynamic_debug: Introduce global fake module param module.ddebug On Tue, 6 Mar 2012 09:47:54 -0500, Jason Baron <jbaron@...hat.com> wrote: > On Tue, Mar 06, 2012 at 10:02:56AM +0100, Thomas Renninger wrote: > > On Monday, March 05, 2012 06:22:06 PM Jim Cromie wrote: > > > 2012/3/5 Thomas Renninger <trenn@...e.de>: > > ... > > > I have reworked 18-25 on top of linux-next, and have been > > > casually boot testing it on my i586 based soekris, also for > > > several weeks. > > > > > > I have some misgivings about trivial things I added to params.c > > > (primarily pr-debugs in init-level), and about a few other details, > > > but I guess I should just write up an explanation and send it out. > > Seeing the stuff in linux-next soon would be great. Agreed. OK, here's a partial review. Patch 1: init: add a pr_info, pr_debug to initcall-levels > add a pr_info to print current level of initcall, > add a pr_debug and a counter to initcall_level() to indicate Meh. We have an initcall_debug flag. Let's use it please. Patch 2: params: add a pr-debug to parse_one's min/max level No, this is a great deal of spew for little purpose. Patch 3: params: parse_one's pr_debug("They are equal! ...") isnt informative Actually, callback address is informative, since it's kernel devs who are debugging this. But as they can add it, we should just drop this patch, or remove the line altogether. [PATCH 04/15] params: add 3rd arg to option handler callback This is fine, I'll take this as is. [PATCH 05/15] dynamic_debug: make dynamic-debug work during module initialization: > +static inline int ddebug_dyndbg_param_cb(char *param, char *val, > + const char *modname) > +{ > + if (strstr(param, "dyndbg")) > + pr_warn("dyndbg supported only in " > + "CONFIG_DYNAMIC_DEBUG builds\n"); > + return 0; /* allow unknown options */ > +} No, this breaks unknown module params! Please split into two callbacks: check_for_dyndebug() check_for_module_dyndebug() The latter may be in module.c and call a common helper, depends how neat it is. > + char *param, *modname; > + > + param = strstr(fqparam, "dyndbg"); > + if (!param) { > + pr_debug("%s: skip %s\n", doing, fqparam); > + return 0; /* param is unknown, ignore it (for boot) */ > + } > + if (param > fqparam) { > + /* fqparam has module prefix, split it in 2 */ > + *(param-1) = '\0'; > + modname = fqparam; > + } > + else > + modname = doing; > + > + if (verbose) > + pr_info("module: %s %s=\"%s\"\n", modname, param, val); > + > + ddebug_exec_queries(val ? val : "+p"); > + return 0; /* query failure shouldnt stop module load */ Please don't hack-parse, this accepts all kinds of invalid crap. Your wildcard patch is even worse. A sane option is: "dyndbg[=...]" on boot line turns dyndbg for everything. "<modname>.dyndbg[=...]" on boot line turns on dyndbg for that module. "dyndbg[=...]" on module line turns dyndbg for that module. Something like: const char *modname = NULL; char *sep; sep = strchr(fqparam, '.'); if (sep) { *sep = '\0'; modname = fqparam; fqparam = sep + 1; } ... [PATCH 07/15] dynamic_debug: add modname arg to exec_query callchain This looks sane. [PATCH 08/15] dynamic_debug: allow *.dyndbg=+p in boot args No, just make it "dyndbg=+p", as above. [PATCH 09/15] dynamic_debug: protect "dyndbg" fake module param name at compile-time BUILD_BUG_DECL is redundant, you should use BUILD_BUG_ZERO. But that insists on a constant expression, so you'd need a new variant. Probably easiest to drop this one. If someone calls their parameter dyndbg they probably mean exactly what they say. [PATCH 11/15] dyndbg: init at core level, not arch Please say why. [PATCH 14/15] dyndbg: replace if (verbose) pr_info with macro vpr_info > #define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__ > +#define vpr_info(fmt, ...) if (verbose) { pr_info(fmt, ##__VA_ARGS__); } This is a bear trap waiting to happen. Please do { } while(0) wrap! Thanks, Rusty. -- How could I marry someone with more hair than me? http://baldalex.org -- 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/
Powered by blists - more mailing lists