| Message ID | 20220316062408.1113-1-thunder.leizhen@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64: add the printing of tpidr_elx in __show_regs() | expand |
On Wed, 16 Mar 2022 14:24:08 +0800, Zhen Lei wrote: > Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access > using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use > tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize > pcpu access. However, when performing reverse execution based on the > registers and the memory contents in kdump, this information is sometimes > required if there is a pcpu access. > > [...] Applied to arm64 (for-next/misc), thanks! [1/1] arm64: add the printing of tpidr_elx in __show_regs() https://git.kernel.org/arm64/c/9ec393c812f2
On Wed, Mar 16, 2022 at 02:24:08PM +0800, Zhen Lei wrote: > Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access > using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use > tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize > pcpu access. However, when performing reverse execution based on the > registers and the memory contents in kdump, this information is sometimes > required if there is a pcpu access. > > Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > --- > arch/arm64/kernel/process.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > v1 --> v2: > Directly print the tpidr_elx register of the current exception level. > Avoid coupling with the implementation of 'my_cpu_offset'. > > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > index 5369e649fa79ff8..738932e6fa4e947 100644 > --- a/arch/arm64/kernel/process.c > +++ b/arch/arm64/kernel/process.c > @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) > show_regs_print_info(KERN_DEFAULT); > print_pstate(regs); > > + switch (read_sysreg(CurrentEL)) { This should use is_kernel_in_hyp_mode() to detect if we're running at El2. > + case CurrentEL_EL1: > + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); > + break; > + case CurrentEL_EL2: > + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); > + break; > + default: > + break; > + } I think this path can be triggered directly from usermode, so we really shouldn't be printing raw kernel virtual addresses here. Will
On Wed, Apr 27, 2022 at 07:40:15PM +0100, Catalin Marinas wrote: > On Wed, 16 Mar 2022 14:24:08 +0800, Zhen Lei wrote: > > Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access > > using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use > > tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize > > pcpu access. However, when performing reverse execution based on the > > registers and the memory contents in kdump, this information is sometimes > > required if there is a pcpu access. > > > > [...] > > Applied to arm64 (for-next/misc), thanks! > > [1/1] arm64: add the printing of tpidr_elx in __show_regs() > https://git.kernel.org/arm64/c/9ec393c812f2 heh, I wasn't expecting you to apply this, but also hadn't realised nobody had reviewed it. I've replied with my concerns, so please can you drop it for now? Will
On 2022/4/28 18:21, Will Deacon wrote: > On Wed, Mar 16, 2022 at 02:24:08PM +0800, Zhen Lei wrote: >> Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access >> using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use >> tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize >> pcpu access. However, when performing reverse execution based on the >> registers and the memory contents in kdump, this information is sometimes >> required if there is a pcpu access. >> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >> --- >> arch/arm64/kernel/process.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> v1 --> v2: >> Directly print the tpidr_elx register of the current exception level. >> Avoid coupling with the implementation of 'my_cpu_offset'. >> >> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >> index 5369e649fa79ff8..738932e6fa4e947 100644 >> --- a/arch/arm64/kernel/process.c >> +++ b/arch/arm64/kernel/process.c >> @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) >> show_regs_print_info(KERN_DEFAULT); >> print_pstate(regs); >> >> + switch (read_sysreg(CurrentEL)) { > > This should use is_kernel_in_hyp_mode() to detect if we're running at El2. > >> + case CurrentEL_EL1: >> + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); >> + break; >> + case CurrentEL_EL2: >> + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); >> + break; >> + default: >> + break; >> + } > > I think this path can be triggered directly from usermode, so we really > shouldn't be printing raw kernel virtual addresses here. I run echo c > /proc/sysrq-trigger and didn't trigger this path, but maybe there's another way. Analysis from the other side, except for the instruction address, all generic registers r0-r31 is output as raw. There's also an opportunity to contain the instruction address. So how about: + if (oops_in_progress) + printk("tpidr : %016lx\n", __my_cpu_offset); > > Will > . >
On 2022/4/28 19:07, Leizhen (ThunderTown) wrote: > > > On 2022/4/28 18:21, Will Deacon wrote: >> On Wed, Mar 16, 2022 at 02:24:08PM +0800, Zhen Lei wrote: >>> Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access >>> using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use >>> tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize >>> pcpu access. However, when performing reverse execution based on the >>> registers and the memory contents in kdump, this information is sometimes >>> required if there is a pcpu access. >>> >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>> --- >>> arch/arm64/kernel/process.c | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> v1 --> v2: >>> Directly print the tpidr_elx register of the current exception level. >>> Avoid coupling with the implementation of 'my_cpu_offset'. >>> >>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>> index 5369e649fa79ff8..738932e6fa4e947 100644 >>> --- a/arch/arm64/kernel/process.c >>> +++ b/arch/arm64/kernel/process.c >>> @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) >>> show_regs_print_info(KERN_DEFAULT); >>> print_pstate(regs); >>> >>> + switch (read_sysreg(CurrentEL)) { >> >> This should use is_kernel_in_hyp_mode() to detect if we're running at El2. static inline bool is_kernel_in_hyp_mode(void) { return read_sysreg(CurrentEL) == CurrentEL_EL2; } I think it's more intuitive to use "switch (read_sysreg(CurrentEL))". >> >>> + case CurrentEL_EL1: >>> + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); >>> + break; >>> + case CurrentEL_EL2: >>> + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); >>> + break; >>> + default: >>> + break; >>> + } >> >> I think this path can be triggered directly from usermode, so we really >> shouldn't be printing raw kernel virtual addresses here. > > I run echo c > /proc/sysrq-trigger and didn't trigger this path, but maybe > there's another way. Analysis from the other side, except for the instruction > address, all generic registers r0-r31 is output as raw. There's also an > opportunity to contain the instruction address. On second thought, there seemed to be nothing wrong with it. The user need to have capable() first. Then the address of the perpcu memory is not static, the memory is dynamically allocated, exposing it is no different than exposing sp. > > So how about: > + if (oops_in_progress) > + printk("tpidr : %016lx\n", __my_cpu_offset); > >> >> Will >> . >> >
On 2022/4/28 20:03, Leizhen (ThunderTown) wrote: > > > On 2022/4/28 19:07, Leizhen (ThunderTown) wrote: >> >> >> On 2022/4/28 18:21, Will Deacon wrote: >>> On Wed, Mar 16, 2022 at 02:24:08PM +0800, Zhen Lei wrote: >>>> Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access >>>> using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use >>>> tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize >>>> pcpu access. However, when performing reverse execution based on the >>>> registers and the memory contents in kdump, this information is sometimes >>>> required if there is a pcpu access. >>>> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>> --- >>>> arch/arm64/kernel/process.c | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> v1 --> v2: >>>> Directly print the tpidr_elx register of the current exception level. >>>> Avoid coupling with the implementation of 'my_cpu_offset'. >>>> >>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>>> index 5369e649fa79ff8..738932e6fa4e947 100644 >>>> --- a/arch/arm64/kernel/process.c >>>> +++ b/arch/arm64/kernel/process.c >>>> @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) >>>> show_regs_print_info(KERN_DEFAULT); >>>> print_pstate(regs); >>>> >>>> + switch (read_sysreg(CurrentEL)) { >>> >>> This should use is_kernel_in_hyp_mode() to detect if we're running at El2. > > static inline bool is_kernel_in_hyp_mode(void) > { > return read_sysreg(CurrentEL) == CurrentEL_EL2; > } > > I think it's more intuitive to use "switch (read_sysreg(CurrentEL))". > >>> >>>> + case CurrentEL_EL1: >>>> + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); >>>> + break; >>>> + case CurrentEL_EL2: >>>> + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); >>>> + break; >>>> + default: >>>> + break; >>>> + } >>> >>> I think this path can be triggered directly from usermode, so we really >>> shouldn't be printing raw kernel virtual addresses here. >> >> I run echo c > /proc/sysrq-trigger and didn't trigger this path, but maybe >> there's another way. Analysis from the other side, except for the instruction >> address, all generic registers r0-r31 is output as raw. There's also an >> opportunity to contain the instruction address. > > On second thought, there seemed to be nothing wrong with it. The user need > to have capable() first. Then the address of the perpcu memory is not static, Sorry, __my_cpu_offset is just an offset. > the memory is dynamically allocated, exposing it is no different than exposing sp. > >> >> So how about: >> + if (oops_in_progress) >> + printk("tpidr : %016lx\n", __my_cpu_offset); >> >>> >>> Will >>> . >>> >> >
On Thu, Apr 28, 2022 at 08:03:50PM +0800, Leizhen (ThunderTown) wrote: > > > On 2022/4/28 19:07, Leizhen (ThunderTown) wrote: > > > > > > On 2022/4/28 18:21, Will Deacon wrote: > >> On Wed, Mar 16, 2022 at 02:24:08PM +0800, Zhen Lei wrote: > >>> Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access > >>> using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use > >>> tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize > >>> pcpu access. However, when performing reverse execution based on the > >>> registers and the memory contents in kdump, this information is sometimes > >>> required if there is a pcpu access. > >>> > >>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> > >>> --- > >>> arch/arm64/kernel/process.c | 11 +++++++++++ > >>> 1 file changed, 11 insertions(+) > >>> > >>> v1 --> v2: > >>> Directly print the tpidr_elx register of the current exception level. > >>> Avoid coupling with the implementation of 'my_cpu_offset'. > >>> > >>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c > >>> index 5369e649fa79ff8..738932e6fa4e947 100644 > >>> --- a/arch/arm64/kernel/process.c > >>> +++ b/arch/arm64/kernel/process.c > >>> @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) > >>> show_regs_print_info(KERN_DEFAULT); > >>> print_pstate(regs); > >>> > >>> + switch (read_sysreg(CurrentEL)) { > >> > >> This should use is_kernel_in_hyp_mode() to detect if we're running at El2. > > static inline bool is_kernel_in_hyp_mode(void) > { > return read_sysreg(CurrentEL) == CurrentEL_EL2; > } > > I think it's more intuitive to use "switch (read_sysreg(CurrentEL))". No, I disagree with you here, sorry. > >>> + case CurrentEL_EL1: > >>> + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); > >>> + break; > >>> + case CurrentEL_EL2: > >>> + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); > >>> + break; > >>> + default: > >>> + break; > >>> + } > >> > >> I think this path can be triggered directly from usermode, so we really > >> shouldn't be printing raw kernel virtual addresses here. > > > > I run echo c > /proc/sysrq-trigger and didn't trigger this path, but maybe > > there's another way. Analysis from the other side, except for the instruction > > address, all generic registers r0-r31 is output as raw. There's also an > > opportunity to contain the instruction address. > > On second thought, there seemed to be nothing wrong with it. The user need > to have capable() first. Then the address of the perpcu memory is not static, > the memory is dynamically allocated, exposing it is no different than exposing sp. If show_unhandled_signals is set, then I think any fatal signal takes this path, no? Will
On 2022/4/28 21:13, Will Deacon wrote: > On Thu, Apr 28, 2022 at 08:03:50PM +0800, Leizhen (ThunderTown) wrote: >> >> >> On 2022/4/28 19:07, Leizhen (ThunderTown) wrote: >>> >>> >>> On 2022/4/28 18:21, Will Deacon wrote: >>>> On Wed, Mar 16, 2022 at 02:24:08PM +0800, Zhen Lei wrote: >>>>> Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access >>>>> using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use >>>>> tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize >>>>> pcpu access. However, when performing reverse execution based on the >>>>> registers and the memory contents in kdump, this information is sometimes >>>>> required if there is a pcpu access. >>>>> >>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> >>>>> --- >>>>> arch/arm64/kernel/process.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> v1 --> v2: >>>>> Directly print the tpidr_elx register of the current exception level. >>>>> Avoid coupling with the implementation of 'my_cpu_offset'. >>>>> >>>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c >>>>> index 5369e649fa79ff8..738932e6fa4e947 100644 >>>>> --- a/arch/arm64/kernel/process.c >>>>> +++ b/arch/arm64/kernel/process.c >>>>> @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) >>>>> show_regs_print_info(KERN_DEFAULT); >>>>> print_pstate(regs); >>>>> >>>>> + switch (read_sysreg(CurrentEL)) { >>>> >>>> This should use is_kernel_in_hyp_mode() to detect if we're running at El2. >> >> static inline bool is_kernel_in_hyp_mode(void) >> { >> return read_sysreg(CurrentEL) == CurrentEL_EL2; >> } >> >> I think it's more intuitive to use "switch (read_sysreg(CurrentEL))". > > No, I disagree with you here, sorry. OK. Change it to the following form in v3? + if (is_kernel_in_hyp_mode()) + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); + else + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); By the way, Is there a requirement on the case of register names? I see some use TPIDR_EL1 and some use tpidr_el1. > >>>>> + case CurrentEL_EL1: >>>>> + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); >>>>> + break; >>>>> + case CurrentEL_EL2: >>>>> + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); >>>>> + break; >>>>> + default: >>>>> + break; >>>>> + } >>>> >>>> I think this path can be triggered directly from usermode, so we really >>>> shouldn't be printing raw kernel virtual addresses here. >>> >>> I run echo c > /proc/sysrq-trigger and didn't trigger this path, but maybe >>> there's another way. Analysis from the other side, except for the instruction >>> address, all generic registers r0-r31 is output as raw. There's also an >>> opportunity to contain the instruction address. >> >> On second thought, there seemed to be nothing wrong with it. The user need >> to have capable() first. Then the address of the perpcu memory is not static, >> the memory is dynamically allocated, exposing it is no different than exposing sp. > > If show_unhandled_signals is set, then I think any fatal signal takes this > path, no? I looked at the implementation of arm64_show_signal(), and there must be a chance to take this path. But last night, I came to my senses, the value stored in tpidr is actually an offset, not an address. So there should be no kernel address leakage problem. > > Will > . >
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index 5369e649fa79ff8..738932e6fa4e947 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -216,6 +216,17 @@ void __show_regs(struct pt_regs *regs) show_regs_print_info(KERN_DEFAULT); print_pstate(regs); + switch (read_sysreg(CurrentEL)) { + case CurrentEL_EL1: + printk("tpidr_el1 : %016llx\n", read_sysreg(TPIDR_EL1)); + break; + case CurrentEL_EL2: + printk("tpidr_el2 : %016llx\n", read_sysreg(TPIDR_EL2)); + break; + default: + break; + } + if (!user_mode(regs)) { printk("pc : %pS\n", (void *)regs->pc); printk("lr : %pS\n", (void *)ptrauth_strip_insn_pac(lr));
Commit 7158627686f0 ("arm64: percpu: implement optimised pcpu access using tpidr_el1") and commit 6d99b68933fb ("arm64: alternatives: use tpidr_el2 on VHE hosts") use tpidr_elx to cache my_cpu_offset to optimize pcpu access. However, when performing reverse execution based on the registers and the memory contents in kdump, this information is sometimes required if there is a pcpu access. Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com> --- arch/arm64/kernel/process.c | 11 +++++++++++ 1 file changed, 11 insertions(+) v1 --> v2: Directly print the tpidr_elx register of the current exception level. Avoid coupling with the implementation of 'my_cpu_offset'.