Hi,
This 3rd version of the series which fixes %p uses in kprobes. Some by replacing with %pS, some by replacing with %px but masking with kallsyms_show_value().
I've read the thread about %pK and if I understand correctly we shouldn't print kernel addresses. However, kprobes debugfs interface can not stop to show the actual probe address because it should be compared with addresses in kallsyms for debugging. So, it depends on that kallsyms_show_value() allows to show address to user, because if it returns true, anyway that user can dump /proc/kallsyms. Other error messages are replaced it with %pS or just removed.
This series also including some fixes for arch ports too.
Changes in this version; - [2/7]: Updated for the latest linus tree. - [4/7][5/7]: Do not use %px.
Thank you,
---
Masami Hiramatsu (7): kprobes: Make blacklist root user read only kprobes: Show blacklist addresses as same as kallsyms does kprobes: Show address of kprobes if kallsyms does kprobes: Replace %p with other pointer types kprobes/x86: Fix %p uses in error messages kprobes/arm: Fix %p uses in error messages kprobes/arm64: Fix %p uses in error messages
arch/arm/probes/kprobes/core.c | 10 ++++--- arch/arm/probes/kprobes/test-core.c | 1 - arch/arm64/kernel/probes/kprobes.c | 4 +-- arch/x86/kernel/kprobes/core.c | 12 +++------ kernel/kprobes.c | 48 ++++++++++++++++++++++------------- 5 files changed, 41 insertions(+), 34 deletions(-)
-- Masami Hiramatsu (Linaro) mhiramat@kernel.org
Since the blacklist file indicates a sensitive address information to reader, it should be restricted to the root user.
Suggested-by: Thomas Richter tmricht@linux.ibm.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- kernel/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index ea619021d901..51096eece801 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void) if (!file) goto error;
- file = debugfs_create_file("blacklist", 0444, dir, NULL, + file = debugfs_create_file("blacklist", 0400, dir, NULL, &debugfs_kprobe_blacklist_ops); if (!file) goto error;
On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
Since the blacklist file indicates a sensitive address information to reader, it should be restricted to the root user.
Suggested-by: Thomas Richter tmricht@linux.ibm.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
kernel/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>
If you want these to go to the stable tree(s), that is great, but please mark them properly. As you did it here for this series, it will not work.
thanks,
greg k-h
On Fri, 27 Apr 2018 08:56:11 +0200 Greg KH gregkh@linuxfoundation.org wrote:
On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
Since the blacklist file indicates a sensitive address information to reader, it should be restricted to the root user.
Suggested-by: Thomas Richter tmricht@linux.ibm.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
kernel/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
<formletter>
This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly.
</formletter>
If you want these to go to the stable tree(s), that is great, but please mark them properly. As you did it here for this series, it will not work.
Sorry Greg. I'll mark it properly for next version.
Thanks,
* Masami Hiramatsu mhiramat@kernel.org wrote:
Since the blacklist file indicates a sensitive address information to reader, it should be restricted to the root user.
Suggested-by: Thomas Richter tmricht@linux.ibm.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
kernel/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index ea619021d901..51096eece801 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void) if (!file) goto error;
- file = debugfs_create_file("blacklist", 0444, dir, NULL,
- file = debugfs_create_file("blacklist", 0400, dir, NULL, &debugfs_kprobe_blacklist_ops); if (!file) goto error;
Note that in a typical Linux distro debugfs is already root-only:
fomalhaut:~> ls -ld /sys/kernel/debug drwx------ 28 root root 0 Apr 23 08:55 /sys/kernel/debug
but this change might make sense if debugfs is mounted in some other fashion.
But the patch looks incomplete, 'blacklist' is not the only word-readable file in the kprobes hierarchy. The kprobes directory itself, and the 'list' file is readable as well:
[root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes
[root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/
-r--r--r-- 1 root root 0 Apr 23 08:55 blacklist -rw------- 1 root root 0 Apr 23 08:55 enabled -r--r--r-- 1 root root 0 Apr 23 08:55 list
So not just the blacklist should be 400 but 'list' as well, and the main kprobes directory as well.
Thanks,
Ingo
On Fri, 27 Apr 2018 09:04:12 +0200 Ingo Molnar mingo@kernel.org wrote:
- Masami Hiramatsu mhiramat@kernel.org wrote:
Since the blacklist file indicates a sensitive address information to reader, it should be restricted to the root user.
Suggested-by: Thomas Richter tmricht@linux.ibm.com Signed-off-by: Masami Hiramatsu mhiramat@kernel.org
kernel/kprobes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index ea619021d901..51096eece801 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void) if (!file) goto error;
- file = debugfs_create_file("blacklist", 0444, dir, NULL,
- file = debugfs_create_file("blacklist", 0400, dir, NULL, &debugfs_kprobe_blacklist_ops); if (!file) goto error;
Note that in a typical Linux distro debugfs is already root-only:
fomalhaut:~> ls -ld /sys/kernel/debug drwx------ 28 root root 0 Apr 23 08:55 /sys/kernel/debug
but this change might make sense if debugfs is mounted in some other fashion.
But the patch looks incomplete, 'blacklist' is not the only word-readable file in the kprobes hierarchy. The kprobes directory itself, and the 'list' file is readable as well:
[root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes
[root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/
-r--r--r-- 1 root root 0 Apr 23 08:55 blacklist -rw------- 1 root root 0 Apr 23 08:55 enabled -r--r--r-- 1 root root 0 Apr 23 08:55 list
So not just the blacklist should be 400 but 'list' as well, and the main kprobes directory as well.
OK, I'll mark it 0400 too. For the kprobes directory, as Thomas pointed in the original thread, that is currently debugfs API's limitation.
https://patchwork.kernel.org/patch/10364817/
We only have debugfs_create_dir() but it doesn't have "mode" flag.
struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);
And doesn't care the parent mode bits.
Thanks,
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 mhiramat@kernel.org --- 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; }
* 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?
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
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,
Show probed address in debugfs kprobe list file as same as kallsyms does. This information is used for checking kprobes are placed in the expected address. So it should be able to compared with address in kallsyms.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- kernel/kprobes.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index e7d7e3e8598a..94af7c99cf81 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2326,6 +2326,7 @@ static void report_probe(struct seq_file *pi, struct kprobe *p, const char *sym, int offset, char *modname, struct kprobe *pp) { char *kprobe_type; + void *addr = p->addr;
if (p->pre_handler == pre_handler_kretprobe) kprobe_type = "r"; @@ -2334,13 +2335,16 @@ static void report_probe(struct seq_file *pi, struct kprobe *p, else kprobe_type = "k";
+ if (!kallsyms_show_value()) + addr = NULL; + if (sym) - seq_printf(pi, "%p %s %s+0x%x %s ", - p->addr, kprobe_type, sym, offset, + seq_printf(pi, "%px %s %s+0x%x %s ", + addr, kprobe_type, sym, offset, (modname ? modname : " ")); - else - seq_printf(pi, "%p %s %p ", - p->addr, kprobe_type, p->addr); + else /* try to use %pS */ + seq_printf(pi, "%px %s %pS ", + addr, kprobe_type, p->addr);
if (!pp) pp = p;
Replace %p with %pS or just remove it if unneeded.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- Changes in v3: - Do not use %px in any case. Changes in v2: - Rebased on linux-next --- kernel/kprobes.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 94af7c99cf81..a72ff4999b11 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap) op = container_of(ap, struct optimized_kprobe, kp); if (unlikely(list_empty(&op->list))) printk(KERN_WARNING "Warning: found a stray unused " - "aggrprobe@%p\n", ap->addr); + "aggrprobe@%pS\n", ap->addr); /* Enable the probe again */ ap->flags &= ~KPROBE_FLAG_DISABLED; /* Optimize it again (remove from op->list) */ @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p) ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 0, 0); if (ret) { - pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret); + pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n", + p->addr, ret); return ret; }
@@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p)
ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0); - WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret); + WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", + p->addr, ret); return ret; } #else /* !CONFIG_KPROBES_ON_FTRACE */ @@ -2169,11 +2171,12 @@ int enable_kprobe(struct kprobe *kp) } EXPORT_SYMBOL_GPL(enable_kprobe);
+/* Caller must NOT call this in usual path. This is only for critical case */ void dump_kprobe(struct kprobe *kp) { - printk(KERN_WARNING "Dumping kprobe:\n"); - printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n", - kp->symbol_name, kp->addr, kp->offset); + pr_err("Dumping kprobe:\n"); + pr_err("Name: %s\nOffset: %x\nAddress: %pS\n", + kp->symbol_name, kp->offset, kp->addr); } NOKPROBE_SYMBOL(dump_kprobe);
@@ -2196,11 +2199,8 @@ static int __init populate_kprobe_blacklist(unsigned long *start, entry = arch_deref_entry_point((void *)*iter);
if (!kernel_text_address(entry) || - !kallsyms_lookup_size_offset(entry, &size, &offset)) { - pr_err("Failed to find blacklist at %p\n", - (void *)entry); + !kallsyms_lookup_size_offset(entry, &size, &offset)) continue; - }
ent = kmalloc(sizeof(*ent), GFP_KERNEL); if (!ent)
* Masami Hiramatsu mhiramat@kernel.org wrote:
@@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap) op = container_of(ap, struct optimized_kprobe, kp); if (unlikely(list_empty(&op->list))) printk(KERN_WARNING "Warning: found a stray unused "
"aggrprobe@%p\n", ap->addr);
"aggrprobe@%pS\n", ap->addr);
Is this a kernel bug? If yes then please convert this to a WARN_ON_ONCE() that doesn't print any kernel addresses.
/* Enable the probe again */ ap->flags &= ~KPROBE_FLAG_DISABLED; /* Optimize it again (remove from op->list) */ @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p) ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 0, 0); if (ret) {
pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",p->addr, ret);
Can this happen during normal use? If yes then just remove the warning message.
} @@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p) ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
- WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
- WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
p->addr, ret);
As this is a signal of a kernel bug, just convert this to a regular WARN_ONCE() that doesn't print any kernel addresses.
+/* Caller must NOT call this in usual path. This is only for critical case */ void dump_kprobe(struct kprobe *kp) {
- printk(KERN_WARNING "Dumping kprobe:\n");
- printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
kp->symbol_name, kp->addr, kp->offset);
- pr_err("Dumping kprobe:\n");
- pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
kp->symbol_name, kp->offset, kp->addr);
No, this function should just go away and be replaced by a WARN() in reenter_kprobe().
Thanks,
Ingo
On Fri, 27 Apr 2018 08:56:36 +0200 Ingo Molnar mingo@kernel.org wrote:
- Masami Hiramatsu mhiramat@kernel.org wrote:
@@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap) op = container_of(ap, struct optimized_kprobe, kp); if (unlikely(list_empty(&op->list))) printk(KERN_WARNING "Warning: found a stray unused "
"aggrprobe@%p\n", ap->addr);
"aggrprobe@%pS\n", ap->addr);Is this a kernel bug? If yes then please convert this to a WARN_ON_ONCE() that doesn't print any kernel addresses.
OK, I will do.
/* Enable the probe again */ ap->flags &= ~KPROBE_FLAG_DISABLED; /* Optimize it again (remove from op->list) */ @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p) ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 0, 0); if (ret) {
pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",p->addr, ret);Can this happen during normal use? If yes then just remove the warning message.
Yes, it can, for example probing on live-patched function.
} @@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p) ret = ftrace_set_filter_ip(&kprobe_ftrace_ops, (unsigned long)p->addr, 1, 0);
- WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
- WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
p->addr, ret);As this is a signal of a kernel bug, just convert this to a regular WARN_ONCE() that doesn't print any kernel addresses.
Would you mean just replace WARN with WARN_ONCE as below?
WARN_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", p->addr, ret);
Anyway, without printing kprobe->addr, it may be meaningless for this case.
+/* Caller must NOT call this in usual path. This is only for critical case */ void dump_kprobe(struct kprobe *kp) {
- printk(KERN_WARNING "Dumping kprobe:\n");
- printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
kp->symbol_name, kp->addr, kp->offset);
- pr_err("Dumping kprobe:\n");
- pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
kp->symbol_name, kp->offset, kp->addr);No, this function should just go away and be replaced by a WARN() in reenter_kprobe().
Would you consider to use pr_err() here? If so, I'll move this dump as you suggested.
Thank you,
On Sat, 28 Apr 2018 00:42:02 +0900 Masami Hiramatsu mhiramat@kernel.org wrote:
+/* Caller must NOT call this in usual path. This is only for critical case */ void dump_kprobe(struct kprobe *kp) {
- printk(KERN_WARNING "Dumping kprobe:\n");
- printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
kp->symbol_name, kp->addr, kp->offset);
- pr_err("Dumping kprobe:\n");
- pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
kp->symbol_name, kp->offset, kp->addr);No, this function should just go away and be replaced by a WARN() in reenter_kprobe().
Would you consider to use pr_err() here? If so, I'll move this dump as you suggested.
So, this is actually called right before BUG(), which means we found a non-recoverable error while recovering from reentrant kprobes. Since the BUG() dumps all registers and stack as same as WARN(), I think we should keep it. (I would like to dump all kprobes fields like flags, etc. so that we can find it is broken or not.)
Thank you,
Fix %p uses in error messages in kprobes/x86. - Some %p uses are not needed. Just remove it (or remove message). - One %p use seems important to show some address so replaced with %pS.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- Changes in v3: - Do not use %px. --- arch/x86/kernel/kprobes/core.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 0715f827607c..d2c9d5c8e4ce 100644 --- a/arch/x86/kernel/kprobes/core.c +++ b/arch/x86/kernel/kprobes/core.c @@ -391,8 +391,6 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn) - (u8 *) real; if ((s64) (s32) newdisp != newdisp) { pr_err("Kprobes error: new displacement does not fit into s32 (%llx)\n", newdisp); - pr_err("\tSrc: %p, Dest: %p, old disp: %x\n", - src, real, insn->displacement.value); return 0; } disp = (u8 *) dest + insn_offset_displacement(insn); @@ -636,8 +634,7 @@ static int reenter_kprobe(struct kprobe *p, struct pt_regs *regs, * Raise a BUG or we'll continue in an endless reentering loop * and eventually a stack overflow. */ - printk(KERN_WARNING "Unrecoverable kprobe detected at %p.\n", - p->addr); + pr_err("Unrecoverable kprobe detected.\n"); dump_kprobe(p); BUG(); default: @@ -1146,12 +1143,11 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) (addr < (u8 *) jprobe_return_end)) { if (stack_addr(regs) != saved_sp) { struct pt_regs *saved_regs = &kcb->jprobe_saved_regs; - printk(KERN_ERR - "current sp %p does not match saved sp %p\n", + pr_err("current sp %pS does not match saved sp %pS\n", stack_addr(regs), saved_sp); - printk(KERN_ERR "Saved registers for jprobe %p\n", jp); + pr_err("Saved registers for jprobe\n"); show_regs(saved_regs); - printk(KERN_ERR "Current registers\n"); + pr_err("Current registers\n"); show_regs(regs); BUG(); }
* Masami Hiramatsu mhiramat@kernel.org wrote:
(addr < (u8 *) jprobe_return_end)) { if (stack_addr(regs) != saved_sp) { struct pt_regs *saved_regs = &kcb->jprobe_saved_regs;
printk(KERN_ERR"current sp %p does not match saved sp %p\n",
pr_err("current sp %pS does not match saved sp %pS\n", stack_addr(regs), saved_sp);
Why does this use %pS? Stack pointers are typically pointing into general kernel RAM, which is totally pointless to display 'symbolically'.
Thanks,
Ingo
On Fri, 27 Apr 2018 08:58:17 +0200 Ingo Molnar mingo@kernel.org wrote:
- Masami Hiramatsu mhiramat@kernel.org wrote:
(addr < (u8 *) jprobe_return_end)) { if (stack_addr(regs) != saved_sp) { struct pt_regs *saved_regs = &kcb->jprobe_saved_regs;
printk(KERN_ERR"current sp %p does not match saved sp %p\n",
pr_err("current sp %pS does not match saved sp %pS\n", stack_addr(regs), saved_sp);Why does this use %pS? Stack pointers are typically pointing into general kernel RAM, which is totally pointless to display 'symbolically'.
That was I pointed in the last thread, but not accepted. OK, anyway, there are no good reason to show such a dynamic value. And now jprobes are gone. I just drop %p from this part.
Thanks,
Fix %p uses in error messages by removing it and using general dumper.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org --- arch/arm/probes/kprobes/core.c | 10 +++++----- arch/arm/probes/kprobes/test-core.c | 1 - 2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c index e90cc8a08186..e90165a56620 100644 --- a/arch/arm/probes/kprobes/core.c +++ b/arch/arm/probes/kprobes/core.c @@ -289,8 +289,8 @@ void __kprobes kprobe_handler(struct pt_regs *regs) break; case KPROBE_REENTER: /* A nested probe was hit in FIQ, it is a BUG */ - pr_warn("Unrecoverable kprobe detected at %p.\n", - p->addr); + pr_warn("Unrecoverable kprobe detected.\n"); + dump_kprobe(p); /* fall through */ default: /* impossible cases */ @@ -615,11 +615,11 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) if (orig_sp != stack_addr) { struct pt_regs *saved_regs = (struct pt_regs *)kcb->jprobe_saved_regs.ARM_sp; - printk("current sp %lx does not match saved sp %lx\n", + pr_err("current sp %lx does not match saved sp %lx\n", orig_sp, stack_addr); - printk("Saved registers for jprobe %p\n", jp); + pr_err("Saved registers for jprobe\n"); show_regs(saved_regs); - printk("Current registers\n"); + pr_err("Current registers\n"); show_regs(regs); BUG(); } diff --git a/arch/arm/probes/kprobes/test-core.c b/arch/arm/probes/kprobes/test-core.c index 9ed0129bed3c..b5c892e24244 100644 --- a/arch/arm/probes/kprobes/test-core.c +++ b/arch/arm/probes/kprobes/test-core.c @@ -1460,7 +1460,6 @@ static bool check_test_results(void) print_registers(&result_regs);
if (mem) { - pr_err("current_stack=%p\n", current_stack); pr_err("expected_memory:\n"); print_memory(expected_memory, mem_size); pr_err("result_memory:\n");
Fix %p uses in error messages by removing it because those are redundant or meaningless.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Acked-by: Will Deacon will.deacon@arm.com --- arch/arm64/kernel/probes/kprobes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c index d849d9804011..34f78d07a068 100644 --- a/arch/arm64/kernel/probes/kprobes.c +++ b/arch/arm64/kernel/probes/kprobes.c @@ -275,7 +275,7 @@ static int __kprobes reenter_kprobe(struct kprobe *p, break; case KPROBE_HIT_SS: case KPROBE_REENTER: - pr_warn("Unrecoverable kprobe detected at %p.\n", p->addr); + pr_warn("Unrecoverable kprobe detected.\n"); dump_kprobe(p); BUG(); break; @@ -521,7 +521,7 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs) (struct pt_regs *)kcb->jprobe_saved_regs.sp; pr_err("current sp %lx does not match saved sp %lx\n", orig_sp, stack_addr); - pr_err("Saved registers for jprobe %p\n", jp); + pr_err("Saved registers for jprobe\n"); __show_regs(saved_regs); pr_err("Current registers\n"); __show_regs(regs);
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Ingo Molnar -
Masami Hiramatsu