| Message ID | 1455693365.3089.8.camel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 02/17/2016 12:46 PM, Balbir Singh wrote: >> > 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 Cant we use USER_REGION_ID directly ?
On Wed, Feb 17, 2016 at 01:33:32PM +0530, Anshuman Khandual wrote: > On 02/17/2016 12:46 PM, Balbir Singh wrote: > >> > 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 > > 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.
Balbir Singh <bsingharora@gmail.com> writes: >> 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 > > 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 <paulus@samba.org> > Signed-off-by: Balbir Singh <bsingharora@gmail.com> > --- > arch/powerpc/kernel/traps.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index b6becc7..4de4fe7 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -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; > + 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. > 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; > } > -- -aneesh
>> 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 <paulus@samba.org> >> Signed-off-by: Balbir Singh <bsingharora@gmail.com> <snip> > 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.
Balbir Singh <bsingharora@gmail.com> writes: >>> 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 <paulus@samba.org> >>> Signed-off-by: Balbir Singh <bsingharora@gmail.com> > > <snip> > >> 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 > 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
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index b6becc7..4de4fe7 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -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; }