On Fri, 27 Apr 2018 09:14:17 +0200
Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Masami Hiramatsu <mhiramat@xxxxxxxxxx> 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@xxxxxxxxxx>