lkml.org 
[lkml]   [2018]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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>

\
 
 \ /
  Last update: 2018-04-27 18:12    [W:0.504 / U:1.304 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site