format idle IP output func+offset/length

PATCH In review Liu, Changcheng 5 participants 11 messages Nov 6, 2017 – Nov 9, 2017 Library code

lore

Share & tools

lore

Liu, Changcheng

author

2017-11-06 05:25 UTC #0 lore raw

PATCH 1/1 format idle IP output func+offset/length

kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info

Signed-off-by Liu Changcheng <[email protected]>

Signed-off-by Jerry Liu <[email protected]>

lib/nmi_backtrace.c +1 -1

index 0bc0a35..9cc4178 100644
@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n",
 				cpu, instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);

Signature · 1 line

2.7.4

Sergey Senozhatsky

2017-11-06 08:05 UTC #1 lore raw

Quoted 11 lines · Changcheng

kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info
Signed-off-by Liu Changcheng <[email protected]>
Signed-off-by Jerry Liu <[email protected]>
lib/nmi_backtrace.c +0 -0
index 0bc0a35..9cc4178 100644

lib/nmi_backtrace.c bool nmi_cpu_backtrace(struct pt_regs *regs) · line 95

Quoted 5 lines

lib/nmi_backtrace.c +1 -1

@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n",

I guess the word 'pc' can be dropped from the message ^^

there seems to be other cases in the kernel when we print regs->pc/regs->ip.

-ss

Quoted 3 lines

lib/nmi_backtrace.c +0 -0

 				cpu, instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);

Liu, Changcheng

author

2017-11-06 10:52 UTC #2 lore raw

kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info

Signed-off-by Liu Changcheng <[email protected]>

Signed-off-by Jerry Liu <[email protected]>

lib/nmi_backtrace.c +1 -1

index 0bc0a35..9cc4178 100644
@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",
 				cpu, instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);

Signature · 1 line

2.7.4

Quoted 26 lines · Sergey Senozhatsky

Quoted 11 lines · Changcheng
kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info
Signed-off-by Liu Changcheng <[email protected]>
Signed-off-by Jerry Liu <[email protected]>
lib/nmi_backtrace.c +0 -0
index 0bc0a35..9cc4178 100644
lib/nmi_backtrace.c bool nmi_cpu_backtrace(struct pt_regs *regs) · line 95
Quoted 5 lines
lib/nmi_backtrace.c +1 -1
@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n",
I guess the word 'pc' can be dropped from the message ^^
there seems to be other cases in the kernel when we print regs->pc/regs->ip.
	-ss
Quoted 3 lines
lib/nmi_backtrace.c +0 -0
 				cpu, instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);

Petr Mladek

Reviewed-by

2017-11-06 12:15 UTC #3 lore raw

Quoted 11 lines · Changcheng

kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info
Signed-off-by Liu Changcheng <[email protected]>
Signed-off-by Jerry Liu <[email protected]>
lib/nmi_backtrace.c +0 -0
index 0bc0a35..9cc4178 100644

lib/nmi_backtrace.c bool nmi_cpu_backtrace(struct pt_regs *regs) · line 95

Quoted 5 lines

lib/nmi_backtrace.c +1 -1

@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",

Yup, removing "pc" makes sense as well.

Reviewed-by Petr Mladek <[email protected]>

Best Regards,
Petr

Josh Poimboeuf

2017-11-06 13:11 UTC #4 lore raw

In reply to Liu, Changcheng

Quoted 11 lines · Changcheng

kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info
Signed-off-by Liu Changcheng <[email protected]>
Signed-off-by Jerry Liu <[email protected]>
lib/nmi_backtrace.c +0 -0
index 0bc0a35..9cc4178 100644

lib/nmi_backtrace.c bool nmi_cpu_backtrace(struct pt_regs *regs) · line 98

Quoted 8 lines

lib/nmi_backtrace.c +1 -1

@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",
 				cpu, instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);

1) The patch introduces a compile warning.

2) When posting a new version of the patch, it should have a new version in the subject, e.g. "PATCH v2".

3) The subject is missing a prefix, like "lib_backtrace: "

4) The subject isn't very clear, how about something like:

lib_backtrace: fix kernel text address leak

5) The description isn't correct. KASLR isn't always enabled, it depends on the user's config. But even without KASLR, we don't want to leak kernel text addresses.

6) The description should use complete sentences.

7) I'm not sure the "Signed-off-by:" chain is correct. Was Jerry Liu the original author of the patch?

Signature · 1 line

Josh

Liu, Changcheng

author

2017-11-06 16:48 UTC #5 lore raw

Thx Josh. I'll refine the patch.
Answer question as below.

Quoted 7 lines · Josh Poimboeuf

Quoted 6 lines · Changcheng
kaslr feature is enabled in kernel.
Remove kernel text address when dumping idle IP info
Signed-off-by Liu Changcheng <[email protected]>
Signed-off-by Jerry Liu <[email protected]>

Quoted 15 lines

lib/nmi_backtrace.c bool nmi_cpu_backtrace(struct pt_regs *regs) · line 98

Quoted 13 lines

lib/nmi_backtrace.c +1 -1

index 0bc0a35..9cc4178 100644
@@ -92,7 +92,7 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
+			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",
 				cpu, instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);

1) The patch introduces a compile warning.

I've got the point because the conversion between pointer and unsigned long.

Quoted 3 lines

2) When posting a new version of the patch, it should have a new version in the subject, e.g. "PATCH v2".

Thx for your guide. I'm on the way to work with community.

Quoted 6 lines

3) The subject is missing a prefix, like "lib_backtrace: "
4) The subject isn't very clear, how about something like:
lib_backtrace: fix kernel text address leak

I'll use this subject.

Quoted 4 lines

5) The description isn't correct. KASLR isn't always enabled, it depends on the user's config. But even without KASLR, we don't want to leak kernel text addresses.

That's fine.

Quoted 2 lines

6) The description should use complete sentences.

Yes.

Quoted 3 lines

7) I'm not sure the "Signed-off-by:" chain is correct. Was Jerry Liu the original author of the patch?

This is my personal bad habit. I'll correct it.

Quoted 3 lines

Signature · 2 lines
-- 
Josh

Liu, Changcheng

author

2017-11-06 16:57 UTC #6 lore raw

Re: [PATCH v2] lib_backtrace: fix kernel text address leak

Don't leak idle function address in NMI backtrace.

Signed-off-by Liu Changcheng <[email protected]>

lib/nmi_backtrace.c +2 -2

index 46e4c749..61a6b5a 100644
@@ -93,8 +93,8 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
-				cpu, instruction_pointer(regs));
+			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",
+				cpu, (void *)instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);
 			if (regs)

Signature · 1 line

2.7.4

Josh Poimboeuf

2017-11-06 17:14 UTC #7 lore raw

Re: [PATCH v2] lib_backtrace: fix kernel text address leak

Quoted 9 lines · Changcheng

Don't leak idle function address in NMI backtrace.
Signed-off-by Liu Changcheng <[email protected]>
lib/nmi_backtrace.c +0 -0
index 46e4c749..61a6b5a 100644

lib/nmi_backtrace.c bool nmi_cpu_backtrace(struct pt_regs *regs) · line 100

Quoted 10 lines

lib/nmi_backtrace.c +2 -2

@@ -93,8 +93,8 @@ bool nmi_cpu_backtrace(struct pt_regs *regs)
 	if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
 		arch_spin_lock(&lock);
 		if (regs && cpu_in_idle(instruction_pointer(regs))) {
-			pr_warn("NMI backtrace for cpu %d skipped: idling at pc %#lx\n",
-				cpu, instruction_pointer(regs));
+			pr_warn("NMI backtrace for cpu %d skipped: idling at %pS\n",
+				cpu, (void *)instruction_pointer(regs));
 		} else {
 			pr_warn("NMI backtrace for cpu %d\n", cpu);
 			if (regs)

Sorry, I had a typo in my suggestion. The subject prefix should be:
"nmi_backtrace" instead of "lib_backtrace".

Also, when posting a followup patch, please remove the "Re: " from the subject so that it's clear that it's a new patch, and not a comment for the old one.

Otherwise it looks great to me. Thanks!

Signature · 1 line

Josh

kbuild test robot

build bot

2017-11-09 03:11 UTC #8 lore raw

In reply to Liu, Changcheng

Thank you for the patch! Perhaps something to improve:

Hi Changcheng,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.14-rc8]
[cannot apply to next-20171108]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Liu-Changcheng/format-idle-IP-output-func-offset-length/20171109-100035
config: i386-randconfig-x074-201745 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All warnings (new ones prefixed by >>):

In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14, from include/linux/cpumask.h:10, from lib/nmi_backtrace.c:16:
lib/nmi_backtrace.c: In function 'nmi_cpu_backtrace':

>> include/linux/kern_levels.h:5:18: warning: format '%p' expects argument of type 'void *', but argument 3 has type 'long unsigned int' [-Wformat=]
    #define KERN_SOH "\001"  /* ASCII Start Of Header */
                     ^
   include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
    #define KERN_WARNING KERN_SOH "4" /* warning conditions */
                         ^~~~~~~~
   include/linux/printk.h:304:9: note: in expansion of macro 'KERN_WARNING'
     printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
            ^~~~~~~~~~~~
   include/linux/printk.h:305:17: note: in expansion of macro 'pr_warning'
    #define pr_warn pr_warning
                    ^~~~~~~~~~
>> lib/nmi_backtrace.c:96:4: note: in expansion of macro 'pr_warn'

pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n", ^~~~~~~

vim +/pr_warn +96 lib/nmi_backtrace.c

    87	
    88	bool nmi_cpu_backtrace(struct pt_regs *regs)
    89	{
    90		static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED;
    91		int cpu = smp_processor_id();
    92	
    93		if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
    94			arch_spin_lock(&lock);
    95			if (regs && cpu_in_idle(instruction_pointer(regs))) {
  > 96				pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n",

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2017-11-09 03:22 UTC #10 lore raw

In reply to Liu, Changcheng

Thank you for the patch! Perhaps something to improve:

Hi Changcheng,

Thank you for the patch! Perhaps something to improve:

url: https://github.com/0day-ci/linux/commits/Liu-Changcheng/format-idle-IP-output-func-offset-length/20171109-100035
config: i386-randconfig-n0-11080844 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All warnings (new ones prefixed by >>):

lib/nmi_backtrace.c: In function 'nmi_cpu_backtrace':

>> lib/nmi_backtrace.c:96:4: warning: format '%p' expects argument of type 'void *', but argument 3 has type 'long unsigned int' [-Wformat=]

pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n", ^

vim +96 lib/nmi_backtrace.c

    87	
    88	bool nmi_cpu_backtrace(struct pt_regs *regs)
    89	{
    90		static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED;
    91		int cpu = smp_processor_id();
    92	
    93		if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
    94			arch_spin_lock(&lock);
    95			if (regs && cpu_in_idle(instruction_pointer(regs))) {
  > 96				pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n",

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Liu, Changcheng

author

2017-11-09 03:29 UTC #9 lore raw

In reply to kbuild test robot

Hi 0Day maintainers,
The right patch is https://lkml.org/lkml/2017/11/6/685
I don't know why the original wrong patch is tested again.

B.R.
Changcheng

Quoted 55 lines · kbuild test robot

Hi Changcheng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.14-rc8]
[cannot apply to next-20171108]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Liu-Changcheng/format-idle-IP-output-func-offset-length/20171109-100035
config: i386-randconfig-x074-201745 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7:0,
from include/linux/kernel.h:14, from include/linux/cpumask.h:10, from lib/nmi_backtrace.c:16:
lib/nmi_backtrace.c: In function 'nmi_cpu_backtrace':
Quoted 1 line
Quoted 1 line
include/linux/kern_levels.h:5:18: warning: format '%p' expects argument of type 'void *', but argument 3 has type 'long unsigned int' [-Wformat=]
    #define KERN_SOH "\001"  /* ASCII Start Of Header */
                     ^
   include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
    #define KERN_WARNING KERN_SOH "4" /* warning conditions */
                         ^~~~~~~~
   include/linux/printk.h:304:9: note: in expansion of macro 'KERN_WARNING'
     printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
            ^~~~~~~~~~~~
   include/linux/printk.h:305:17: note: in expansion of macro 'pr_warning'
    #define pr_warn pr_warning
                    ^~~~~~~~~~
Quoted 1 line
Quoted 1 line
lib/nmi_backtrace.c:96:4: note: in expansion of macro 'pr_warn'
pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n", ^~~~~~~
vim +/pr_warn +96 lib/nmi_backtrace.c
    87	
    88	bool nmi_cpu_backtrace(struct pt_regs *regs)
    89	{
    90		static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED;
    91		int cpu = smp_processor_id();
    92	
    93		if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
    94			arch_spin_lock(&lock);
    95			if (regs && cpu_in_idle(instruction_pointer(regs))) {
Quoted 1 line
96 pr_warn("NMI backtrace for cpu %d skipped: idling at pc %pS\n",
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation