Skip to contentSkip to conversation

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
#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
#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);
author
#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);
Reviewed-by
#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

#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
author
#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
author
#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
#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
build bot
#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

author
#9 lore raw

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

build bot
#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:

[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-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