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  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]
Date:   Fri, 27 Apr 2018 09:14:17 +0200
From:   Ingo Molnar <>
To:     Masami Hiramatsu <>
        Ingo Molnar <>,
        "H . Peter Anvin" <>,,
        Ananth N Mavinakayanahalli <>,
        Anil S Keshavamurthy <>,
        "David S . Miller" <>,
        Jon Medhurst <>,
        Will Deacon <>,
        Arnd Bergmann <>,
        David Howells <>,
        Heiko Carstens <>,
        "Tobin C . Harding" <>,
        Linus Torvalds <>,
        Thomas Richter <>,,,,,,
Subject: Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as
 kallsyms does

* Masami Hiramatsu <> 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?

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:
> +	 */

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 

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);
		seq_printf(m, "0x%px-0x%px\t%ps\n", ent->start_addr, ent->end_addr, ent->start_addr);


This variant eliminates the unnecessary complication over local variables and 
makes it abundantly clear what gets printed and how.

( Note that the kprobe_blacklist_entry type cleanup should still be done, 
  regardless of code flow choices. )



Powered by blists - more mailing lists