Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Masami Hiramatsu]

On Fri, 27 Apr 2018 09:14:17 +0200
Ingo Molnar  wrote:

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

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 

Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Masami Hiramatsu]

On Fri, 27 Apr 2018 09:14:17 +0200
Ingo Molnar  wrote:

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

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 

Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Ingo Molnar]

* 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 
arch_deref_entry_point()...

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);

?

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. )

Thanks,

Ingo

Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Ingo Molnar]

* 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 
arch_deref_entry_point()...

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);

?

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. )

Thanks,

Ingo

[PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Masami Hiramatsu]

Show kprobes blacklist addresses under same condition of
showing kallsyms addresses.

Since there are several name conflict for local symbols,
kprobe blacklist needs to show each addresses so that
user can identify where is on blacklist by comparing
with kallsyms.

Signed-off-by: Masami Hiramatsu 
---
 Changes in v3:
  - Updated based on the latest linus tree.
---
 kernel/kprobes.c |   12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 51096eece801..e7d7e3e8598a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2427,9 +2427,17 @@ static int kprobe_blacklist_seq_show(struct seq_file *m, 
void *v)
 {
struct kprobe_blacklist_entry *ent =
list_entry(v, struct kprobe_blacklist_entry, list);
+   void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;
 
-   seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
-  (void *)ent->end_addr, (void *)ent->start_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);
return 0;
 }
 

[PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Masami Hiramatsu]

Show kprobes blacklist addresses under same condition of
showing kallsyms addresses.

Since there are several name conflict for local symbols,
kprobe blacklist needs to show each addresses so that
user can identify where is on blacklist by comparing
with kallsyms.

Signed-off-by: Masami Hiramatsu 
---
 Changes in v3:
  - Updated based on the latest linus tree.
---
 kernel/kprobes.c |   12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 51096eece801..e7d7e3e8598a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2427,9 +2427,17 @@ static int kprobe_blacklist_seq_show(struct seq_file *m, 
void *v)
 {
struct kprobe_blacklist_entry *ent =
list_entry(v, struct kprobe_blacklist_entry, list);
+   void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;
 
-   seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
-  (void *)ent->end_addr, (void *)ent->start_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);
return 0;
 }