Fix BUG_ON() reporting in real mode on powerpc

PATCH Balbir Singh 4 participants 8 messages Feb 17, 2016 – Feb 18, 2016 PowerPC

Share & tools

Balbir Singh

author

2016-02-17 04:43 UTC #0 lore raw

I ran into this issue while debugging an early boot problem. The system hit a BUG_ON() but report bug failed to print the line number and file name. The reason being that the system was running in real mode and report_bug() searches for addresses in the PAGE_OFFSET+ region

Signed-off-by Balbir Singh <[email protected]>

---
arch/powerpc/kernel/traps.c | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

arch/powerpc/kernel/traps.c +0 -0

index b6becc7..8f28120 100644
@@ -1148,6 +1148,7 @@ void __kprobes program_check_exception(struct pt_regs *regs)

goto bail;
}
if (reason & REASON_TRAP) {
+ unsigned long bugaddr;
/* Debugger is first in line to stop recursive faults in
* rcu_lock, notify_die, or atomic_notifier_call_chain */ if (debugger_bpt(regs))
@@ -1158,8 +1159,13 @@ void __kprobes program_check_exception(struct pt_regs *regs) == NOTIFY_STOP)
goto bail;

+ if (!(regs->msr & MSR_IR))
+ bugaddr = regs->nip + PAGE_OFFSET;
+ else
+ bugaddr = regs->nip;
+
if (!(regs->msr & MSR_PR) &&  /* not user-mode */
-     report_bug(regs->nip, regs) == BUG_TRAP_TYPE_WARN) {
+     report_bug(bugaddr, regs) == BUG_TRAP_TYPE_WARN) {
regs->nip += 4;
goto bail;
}
--
2.5.0

Paul Mackerras

2016-02-17 04:56 UTC #1 lore raw

Quoted 30 lines · Balbir Singh

From: Balbir Singh <[email protected]>
I ran into this issue while debugging an early boot problem. The system hit a BUG_ON() but report bug failed to print the line number and file name. The reason being that the system was running in real mode and report_bug() searches for addresses in the PAGE_OFFSET+ region
Suggested-by Paul Mackerras <[email protected]>
Signed-off-by Balbir Singh <[email protected]>
---
arch/powerpc/kernel/traps.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
arch/powerpc/kernel/traps.c +1 -0
index b6becc7..8f28120 100644
@@ -1148,6 +1148,7 @@ void __kprobes program_check_exception(struct pt_regs *regs)
  		goto bail;
  	}
  	if (reason & REASON_TRAP) {
+		unsigned long bugaddr;
  		/* Debugger is first in line to stop recursive faults in
  		 * rcu_lock, notify_die, or atomic_notifier_call_chain */
  		if (debugger_bpt(regs))
@@ -1158,8 +1159,13 @@ void __kprobes program_check_exception(struct pt_regs *regs)
  				== NOTIFY_STOP)
  			goto bail;

arch/powerpc/kernel/traps.c

Quoted 4 lines

arch/powerpc/kernel/traps.c +4 -0

+		if (!(regs->msr & MSR_IR))
+			bugaddr = regs->nip + PAGE_OFFSET;
+		else
+			bugaddr = regs->nip;

It might be a little better to do this:

		bugaddr = regs->nip;
		if (REGION_ID(bugaddr) == 0 && !(regs->msr & MSR_IR))
			bugaddr += PAGE_OFFSET;

It is possible to execute from addresses with the 0xc000... on top in real mode, because the CPU ignores the top 4 address bits in real mode.

Paul.

Balbir Singh

author

2016-02-17 07:16 UTC #2 lore raw

Quoted 10 lines

It might be a little better to do this:
		bugaddr = regs->nip;
		if (REGION_ID(bugaddr) == 0 && !(regs->msr & MSR_IR))
			bugaddr += PAGE_OFFSET;
It is possible to execute from addresses with the 0xc000... on top in real mode, because the CPU ignores the top 4 address bits in real mode.

Good catch! Thank you

Changelog:
Don't add PAGE_OFFSET blindly, check if REGION_ID is 0

Signed-off-by Balbir Singh <[email protected]>

---
arch/powerpc/kernel/traps.c | 7 ++++++-

 1 file changed, 6 insertions(+), 1 deletion(-)

arch/powerpc/kernel/traps.c +0 -0

index b6becc7..4de4fe7 100644
@@ -1148,6 +1148,7 @@ void __kprobes program_check_exception(struct pt_regs *regs)

goto bail;
}
if (reason & REASON_TRAP) {
+ unsigned long bugaddr;
/* Debugger is first in line to stop recursive faults in
* rcu_lock, notify_die, or atomic_notifier_call_chain */ if (debugger_bpt(regs))
@@ -1158,8 +1159,12 @@ void __kprobes program_check_exception(struct pt_regs *regs) == NOTIFY_STOP)
goto bail;

+ bugaddr = regs->nip;
+ if ((REGION_ID(bugaddr) == 0) && !(regs->msr & MSR_IR))
+ bugaddr += PAGE_OFFSET;
+
if (!(regs->msr & MSR_PR) &&  /* not user-mode */
-     report_bug(regs->nip, regs) == BUG_TRAP_TYPE_WARN) {
+     report_bug(bugaddr, regs) == BUG_TRAP_TYPE_WARN) {
regs->nip += 4;
goto bail;
}
--
2.5.0

Anshuman Khandual

2016-02-17 08:34 UTC #3 lore raw

Quoted 6 lines · Balbir Singh

Quoted 6 lines
Quoted 6 lines
It might be a little better to do this:
		bugaddr = regs->nip;
		if (REGION_ID(bugaddr) == 0 && !(regs->msr & MSR_IR))
			bugaddr += PAGE_OFFSET;

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
It is possible to execute from addresses with the 0xc000... on top in real mode, because the CPU ignores the top 4 address bits in real mode.
Good catch! Thank you
Changelog:
Don't add PAGE_OFFSET blindly, check if REGION_ID is 0

Cant we use USER_REGION_ID directly ?

Paul Mackerras

2016-02-17 08:23 UTC #4 lore raw

Quoted 10 lines · Anshuman Khandual

Quoted 9 lines · Balbir Singh
Quoted 9 lines
Quoted 9 lines
It might be a little better to do this:
		bugaddr = regs->nip;
		if (REGION_ID(bugaddr) == 0 && !(regs->msr & MSR_IR))
			bugaddr += PAGE_OFFSET;
It is possible to execute from addresses with the 0xc000... on top in real mode, because the CPU ignores the top 4 address bits in real mode.

Quoted 6 lines

Quoted 4 lines
Good catch! Thank you
Changelog:
Don't add PAGE_OFFSET blindly, check if REGION_ID is 0
Cant we use USER_REGION_ID directly ?

If we use USER_REGION_ID then the reader needs to know that the user region is region 0 to understand the code. Thus I think it is clearer to use REGION_ID(bugaddr) == 0. Whether or not the address is a user region address is not really relevant to the question of whether it's a physical address being accessed directly in real mode vs. a kernel virtual address, which is what we're trying to determine.

Paul.

Aneesh Kumar K.V

2016-02-17 15:16 UTC #5 lore raw

In reply to Balbir Singh

arch/powerpc/kernel/traps.c void __kprobes program_check_exception(struct pt_re=

Quoted 32 lines · Balbir Singh

Quoted 9 lines
It might be a little better to do this:
=20
		bugaddr =3D regs->nip;
		if (REGION_ID(bugaddr) =3D=3D 0 && !(regs->msr & MSR_IR))
			bugaddr +=3D PAGE_OFFSET;
=20
It is possible to execute from addresses with the 0xc000... on top in real mode, because the CPU ignores the top 4 address bits in real mode.
Good catch! Thank you
Changelog:
Don't add PAGE_OFFSET blindly, check if REGION_ID is 0
I ran into this issue while debugging an early boot problem. The system hit a BUG_ON() but report bug failed to print the line number and file name. The reason being that the system was running in real mode and report_bug() searches for addresses in the PAGE_OFFSET+ region
Suggested-by Paul Mackerras <[email protected]>
Signed-off-by Balbir Singh <[email protected]>
---
=C2=A0arch/powerpc/kernel/traps.c | 7 ++++++- =C2=A01 file changed, 6 insertions(+), 1 deletion(-)
arch/powerpc/kernel/traps.c +0 -0
index b6becc7..4de4fe7 100644
@@ -1148,6 +1148,7 @@ void __kprobes program_check_exception(struct pt_re=

gs *regs)

arch/powerpc/kernel/traps.c void __kprobes program_check_exception(struct pt_r=

Quoted 8 lines

arch/powerpc/kernel/traps.c +1 -0
 =C2=A0		goto bail;
 =C2=A0	}
 =C2=A0	if (reason & REASON_TRAP) {
+		unsigned long bugaddr;
=C2=A0 /* Debugger is first in line to stop recursive faults in
=C2=A0 =C2=A0* rcu_lock, notify_die, or atomic_notifier_call_chain */
=C2=A0 if (debugger_bpt(regs))
arch/powerpc/kernel/traps.c +0 -0
@@ -1158,8 +1159,12 @@ void __kprobes program_check_exception(struct pt_r=

egs *regs)

arch/powerpc/kernel/traps.c

Quoted 7 lines

arch/powerpc/kernel/traps.c +0 -0

 =C2=A0				=3D=3D NOTIFY_STOP)
 =C2=A0			goto bail;

=C2=A0

arch/powerpc/kernel/traps.c +4 -0

+		bugaddr =3D regs->nip;
+		if ((REGION_ID(bugaddr) =3D=3D 0) && !(regs->msr & MSR_IR))
+			bugaddr +=3D PAGE_OFFSET;
+

Can we add some comments around this. When i looked at this first, i was wondering how nip can be in user region. But then realized that what we are checking here is kernel address used in real mode. The use of REGION_ID eventhough simpler is confusing. Hence adding the comment with details Paul mentioned in email will help.

arch/powerpc/kernel/traps.c

Quoted 2 lines

=C2=A0 if (!(regs->msr & MSR_PR) &&=C2=A0=C2=A0/* not user-mode */
arch/powerpc/kernel/traps.c +0 -1
-		=C2=A0=C2=A0=C2=A0=C2=A0report_bug(regs->nip, regs) =3D=3D BUG_TRAP_TY=

PE_WARN) {

arch/powerpc/kernel/traps.c

Quoted 1 line

arch/powerpc/kernel/traps.c +1 -0

+		=C2=A0=C2=A0=C2=A0=C2=A0report_bug(bugaddr, regs) =3D=3D BUG_TRAP_TYPE=

_WARN) {

Quoted 4 lines

=C2=A0 regs->nip +=3D 4;
=C2=A0 goto bail;
=C2=A0 }
--=C2=A0

-aneesh

Balbir Singh

author

2016-02-18 00:26 UTC #6 lore raw

Quoted 11 lines

Quoted 11 lines
Changelog:
Don't add PAGE_OFFSET blindly, check if REGION_ID is 0
I ran into this issue while debugging an early boot problem. The system hit a BUG_ON() but report bug failed to print the line number and file name. The reason being that the system was running in real mode and report_bug() searches for addresses in the PAGE_OFFSET+ region
Suggested-by Paul Mackerras <[email protected]>
Signed-off-by Balbir Singh <[email protected]>

<snip>

Quoted 7 lines

Can we add some comments around this. When i looked at this first, i was wondering how nip can be in user region. But then realized that what we are checking here is kernel address used in real mode. The use of REGION_ID eventhough simpler is confusing. Hence adding the comment with details Paul mentioned in email will help.

I've tried and covered it in the changelog, I thought a code comment would make sense for the very non obvious cases and not repeat what the code does as comment

Balbir Singh.

Aneesh Kumar K.V

2016-02-18 02:41 UTC #7 lore raw

Quoted 21 lines · Balbir Singh

Quoted 11 lines
Quoted 11 lines
Changelog:
Don't add PAGE_OFFSET blindly, check if REGION_ID is 0
I ran into this issue while debugging an early boot problem. The system hit a BUG_ON() but report bug failed to print the line number and file name. The reason being that the system was running in real mode and report_bug() searches for addresses in the PAGE_OFFSET+ region
Suggested-by Paul Mackerras <[email protected]>
Signed-off-by Balbir Singh <[email protected]>
<snip>
Quoted 7 lines
Can we add some comments around this. When i looked at this first, i was wondering how nip can be in user region. But then realized that what we are checking here is kernel address used in real mode. The use of REGION_ID eventhough simpler is confusing. Hence adding the comment with details Paul mentioned in email will help.

Quoted 4 lines

I've tried and covered it in the changelog, I thought a code comment would make sense for the very non obvious cases and not repeat what the code does as comment

The use of REGION_ID indicate that you are checking for region. Hence the suggestion. Looking at this again, I suggest we should add a new macro or will have to open code it. Because in the radix series we make REGION_ID a hash config thing and this is generic stuff.

-aneesh