[PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Zhen Lei]

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)) {
+	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));
-- 
2.25.1

[PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Zhen Lei]

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)) {
+	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));
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Catalin Marinas]

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

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Catalin Marinas]

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

-- 
Catalin

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Will Deacon]

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

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Will Deacon]

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

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Will Deacon]

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

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Will Deacon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

Re: [PATCH v2] arm64: add the printing of tpidr_elx in __show_regs()

[Leizhen (ThunderTown)]

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

-- 
Regards,
  Zhen Lei

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel