Messages in this thread | | | Date | Sat, 28 Apr 2018 01:10:56 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does |
| |
On Fri, 27 Apr 2018 09:14:17 +0200 Ingo Molnar <mingo@kernel.org> wrote:
> > * Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > + /* > > + * As long as kallsyms shows the address, kprobes blacklist also > > + * show it, Or, it shows null address and symbol. > > + */ > > Please _read_ the comments you write! > > In which universe does a capitalized 'Or' make sense, even if we ignore the > various other spelling mistakes?
It's a typo. I mean "show it. Or, it shows..." anyway,
> > Also, that sentence is unnecessarily complex, just say this: > > > + /* > > + * If /proc/kallsyms is not showing kernel addresses then we won't show > > + * them here either: > > + */
OK, look good to me.
> > But I'm unhappy about the messy typing and the messy code flow: > > + void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr; > > + /* > + * As long as kallsyms shows the address, kprobes blacklist also > + * show it, Or, it shows null address and symbol. > + */ > + if (!kallsyms_show_value()) > + start = end = NULL; > + > + seq_printf(m, "0x%px-0x%px\t%ps\n", start, end, > + (void *)ent->start_addr); > > > All three 'void *' type casts here are due to the bad type choices here: > > struct kprobe_blacklist_entry { > struct list_head list; > unsigned long start_addr; > unsigned long end_addr; > }; > > The natural type of ->start_addr and ->end_addr is 'void *', AFAICS this would > remove some other type casts from the kprobes code as well, such as from the > arch_deref_entry_point()...
Would you really think we should handle all the address with 'void *'? IOW, are there any policy that we handle the generic address by 'void *' or 'unsigned long'? For example, other address checker like kernel_text_address(), module_text_address(), and ftrace_location() receive 'unsigned long'. (only jump_label_text_reserved() using 'void *')
> > But the whole code flow introduced by this patch is messy as hell as well. > Why cannot this do the obvious thing: > > if (!kallsyms_show_value()) > seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL, ent->start_addr); > else > seq_printf(m, "0x%px-0x%px\t%ps\n", ent->start_addr, ent->end_addr, ent->start_addr); > > ?
Both are OK to me. I just didn't want to repeat the printk format string there.
> > This variant eliminates the unnecessary complication over local variables and > makes it abundantly clear what gets printed and how.
Agreed, it may shorten the patch.
> ( Note that the kprobe_blacklist_entry type cleanup should still be done, > regardless of code flow choices. ) > > Thanks, > > Ingo
Thank you,
-- Masami Hiramatsu <mhiramat@kernel.org>
| |