[PATCH v2 0/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[PATCH v2 0/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[Zhen Lei]

v1 --> v2:
1. Cleanup sparse warnings.

Zhen Lei (2):
  ARM: Fix some check warnings of tool sparse
  ARM: Make the dumped instructions are consistent with the disassembled
    ones

 arch/arm/kernel/traps.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Zhen Lei]

Fix the following warnings:
 warning: incorrect type in initializer (different address spaces)
    expected unsigned short [noderef] __user *register __p
    got unsigned short [usertype] *
 warning: cast removes address space '__user' of expression

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/arm/kernel/traps.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 20b2db6dcd1ced7..34aa80c09c508c1 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
 			}
 		} else {
 			if (thumb)
-				bad = get_user(val, &((u16 *)addr)[i]);
+				bad = get_user(val, &((u16 __user *)addr)[i]);
 			else
-				bad = get_user(val, &((u32 *)addr)[i]);
+				bad = get_user(val, &((u32 __user *)addr)[i]);
 		}
 
 		if (!bad)
@@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
 	if (processor_mode(regs) == SVC_MODE) {
 #ifdef CONFIG_THUMB2_KERNEL
 		if (thumb_mode(regs)) {
-			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
+			instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
 			if (is_wide_instruction(instr)) {
 				u16 inst2;
-				inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
+				inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]);
 				instr = __opcode_thumb32_compose(instr, inst2);
 			}
 		} else
 #endif
-			instr = __mem_to_opcode_arm(*(u32 *) pc);
+			instr = __mem_to_opcode_arm(*(__force u32 *) pc);
 	} else if (thumb_mode(regs)) {
 		if (get_user(instr, (u16 __user *)pc))
 			goto die_sig;
-- 
2.25.1

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Ard Biesheuvel]

On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>
> Fix the following warnings:
>  warning: incorrect type in initializer (different address spaces)
>     expected unsigned short [noderef] __user *register __p
>     got unsigned short [usertype] *
>  warning: cast removes address space '__user' of expression
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  arch/arm/kernel/traps.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>                         }
>                 } else {
>                         if (thumb)
> -                               bad = get_user(val, &((u16 *)addr)[i]);
> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
>                         else
> -                               bad = get_user(val, &((u32 *)addr)[i]);
> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
>                 }
>
>                 if (!bad)
> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>         if (processor_mode(regs) == SVC_MODE) {
>  #ifdef CONFIG_THUMB2_KERNEL
>                 if (thumb_mode(regs)) {
> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);

Shouldn't this be __user as well? (and below)

>                         if (is_wide_instruction(instr)) {
>                                 u16 inst2;
> -                               inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
> +                               inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]);
>                                 instr = __opcode_thumb32_compose(instr, inst2);
>                         }
>                 } else
>  #endif
> -                       instr = __mem_to_opcode_arm(*(u32 *) pc);
> +                       instr = __mem_to_opcode_arm(*(__force u32 *) pc);
>         } else if (thumb_mode(regs)) {
>                 if (get_user(instr, (u16 __user *)pc))
>                         goto die_sig;
> --
> 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 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/10 18:20, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>
>> Fix the following warnings:
>>  warning: incorrect type in initializer (different address spaces)
>>     expected unsigned short [noderef] __user *register __p
>>     got unsigned short [usertype] *
>>  warning: cast removes address space '__user' of expression
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  arch/arm/kernel/traps.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>                         }
>>                 } else {
>>                         if (thumb)
>> -                               bad = get_user(val, &((u16 *)addr)[i]);
>> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
>>                         else
>> -                               bad = get_user(val, &((u32 *)addr)[i]);
>> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
>>                 }
>>
>>                 if (!bad)
>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>         if (processor_mode(regs) == SVC_MODE) {
>>  #ifdef CONFIG_THUMB2_KERNEL
>>                 if (thumb_mode(regs)) {
>> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
> 
> Shouldn't this be __user as well? (and below)

unsigned int instr;
void __user *pc;

The __user can clear the warning, but a new warning will be generated.

instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
      ^new                           ^old

arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression

> 
>>                         if (is_wide_instruction(instr)) {
>>                                 u16 inst2;
>> -                               inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
>> +                               inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]);
>>                                 instr = __opcode_thumb32_compose(instr, inst2);
>>                         }
>>                 } else
>>  #endif
>> -                       instr = __mem_to_opcode_arm(*(u32 *) pc);
>> +                       instr = __mem_to_opcode_arm(*(__force u32 *) pc);
>>         } else if (thumb_mode(regs)) {
>>                 if (get_user(instr, (u16 __user *)pc))
>>                         goto die_sig;
>> --
>> 2.25.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Ard Biesheuvel]

On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
>
>
>
> On 2022/10/10 18:20, Ard Biesheuvel wrote:
> > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >>
> >> Fix the following warnings:
> >>  warning: incorrect type in initializer (different address spaces)
> >>     expected unsigned short [noderef] __user *register __p
> >>     got unsigned short [usertype] *
> >>  warning: cast removes address space '__user' of expression
> >>
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >> ---
> >>  arch/arm/kernel/traps.c | 10 +++++-----
> >>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
> >> --- a/arch/arm/kernel/traps.c
> >> +++ b/arch/arm/kernel/traps.c
> >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> >>                         }
> >>                 } else {
> >>                         if (thumb)
> >> -                               bad = get_user(val, &((u16 *)addr)[i]);
> >> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
> >>                         else
> >> -                               bad = get_user(val, &((u32 *)addr)[i]);
> >> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
> >>                 }
> >>
> >>                 if (!bad)
> >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
> >>         if (processor_mode(regs) == SVC_MODE) {
> >>  #ifdef CONFIG_THUMB2_KERNEL
> >>                 if (thumb_mode(regs)) {
> >> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> >> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
> >
> > Shouldn't this be __user as well? (and below)
>
> unsigned int instr;
> void __user *pc;
>
> The __user can clear the warning, but a new warning will be generated.
>
> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>       ^new                           ^old
>
> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
>

This is because dereferencing a __user pointer is not permitted.

So this code should be using get_kernel_nofault() here not a plain
dereference of PC. So better to fix that properly instead of papering
over it with a __force cast just to make sparse happy.


> >
> >>                         if (is_wide_instruction(instr)) {
> >>                                 u16 inst2;
> >> -                               inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
> >> +                               inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]);
> >>                                 instr = __opcode_thumb32_compose(instr, inst2);
> >>                         }
> >>                 } else
> >>  #endif
> >> -                       instr = __mem_to_opcode_arm(*(u32 *) pc);
> >> +                       instr = __mem_to_opcode_arm(*(__force u32 *) pc);
> >>         } else if (thumb_mode(regs)) {
> >>                 if (get_user(instr, (u16 __user *)pc))
> >>                         goto die_sig;
> >> --
> >> 2.25.1
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > .
> >
>
> --
> Regards,
>   Zhen Lei

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Russell King (Oracle)]

On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
> >
> >
> >
> > On 2022/10/10 18:20, Ard Biesheuvel wrote:
> > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
> > >>
> > >> Fix the following warnings:
> > >>  warning: incorrect type in initializer (different address spaces)
> > >>     expected unsigned short [noderef] __user *register __p
> > >>     got unsigned short [usertype] *
> > >>  warning: cast removes address space '__user' of expression
> > >>
> > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > >> ---
> > >>  arch/arm/kernel/traps.c | 10 +++++-----
> > >>  1 file changed, 5 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
> > >> --- a/arch/arm/kernel/traps.c
> > >> +++ b/arch/arm/kernel/traps.c
> > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> > >>                         }
> > >>                 } else {
> > >>                         if (thumb)
> > >> -                               bad = get_user(val, &((u16 *)addr)[i]);
> > >> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
> > >>                         else
> > >> -                               bad = get_user(val, &((u32 *)addr)[i]);
> > >> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
> > >>                 }
> > >>
> > >>                 if (!bad)
> > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
> > >>         if (processor_mode(regs) == SVC_MODE) {
> > >>  #ifdef CONFIG_THUMB2_KERNEL
> > >>                 if (thumb_mode(regs)) {
> > >> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> > >> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
> > >
> > > Shouldn't this be __user as well? (and below)
> >
> > unsigned int instr;
> > void __user *pc;
> >
> > The __user can clear the warning, but a new warning will be generated.
> >
> > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> >       ^new                           ^old
> >
> > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
> >
> 
> This is because dereferencing a __user pointer is not permitted.
> 
> So this code should be using get_kernel_nofault() here not a plain
> dereference of PC. So better to fix that properly instead of papering
> over it with a __force cast just to make sparse happy.

Why? We won't get here unless the PC can be dereferenced. If it's not
able to be dereferenced, then we'd be dealing with a prefetch abort.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Ard Biesheuvel]

On Mon, 10 Oct 2022 at 18:08, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote:
> > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
> > <thunder.leizhen@huawei.com> wrote:
> > >
> > >
> > >
> > > On 2022/10/10 18:20, Ard Biesheuvel wrote:
> > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
> > > >>
> > > >> Fix the following warnings:
> > > >>  warning: incorrect type in initializer (different address spaces)
> > > >>     expected unsigned short [noderef] __user *register __p
> > > >>     got unsigned short [usertype] *
> > > >>  warning: cast removes address space '__user' of expression
> > > >>
> > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > > >> ---
> > > >>  arch/arm/kernel/traps.c | 10 +++++-----
> > > >>  1 file changed, 5 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
> > > >> --- a/arch/arm/kernel/traps.c
> > > >> +++ b/arch/arm/kernel/traps.c
> > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> > > >>                         }
> > > >>                 } else {
> > > >>                         if (thumb)
> > > >> -                               bad = get_user(val, &((u16 *)addr)[i]);
> > > >> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
> > > >>                         else
> > > >> -                               bad = get_user(val, &((u32 *)addr)[i]);
> > > >> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
> > > >>                 }
> > > >>
> > > >>                 if (!bad)
> > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
> > > >>         if (processor_mode(regs) == SVC_MODE) {
> > > >>  #ifdef CONFIG_THUMB2_KERNEL
> > > >>                 if (thumb_mode(regs)) {
> > > >> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> > > >> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
> > > >
> > > > Shouldn't this be __user as well? (and below)
> > >
> > > unsigned int instr;
> > > void __user *pc;
> > >
> > > The __user can clear the warning, but a new warning will be generated.
> > >
> > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> > >       ^new                           ^old
> > >
> > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
> > >
> >
> > This is because dereferencing a __user pointer is not permitted.
> >
> > So this code should be using get_kernel_nofault() here not a plain
> > dereference of PC. So better to fix that properly instead of papering
> > over it with a __force cast just to make sparse happy.
>
> Why? We won't get here unless the PC can be dereferenced. If it's not
> able to be dereferenced, then we'd be dealing with a prefetch abort.
>

If that is guaranteed (i.e., there is no way we might be racing with a
module unload on another CPU or something like that), then I agree
that dereferencing PC is fine.

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Russell King (Oracle)]

On Mon, Oct 10, 2022 at 06:14:56PM +0200, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 18:08, Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> >
> > On Mon, Oct 10, 2022 at 01:06:19PM +0200, Ard Biesheuvel wrote:
> > > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
> > > <thunder.leizhen@huawei.com> wrote:
> > > >
> > > >
> > > >
> > > > On 2022/10/10 18:20, Ard Biesheuvel wrote:
> > > > > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
> > > > >>
> > > > >> Fix the following warnings:
> > > > >>  warning: incorrect type in initializer (different address spaces)
> > > > >>     expected unsigned short [noderef] __user *register __p
> > > > >>     got unsigned short [usertype] *
> > > > >>  warning: cast removes address space '__user' of expression
> > > > >>
> > > > >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > > > >> ---
> > > > >>  arch/arm/kernel/traps.c | 10 +++++-----
> > > > >>  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > >>
> > > > >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > > > >> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
> > > > >> --- a/arch/arm/kernel/traps.c
> > > > >> +++ b/arch/arm/kernel/traps.c
> > > > >> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> > > > >>                         }
> > > > >>                 } else {
> > > > >>                         if (thumb)
> > > > >> -                               bad = get_user(val, &((u16 *)addr)[i]);
> > > > >> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
> > > > >>                         else
> > > > >> -                               bad = get_user(val, &((u32 *)addr)[i]);
> > > > >> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
> > > > >>                 }
> > > > >>
> > > > >>                 if (!bad)
> > > > >> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
> > > > >>         if (processor_mode(regs) == SVC_MODE) {
> > > > >>  #ifdef CONFIG_THUMB2_KERNEL
> > > > >>                 if (thumb_mode(regs)) {
> > > > >> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> > > > >> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
> > > > >
> > > > > Shouldn't this be __user as well? (and below)
> > > >
> > > > unsigned int instr;
> > > > void __user *pc;
> > > >
> > > > The __user can clear the warning, but a new warning will be generated.
> > > >
> > > > instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> > > >       ^new                           ^old
> > > >
> > > > arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
> > > >
> > >
> > > This is because dereferencing a __user pointer is not permitted.
> > >
> > > So this code should be using get_kernel_nofault() here not a plain
> > > dereference of PC. So better to fix that properly instead of papering
> > > over it with a __force cast just to make sparse happy.
> >
> > Why? We won't get here unless the PC can be dereferenced. If it's not
> > able to be dereferenced, then we'd be dealing with a prefetch abort.
> >
> 
> If that is guaranteed (i.e., there is no way we might be racing with a
> module unload on another CPU or something like that), then I agree
> that dereferencing PC is fine.

If we get here for an instruction in a module that's being unloaded, we
have way bigger problems. We shouldn't be executing code in a module
being unloaded in the first place. That becomes a case of "deserves to
oops".

The more likely case would be a prefetch abort when the page is
unmapped. You'd have to try pretty hard to get an undef to race with
a module unload.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/10 19:06, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>>
>>
>>
>> On 2022/10/10 18:20, Ard Biesheuvel wrote:
>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>
>>>> Fix the following warnings:
>>>>  warning: incorrect type in initializer (different address spaces)
>>>>     expected unsigned short [noderef] __user *register __p
>>>>     got unsigned short [usertype] *
>>>>  warning: cast removes address space '__user' of expression
>>>>
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>>  arch/arm/kernel/traps.c | 10 +++++-----
>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
>>>> --- a/arch/arm/kernel/traps.c
>>>> +++ b/arch/arm/kernel/traps.c
>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>>                         }
>>>>                 } else {
>>>>                         if (thumb)
>>>> -                               bad = get_user(val, &((u16 *)addr)[i]);
>>>> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
>>>>                         else
>>>> -                               bad = get_user(val, &((u32 *)addr)[i]);
>>>> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
>>>>                 }
>>>>
>>>>                 if (!bad)
>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>>>         if (processor_mode(regs) == SVC_MODE) {
>>>>  #ifdef CONFIG_THUMB2_KERNEL
>>>>                 if (thumb_mode(regs)) {
>>>> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
>>>
>>> Shouldn't this be __user as well? (and below)
>>
>> unsigned int instr;
>> void __user *pc;
>>
>> The __user can clear the warning, but a new warning will be generated.
>>
>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>       ^new                           ^old
>>
>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
>>
> 
> This is because dereferencing a __user pointer is not permitted.
> 
> So this code should be using get_kernel_nofault() here not a plain
> dereference of PC. So better to fix that properly instead of papering
> over it with a __force cast just to make sparse happy.

How about:
@@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
 asmlinkage void do_undefinstr(struct pt_regs *regs)
 {
        unsigned int instr;
-       void __user *pc;
+       void *pc;

-       pc = (void __user *)instruction_pointer(regs);
+       pc = (void *)instruction_pointer(regs);

        if (processor_mode(regs) == SVC_MODE) {
 #ifdef CONFIG_THUMB2_KERNEL
@@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
        }
 #endif
        arm_notify_die("Oops - undefined instruction", regs,
-                      SIGILL, ILL_ILLOPC, pc, 0, 6);
+                      SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
 }
 NOKPROBE_SYMBOL(do_undefinstr)


The 'pc' may come from kernel or user. And I found that all the get_user()
calls have already done type casts to the pc, except arm_notify_die().
I think the above changes are reasonable.

> 
> 
>>>
>>>>                         if (is_wide_instruction(instr)) {
>>>>                                 u16 inst2;
>>>> -                               inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
>>>> +                               inst2 = __mem_to_opcode_thumb16(((__force u16 *)pc)[1]);
>>>>                                 instr = __opcode_thumb32_compose(instr, inst2);
>>>>                         }
>>>>                 } else
>>>>  #endif
>>>> -                       instr = __mem_to_opcode_arm(*(u32 *) pc);
>>>> +                       instr = __mem_to_opcode_arm(*(__force u32 *) pc);
>>>>         } else if (thumb_mode(regs)) {
>>>>                 if (get_user(instr, (u16 __user *)pc))
>>>>                         goto die_sig;
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>> .
>>>
>>
>> --
>> Regards,
>>   Zhen Lei
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Russell King (Oracle)]

On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote:
> On 2022/10/10 19:06, Ard Biesheuvel wrote:
> > On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
> > <thunder.leizhen@huawei.com> wrote:
> >> On 2022/10/10 18:20, Ard Biesheuvel wrote:
> >>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >>>>
> >>>> Fix the following warnings:
> >>>>  warning: incorrect type in initializer (different address spaces)
> >>>>     expected unsigned short [noderef] __user *register __p
> >>>>     got unsigned short [usertype] *
> >>>>  warning: cast removes address space '__user' of expression
> >>>>
> >>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>>> ---
> >>>>  arch/arm/kernel/traps.c | 10 +++++-----
> >>>>  1 file changed, 5 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> >>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
> >>>> --- a/arch/arm/kernel/traps.c
> >>>> +++ b/arch/arm/kernel/traps.c
> >>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> >>>>                         }
> >>>>                 } else {
> >>>>                         if (thumb)
> >>>> -                               bad = get_user(val, &((u16 *)addr)[i]);
> >>>> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
> >>>>                         else
> >>>> -                               bad = get_user(val, &((u32 *)addr)[i]);
> >>>> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
> >>>>                 }
> >>>>
> >>>>                 if (!bad)
> >>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
> >>>>         if (processor_mode(regs) == SVC_MODE) {
> >>>>  #ifdef CONFIG_THUMB2_KERNEL
> >>>>                 if (thumb_mode(regs)) {
> >>>> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> >>>> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
> >>>
> >>> Shouldn't this be __user as well? (and below)
> >>
> >> unsigned int instr;
> >> void __user *pc;
> >>
> >> The __user can clear the warning, but a new warning will be generated.
> >>
> >> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> >>       ^new                           ^old
> >>
> >> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
> >>
> > 
> > This is because dereferencing a __user pointer is not permitted.
> > 
> > So this code should be using get_kernel_nofault() here not a plain
> > dereference of PC. So better to fix that properly instead of papering
> > over it with a __force cast just to make sparse happy.
> 
> How about:
> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>  {
>         unsigned int instr;
> -       void __user *pc;
> +       void *pc;
> 
> -       pc = (void __user *)instruction_pointer(regs);
> +       pc = (void *)instruction_pointer(regs);
> 
>         if (processor_mode(regs) == SVC_MODE) {
>  #ifdef CONFIG_THUMB2_KERNEL
> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>         }
>  #endif
>         arm_notify_die("Oops - undefined instruction", regs,
> -                      SIGILL, ILL_ILLOPC, pc, 0, 6);
> +                      SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>  }
>  NOKPROBE_SYMBOL(do_undefinstr)
> 
> 
> The 'pc' may come from kernel or user. And I found that all the get_user()
> calls have already done type casts to the pc, except arm_notify_die().
> I think the above changes are reasonable.

If we're going to do that, lets do it properly - I think the above would
need some __force usage to stop sparse complaining, whereas I don't
think this will (untested):

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 3f468ac98592..827cbc022900 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
 asmlinkage void do_undefinstr(struct pt_regs *regs)
 {
 	unsigned int instr;
-	void __user *pc;
+	unsigned long pc;
 
-	pc = (void __user *)instruction_pointer(regs);
+	pc = instruction_pointer(regs);
 
 	if (processor_mode(regs) == SVC_MODE) {
-#ifdef CONFIG_THUMB2_KERNEL
-		if (thumb_mode(regs)) {
-			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
+		if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) {
+			u16 *tpc = (u16 *)pc;
+
+			instr = __mem_to_opcode_thumb16(tpc[0]);
 			if (is_wide_instruction(instr)) {
 				u16 inst2;
-				inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
+
+				inst2 = __mem_to_opcode_thumb16(tpc[1]);
 				instr = __opcode_thumb32_compose(instr, inst2);
 			}
-		} else
-#endif
-			instr = __mem_to_opcode_arm(*(u32 *) pc);
+		} else {
+			u32 *apc = (u32 *)pc;
+
+			instr = __mem_to_opcode_arm(*apc);
+		}
 	} else if (thumb_mode(regs)) {
-		if (get_user(instr, (u16 __user *)pc))
+		u16 __user *tpc = (u16 __user *)pc;
+
+		if (get_user(instr, tpc))
 			goto die_sig;
 		instr = __mem_to_opcode_thumb16(instr);
 		if (is_wide_instruction(instr)) {
 			unsigned int instr2;
-			if (get_user(instr2, (u16 __user *)pc+1))
+			if (get_user(instr2, tpc + 1))
 				goto die_sig;
 			instr2 = __mem_to_opcode_thumb16(instr2);
 			instr = __opcode_thumb32_compose(instr, instr2);
 		}
 	} else {
-		if (get_user(instr, (u32 __user *)pc))
+		u32 __user *apc = (u32 __user *)pc;
+
+		if (get_user(instr, apc))
 			goto die_sig;
+
 		instr = __mem_to_opcode_arm(instr);
 	}
 
@@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
 	}
 #endif
 	arm_notify_die("Oops - undefined instruction", regs,
-		       SIGILL, ILL_ILLOPC, pc, 0, 6);
+		       SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
 }
 NOKPROBE_SYMBOL(do_undefinstr)
 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/13 18:51, Russell King (Oracle) wrote:
> On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote:
>> On 2022/10/10 19:06, Ard Biesheuvel wrote:
>>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
>>> <thunder.leizhen@huawei.com> wrote:
>>>> On 2022/10/10 18:20, Ard Biesheuvel wrote:
>>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>>>
>>>>>> Fix the following warnings:
>>>>>>  warning: incorrect type in initializer (different address spaces)
>>>>>>     expected unsigned short [noderef] __user *register __p
>>>>>>     got unsigned short [usertype] *
>>>>>>  warning: cast removes address space '__user' of expression
>>>>>>
>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>> ---
>>>>>>  arch/arm/kernel/traps.c | 10 +++++-----
>>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
>>>>>> --- a/arch/arm/kernel/traps.c
>>>>>> +++ b/arch/arm/kernel/traps.c
>>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>>>>                         }
>>>>>>                 } else {
>>>>>>                         if (thumb)
>>>>>> -                               bad = get_user(val, &((u16 *)addr)[i]);
>>>>>> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
>>>>>>                         else
>>>>>> -                               bad = get_user(val, &((u32 *)addr)[i]);
>>>>>> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
>>>>>>                 }
>>>>>>
>>>>>>                 if (!bad)
>>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>>>>>         if (processor_mode(regs) == SVC_MODE) {
>>>>>>  #ifdef CONFIG_THUMB2_KERNEL
>>>>>>                 if (thumb_mode(regs)) {
>>>>>> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>>>> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
>>>>>
>>>>> Shouldn't this be __user as well? (and below)
>>>>
>>>> unsigned int instr;
>>>> void __user *pc;
>>>>
>>>> The __user can clear the warning, but a new warning will be generated.
>>>>
>>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>>       ^new                           ^old
>>>>
>>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
>>>>
>>>
>>> This is because dereferencing a __user pointer is not permitted.
>>>
>>> So this code should be using get_kernel_nofault() here not a plain
>>> dereference of PC. So better to fix that properly instead of papering
>>> over it with a __force cast just to make sparse happy.
>>
>> How about:
>> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>>  {
>>         unsigned int instr;
>> -       void __user *pc;
>> +       void *pc;
>>
>> -       pc = (void __user *)instruction_pointer(regs);
>> +       pc = (void *)instruction_pointer(regs);
>>
>>         if (processor_mode(regs) == SVC_MODE) {
>>  #ifdef CONFIG_THUMB2_KERNEL
>> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>         }
>>  #endif
>>         arm_notify_die("Oops - undefined instruction", regs,
>> -                      SIGILL, ILL_ILLOPC, pc, 0, 6);
>> +                      SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>>  }
>>  NOKPROBE_SYMBOL(do_undefinstr)
>>
>>
>> The 'pc' may come from kernel or user. And I found that all the get_user()
>> calls have already done type casts to the pc, except arm_notify_die().
>> I think the above changes are reasonable.
> 
> If we're going to do that, lets do it properly - I think the above would
> need some __force usage to stop sparse complaining, whereas I don't

No need to add __force. I sent it after testing.

$ cat .config | grep THUMB2
CONFIG_THUMB2_KERNEL=y
$ make C=2 ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- arch/arm/kernel/traps.o
  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  CHECK   arch/arm/kernel/traps.c
arch/arm/kernel/traps.c:97:6: warning: symbol 'dump_backtrace_stm' was not declared. Should it be static?
arch/arm/kernel/traps.c:451:17: warning: symbol 'do_undefinstr' was not declared. Should it be static?
arch/arm/kernel/traps.c:516:39: warning: symbol 'handle_fiq_as_nmi' was not declared. Should it be static?
arch/arm/kernel/traps.c:535:17: warning: symbol 'bad_mode' was not declared. Should it be static?
arch/arm/kernel/traps.c:608:16: warning: symbol 'arm_syscall' was not declared. Should it be static?
arch/arm/kernel/traps.c:734:1: warning: symbol 'baddataabort' was not declared. Should it be static?
arch/arm/kernel/traps.c:774:17: warning: symbol '__div0' was not declared. Should it be static?
arch/arm/kernel/traps.c:781:6: warning: symbol 'abort' was not declared. Should it be static?
arch/arm/kernel/traps.c:905:12: warning: symbol 'overflow_stack_ptr' was not declared. Should it be static?
arch/arm/kernel/traps.c:922:17: warning: symbol 'handle_bad_stack' was not declared. Should it be static?

> think this will (untested):
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3f468ac98592..827cbc022900 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>  {
>  	unsigned int instr;
> -	void __user *pc;
> +	unsigned long pc;
>  
> -	pc = (void __user *)instruction_pointer(regs);
> +	pc = instruction_pointer(regs);
>  
>  	if (processor_mode(regs) == SVC_MODE) {
> -#ifdef CONFIG_THUMB2_KERNEL
> -		if (thumb_mode(regs)) {
> -			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> +		if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) {
> +			u16 *tpc = (u16 *)pc;
> +
> +			instr = __mem_to_opcode_thumb16(tpc[0]);
>  			if (is_wide_instruction(instr)) {
>  				u16 inst2;
> -				inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
> +
> +				inst2 = __mem_to_opcode_thumb16(tpc[1]);
>  				instr = __opcode_thumb32_compose(instr, inst2);
>  			}
> -		} else
> -#endif
> -			instr = __mem_to_opcode_arm(*(u32 *) pc);
> +		} else {
> +			u32 *apc = (u32 *)pc;
> +
> +			instr = __mem_to_opcode_arm(*apc);
> +		}
>  	} else if (thumb_mode(regs)) {
> -		if (get_user(instr, (u16 __user *)pc))
> +		u16 __user *tpc = (u16 __user *)pc;
> +
> +		if (get_user(instr, tpc))
>  			goto die_sig;
>  		instr = __mem_to_opcode_thumb16(instr);
>  		if (is_wide_instruction(instr)) {
>  			unsigned int instr2;
> -			if (get_user(instr2, (u16 __user *)pc+1))
> +			if (get_user(instr2, tpc + 1))
>  				goto die_sig;
>  			instr2 = __mem_to_opcode_thumb16(instr2);
>  			instr = __opcode_thumb32_compose(instr, instr2);
>  		}
>  	} else {
> -		if (get_user(instr, (u32 __user *)pc))
> +		u32 __user *apc = (u32 __user *)pc;
> +
> +		if (get_user(instr, apc))
>  			goto die_sig;
> +
>  		instr = __mem_to_opcode_arm(instr);
>  	}
>  
> @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>  	}
>  #endif
>  	arm_notify_die("Oops - undefined instruction", regs,
> -		       SIGILL, ILL_ILLOPC, pc, 0, 6);
> +		       SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>  }
>  NOKPROBE_SYMBOL(do_undefinstr)
>  
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/13 18:51, Russell King (Oracle) wrote:
> On Tue, Oct 11, 2022 at 10:29:58AM +0800, Leizhen (ThunderTown) wrote:
>> On 2022/10/10 19:06, Ard Biesheuvel wrote:
>>> On Mon, 10 Oct 2022 at 12:58, Leizhen (ThunderTown)
>>> <thunder.leizhen@huawei.com> wrote:
>>>> On 2022/10/10 18:20, Ard Biesheuvel wrote:
>>>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>>>
>>>>>> Fix the following warnings:
>>>>>>  warning: incorrect type in initializer (different address spaces)
>>>>>>     expected unsigned short [noderef] __user *register __p
>>>>>>     got unsigned short [usertype] *
>>>>>>  warning: cast removes address space '__user' of expression
>>>>>>
>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>> ---
>>>>>>  arch/arm/kernel/traps.c | 10 +++++-----
>>>>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>>>>> index 20b2db6dcd1ced7..34aa80c09c508c1 100644
>>>>>> --- a/arch/arm/kernel/traps.c
>>>>>> +++ b/arch/arm/kernel/traps.c
>>>>>> @@ -188,9 +188,9 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>>>>                         }
>>>>>>                 } else {
>>>>>>                         if (thumb)
>>>>>> -                               bad = get_user(val, &((u16 *)addr)[i]);
>>>>>> +                               bad = get_user(val, &((u16 __user *)addr)[i]);
>>>>>>                         else
>>>>>> -                               bad = get_user(val, &((u32 *)addr)[i]);
>>>>>> +                               bad = get_user(val, &((u32 __user *)addr)[i]);
>>>>>>                 }
>>>>>>
>>>>>>                 if (!bad)
>>>>>> @@ -455,15 +455,15 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>>>>>         if (processor_mode(regs) == SVC_MODE) {
>>>>>>  #ifdef CONFIG_THUMB2_KERNEL
>>>>>>                 if (thumb_mode(regs)) {
>>>>>> -                       instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>>>> +                       instr = __mem_to_opcode_thumb16(((__force u16 *)pc)[0]);
>>>>>
>>>>> Shouldn't this be __user as well? (and below)
>>>>
>>>> unsigned int instr;
>>>> void __user *pc;
>>>>
>>>> The __user can clear the warning, but a new warning will be generated.
>>>>
>>>> instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
>>>>       ^new                           ^old
>>>>
>>>> arch/arm/kernel/traps.c:473:33: warning: dereference of noderef expression
>>>>
>>>
>>> This is because dereferencing a __user pointer is not permitted.
>>>
>>> So this code should be using get_kernel_nofault() here not a plain
>>> dereference of PC. So better to fix that properly instead of papering
>>> over it with a __force cast just to make sparse happy.
>>
>> How about:
>> @@ -451,9 +451,9 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>>  {
>>         unsigned int instr;
>> -       void __user *pc;
>> +       void *pc;
>>
>> -       pc = (void __user *)instruction_pointer(regs);
>> +       pc = (void *)instruction_pointer(regs);
>>
>>         if (processor_mode(regs) == SVC_MODE) {
>>  #ifdef CONFIG_THUMB2_KERNEL
>> @@ -497,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>>         }
>>  #endif
>>         arm_notify_die("Oops - undefined instruction", regs,
>> -                      SIGILL, ILL_ILLOPC, pc, 0, 6);
>> +                      SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>>  }
>>  NOKPROBE_SYMBOL(do_undefinstr)
>>
>>
>> The 'pc' may come from kernel or user. And I found that all the get_user()
>> calls have already done type casts to the pc, except arm_notify_die().
>> I think the above changes are reasonable.
> 
> If we're going to do that, lets do it properly - I think the above would
> need some __force usage to stop sparse complaining, whereas I don't
> think this will (untested):

The following changes are a little too big, and the above changes have solved
the problem completely. In order to focus on the main target, I removed this
part of the modification in v4. I hope you'll take the time to review v4.

> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 3f468ac98592..827cbc022900 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -449,36 +449,45 @@ int call_undef_hook(struct pt_regs *regs, unsigned int instr)
>  asmlinkage void do_undefinstr(struct pt_regs *regs)
>  {
>  	unsigned int instr;
> -	void __user *pc;
> +	unsigned long pc;

The following changes need to be added:
@@ -487,7 +497,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
 die_sig:
 #ifdef CONFIG_DEBUG_USER
        if (user_debug & UDBG_UNDEFINED) {
-               pr_info("%s (%d): undefined instruction: pc=%px\n",
+               pr_info("%s (%d): undefined instruction: pc=%lx\n",
                        current->comm, task_pid_nr(current), pc);
                __show_regs(regs);
                dump_instr(KERN_INFO, regs);
>  
> -	pc = (void __user *)instruction_pointer(regs);
> +	pc = instruction_pointer(regs);
>  
>  	if (processor_mode(regs) == SVC_MODE) {
> -#ifdef CONFIG_THUMB2_KERNEL
> -		if (thumb_mode(regs)) {
> -			instr = __mem_to_opcode_thumb16(((u16 *)pc)[0]);
> +		if (IS_ENABLED(CONFIG_THUMB2_KERNEL) && thumb_mode(regs)) {
> +			u16 *tpc = (u16 *)pc;
> +
> +			instr = __mem_to_opcode_thumb16(tpc[0]);
>  			if (is_wide_instruction(instr)) {
>  				u16 inst2;
> -				inst2 = __mem_to_opcode_thumb16(((u16 *)pc)[1]);
> +
> +				inst2 = __mem_to_opcode_thumb16(tpc[1]);
>  				instr = __opcode_thumb32_compose(instr, inst2);
>  			}
> -		} else
> -#endif
> -			instr = __mem_to_opcode_arm(*(u32 *) pc);
> +		} else {
> +			u32 *apc = (u32 *)pc;
> +
> +			instr = __mem_to_opcode_arm(*apc);
> +		}
>  	} else if (thumb_mode(regs)) {
> -		if (get_user(instr, (u16 __user *)pc))
> +		u16 __user *tpc = (u16 __user *)pc;
> +
> +		if (get_user(instr, tpc))
>  			goto die_sig;
>  		instr = __mem_to_opcode_thumb16(instr);
>  		if (is_wide_instruction(instr)) {
>  			unsigned int instr2;
> -			if (get_user(instr2, (u16 __user *)pc+1))
> +			if (get_user(instr2, tpc + 1))
>  				goto die_sig;
>  			instr2 = __mem_to_opcode_thumb16(instr2);
>  			instr = __opcode_thumb32_compose(instr, instr2);
>  		}
>  	} else {
> -		if (get_user(instr, (u32 __user *)pc))
> +		u32 __user *apc = (u32 __user *)pc;
> +
> +		if (get_user(instr, apc))
>  			goto die_sig;
> +
>  		instr = __mem_to_opcode_arm(instr);
>  	}
>  
> @@ -495,7 +504,7 @@ asmlinkage void do_undefinstr(struct pt_regs *regs)
>  	}
>  #endif
>  	arm_notify_die("Oops - undefined instruction", regs,
> -		       SIGILL, ILL_ILLOPC, pc, 0, 6);
> +		       SIGILL, ILL_ILLOPC, (void __user *)pc, 0, 6);
>  }
>  NOKPROBE_SYMBOL(do_undefinstr)
>  
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Russell King (Oracle)]

On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote:
> Fix the following warnings:
>  warning: incorrect type in initializer (different address spaces)
>     expected unsigned short [noderef] __user *register __p
>     got unsigned short [usertype] *
>  warning: cast removes address space '__user' of expression

I have a general principle that not all warnings should be fixed,
especially when it comes to sparse.

The aim is not to get to zero warnings - it's to get to a point where
the code is correct, and plastering the code with __force casts means
it isn't correct - you're just masking the warning.

So no, I really don't like this. And I really don't like seeing
__force being used in open code in casts.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/11 0:05, Russell King (Oracle) wrote:
> On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote:
>> Fix the following warnings:
>>  warning: incorrect type in initializer (different address spaces)
>>     expected unsigned short [noderef] __user *register __p
>>     got unsigned short [usertype] *
>>  warning: cast removes address space '__user' of expression
> 
> I have a general principle that not all warnings should be fixed,
> especially when it comes to sparse.

OK, I got it.

> 
> The aim is not to get to zero warnings - it's to get to a point where
> the code is correct, and plastering the code with __force casts means
> it isn't correct - you're just masking the warning.
> 
> So no, I really don't like this. And I really don't like seeing
> __force being used in open code in casts.

OK, I will clear only the first warning and leave the second warning.

> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2 1/2] ARM: Fix some check warnings of tool sparse

[Leizhen (ThunderTown)]

On 2022/10/11 10:13, Leizhen (ThunderTown) wrote:
> 
> 
> On 2022/10/11 0:05, Russell King (Oracle) wrote:
>> On Mon, Oct 10, 2022 at 05:53:45PM +0800, Zhen Lei wrote:
>>> Fix the following warnings:
>>>  warning: incorrect type in initializer (different address spaces)
>>>     expected unsigned short [noderef] __user *register __p
>>>     got unsigned short [usertype] *
>>>  warning: cast removes address space '__user' of expression
>>
>> I have a general principle that not all warnings should be fixed,
>> especially when it comes to sparse.
> 
> OK, I got it.
> 
>>
>> The aim is not to get to zero warnings - it's to get to a point where
>> the code is correct, and plastering the code with __force casts means
>> it isn't correct - you're just masking the warning.
>>
>> So no, I really don't like this. And I really don't like seeing
>> __force being used in open code in casts.
> 
> OK, I will clear only the first warning and leave the second warning.

Sorry, Maybe I misunderstood. '__force casts' mentioned here may include
not only '(__force u16 *)', but also '(u16 __user *)'.

> 
>>
> 

-- 
Regards,
  Zhen Lei

[PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[Zhen Lei]

In ARM, the mapping of instruction memory is always little-endian, except
some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction
endianness may be BE-32. Of course, its data endianness will also be BE-32
mode. Due to two negatives make a positive, the instruction stored in the
register after reading is in little-endian format. But for the case of
BE-8, the instruction endianness is LE, the instruction stored in the
register after reading is in big-endian format, which is inconsistent
with the disassembled one.

For example:
The content of disassembly:
c0429ee8:       e3500000        cmp     r0, #0
c0429eec:       159f2044        ldrne   r2, [pc, #68]
c0429ef0:       108f2002        addne   r2, pc, r2
c0429ef4:       1882000a        stmne   r2, {r1, r3}
c0429ef8:       e7f000f0        udf     #0

The output of undefined instruction exception:
Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
... ...
Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7)

This inconveniences the checking of instructions. What's worse is that,
for somebody who don't know about this, might think the instructions are
all broken.

So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to
little-endian format before they are printed. The conversion result is
as follows:
Code: e3500000 159f2044 108f2002 1882000a (e7f000f0)

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/arm/kernel/traps.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 34aa80c09c508c1..50b00c9091f079d 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
 				bad = get_user(val, &((u32 __user *)addr)[i]);
 		}
 
+		if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) {
+			if (thumb)
+				val = (__force unsigned int)cpu_to_le16(val);
+			else
+				val = (__force unsigned int)cpu_to_le32(val);
+		}
+
 		if (!bad)
 			p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ",
 					width, val);
-- 
2.25.1

Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[Ard Biesheuvel]

On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>
> In ARM, the mapping of instruction memory is always little-endian, except
> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction
> endianness may be BE-32. Of course, its data endianness will also be BE-32
> mode. Due to two negatives make a positive, the instruction stored in the
> register after reading is in little-endian format. But for the case of
> BE-8, the instruction endianness is LE, the instruction stored in the
> register after reading is in big-endian format, which is inconsistent
> with the disassembled one.
>
> For example:
> The content of disassembly:
> c0429ee8:       e3500000        cmp     r0, #0
> c0429eec:       159f2044        ldrne   r2, [pc, #68]
> c0429ef0:       108f2002        addne   r2, pc, r2
> c0429ef4:       1882000a        stmne   r2, {r1, r3}
> c0429ef8:       e7f000f0        udf     #0
>
> The output of undefined instruction exception:
> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
> ... ...
> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7)
>
> This inconveniences the checking of instructions. What's worse is that,
> for somebody who don't know about this, might think the instructions are
> all broken.
>
> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to
> little-endian format before they are printed. The conversion result is
> as follows:
> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0)
>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  arch/arm/kernel/traps.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 34aa80c09c508c1..50b00c9091f079d 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>                                 bad = get_user(val, &((u32 __user *)addr)[i]);
>                 }
>
> +               if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) {
> +                       if (thumb)
> +                               val = (__force unsigned int)cpu_to_le16(val);

Better use swab16() here instead of the ugly __force cast, given that
the swab is going to occur unconditionally here.


> +                       else
> +                               val = (__force unsigned int)cpu_to_le32(val);

and swab32() here


> +               }
> +
>                 if (!bad)
>                         p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ",
>                                         width, val);
> --
> 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 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[Leizhen (ThunderTown)]

On 2022/10/10 18:10, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>
>> In ARM, the mapping of instruction memory is always little-endian, except
>> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction
>> endianness may be BE-32. Of course, its data endianness will also be BE-32
>> mode. Due to two negatives make a positive, the instruction stored in the
>> register after reading is in little-endian format. But for the case of
>> BE-8, the instruction endianness is LE, the instruction stored in the
>> register after reading is in big-endian format, which is inconsistent
>> with the disassembled one.
>>
>> For example:
>> The content of disassembly:
>> c0429ee8:       e3500000        cmp     r0, #0
>> c0429eec:       159f2044        ldrne   r2, [pc, #68]
>> c0429ef0:       108f2002        addne   r2, pc, r2
>> c0429ef4:       1882000a        stmne   r2, {r1, r3}
>> c0429ef8:       e7f000f0        udf     #0
>>
>> The output of undefined instruction exception:
>> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
>> ... ...
>> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7)
>>
>> This inconveniences the checking of instructions. What's worse is that,
>> for somebody who don't know about this, might think the instructions are
>> all broken.
>>
>> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to
>> little-endian format before they are printed. The conversion result is
>> as follows:
>> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0)
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  arch/arm/kernel/traps.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index 34aa80c09c508c1..50b00c9091f079d 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>                                 bad = get_user(val, &((u32 __user *)addr)[i]);
>>                 }
>>
>> +               if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) {
>> +                       if (thumb)
>> +                               val = (__force unsigned int)cpu_to_le16(val);
> 
> Better use swab16() here instead of the ugly __force cast, given that
> the swab is going to occur unconditionally here.

Good idea.

> 
> 
>> +                       else
>> +                               val = (__force unsigned int)cpu_to_le32(val);
> 
> and swab32() here

OK

> 
> 
>> +               }
>> +
>>                 if (!bad)
>>                         p += sprintf(p, i == 0 ? "(%0*x) " : "%0*x ",
>>                                         width, val);
>> --
>> 2.25.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[Ard Biesheuvel]

On Mon, 10 Oct 2022 at 12:46, Leizhen (ThunderTown)
<thunder.leizhen@huawei.com> wrote:
>
>
>
> On 2022/10/10 18:10, Ard Biesheuvel wrote:
> > On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
> >>
> >> In ARM, the mapping of instruction memory is always little-endian, except
> >> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction
> >> endianness may be BE-32. Of course, its data endianness will also be BE-32
> >> mode. Due to two negatives make a positive, the instruction stored in the
> >> register after reading is in little-endian format. But for the case of
> >> BE-8, the instruction endianness is LE, the instruction stored in the
> >> register after reading is in big-endian format, which is inconsistent
> >> with the disassembled one.
> >>
> >> For example:
> >> The content of disassembly:
> >> c0429ee8:       e3500000        cmp     r0, #0
> >> c0429eec:       159f2044        ldrne   r2, [pc, #68]
> >> c0429ef0:       108f2002        addne   r2, pc, r2
> >> c0429ef4:       1882000a        stmne   r2, {r1, r3}
> >> c0429ef8:       e7f000f0        udf     #0
> >>
> >> The output of undefined instruction exception:
> >> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
> >> ... ...
> >> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7)
> >>
> >> This inconveniences the checking of instructions. What's worse is that,
> >> for somebody who don't know about this, might think the instructions are
> >> all broken.
> >>
> >> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to
> >> little-endian format before they are printed. The conversion result is
> >> as follows:
> >> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0)
> >>
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >> ---
> >>  arch/arm/kernel/traps.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> >> index 34aa80c09c508c1..50b00c9091f079d 100644
> >> --- a/arch/arm/kernel/traps.c
> >> +++ b/arch/arm/kernel/traps.c
> >> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> >>                                 bad = get_user(val, &((u32 __user *)addr)[i]);
> >>                 }
> >>
> >> +               if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) {
> >> +                       if (thumb)
> >> +                               val = (__force unsigned int)cpu_to_le16(val);
> >
> > Better use swab16() here instead of the ugly __force cast, given that
> > the swab is going to occur unconditionally here.
>
> Good idea.
>
> >
> >
> >> +                       else
> >> +                               val = (__force unsigned int)cpu_to_le32(val);
> >
> > and swab32() here
>
> OK
>

Actually, come to think of it, should this code perhaps be using the
mem_to_opcode helpers that are being used elsewhere in the file?

Re: [PATCH v2 2/2] ARM: Make the dumped instructions are consistent with the disassembled ones

[Leizhen (ThunderTown)]

On 2022/10/10 19:07, Ard Biesheuvel wrote:
> On Mon, 10 Oct 2022 at 12:46, Leizhen (ThunderTown)
> <thunder.leizhen@huawei.com> wrote:
>>
>>
>>
>> On 2022/10/10 18:10, Ard Biesheuvel wrote:
>>> On Mon, 10 Oct 2022 at 11:56, Zhen Lei <thunder.leizhen@huawei.com> wrote:
>>>>
>>>> In ARM, the mapping of instruction memory is always little-endian, except
>>>> some BE-32 supported ARM architectures. Such as ARMv7-R, its instruction
>>>> endianness may be BE-32. Of course, its data endianness will also be BE-32
>>>> mode. Due to two negatives make a positive, the instruction stored in the
>>>> register after reading is in little-endian format. But for the case of
>>>> BE-8, the instruction endianness is LE, the instruction stored in the
>>>> register after reading is in big-endian format, which is inconsistent
>>>> with the disassembled one.
>>>>
>>>> For example:
>>>> The content of disassembly:
>>>> c0429ee8:       e3500000        cmp     r0, #0
>>>> c0429eec:       159f2044        ldrne   r2, [pc, #68]
>>>> c0429ef0:       108f2002        addne   r2, pc, r2
>>>> c0429ef4:       1882000a        stmne   r2, {r1, r3}
>>>> c0429ef8:       e7f000f0        udf     #0
>>>>
>>>> The output of undefined instruction exception:
>>>> Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
>>>> ... ...
>>>> Code: 000050e3 44209f15 02208f10 0a008218 (f000f0e7)
>>>>
>>>> This inconveniences the checking of instructions. What's worse is that,
>>>> for somebody who don't know about this, might think the instructions are
>>>> all broken.
>>>>
>>>> So, when CONFIG_CPU_ENDIAN_BE8=y, let's convert the instructions to
>>>> little-endian format before they are printed. The conversion result is
>>>> as follows:
>>>> Code: e3500000 159f2044 108f2002 1882000a (e7f000f0)
>>>>
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>>  arch/arm/kernel/traps.c | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>>>> index 34aa80c09c508c1..50b00c9091f079d 100644
>>>> --- a/arch/arm/kernel/traps.c
>>>> +++ b/arch/arm/kernel/traps.c
>>>> @@ -193,6 +193,13 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
>>>>                                 bad = get_user(val, &((u32 __user *)addr)[i]);
>>>>                 }
>>>>
>>>> +               if (IS_ENABLED(CONFIG_CPU_ENDIAN_BE8)) {
>>>> +                       if (thumb)
>>>> +                               val = (__force unsigned int)cpu_to_le16(val);
>>>
>>> Better use swab16() here instead of the ugly __force cast, given that
>>> the swab is going to occur unconditionally here.
>>
>> Good idea.
>>
>>>
>>>
>>>> +                       else
>>>> +                               val = (__force unsigned int)cpu_to_le32(val);
>>>
>>> and swab32() here
>>
>> OK
>>
> 
> Actually, come to think of it, should this code perhaps be using the
> mem_to_opcode helpers that are being used elsewhere in the file?

Right, __mem_to_opcode_xxx is the correct solution. I need to test it.

> .
> 

-- 
Regards,
  Zhen Lei