[PATCH] arm: fix page faults in do_alignment

[PATCH] arm: fix page faults in do_alignment

[Jing Xiangfeng]

The function do_alignment can handle misaligned address for user and
kernel space. If it is a userspace access, do_alignment may fail on
a low-memory situation, because page faults are disabled in
probe_kernel_address.

Fix this by using __copy_from_user stead of probe_kernel_address.

Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
---
 arch/arm/mm/alignment.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 04b3643..2ccabd3 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -774,6 +774,7 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
 	unsigned long instr = 0, instrptr;
 	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
 	unsigned int type;
+	mm_segment_t fs;
 	unsigned int fault;
 	u16 tinstr = 0;
 	int isize = 4;
@@ -784,16 +785,22 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
 
 	instrptr = instruction_pointer(regs);
 
+	fs = get_fs();
+	set_fs(KERNEL_DS);
 	if (thumb_mode(regs)) {
 		u16 *ptr = (u16 *)(instrptr & ~1);
-		fault = probe_kernel_address(ptr, tinstr);
+		fault = __copy_from_user(tinstr,
+				(__force const void __user *)ptr,
+				sizeof(tinstr));
 		tinstr = __mem_to_opcode_thumb16(tinstr);
 		if (!fault) {
 			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
 			    IS_T32(tinstr)) {
 				/* Thumb-2 32-bit */
 				u16 tinst2 = 0;
-				fault = probe_kernel_address(ptr + 1, tinst2);
+				fault = __copy_from_user(tinst2,
+						(__force const void __user *)(ptr+1),
+						sizeof(tinst2));
 				tinst2 = __mem_to_opcode_thumb16(tinst2);
 				instr = __opcode_thumb32_compose(tinstr, tinst2);
 				thumb2_32b = 1;
@@ -803,10 +810,13 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
 			}
 		}
 	} else {
-		fault = probe_kernel_address((void *)instrptr, instr);
+		fault = __copy_from_user(instr,
+				(__force const void __user *)instrptr,
+				sizeof(instr));
 		instr = __mem_to_opcode_arm(instr);
 	}
 
+	set_fs(fs);
 	if (fault) {
 		type = TYPE_FAULT;
 		goto bad_or_fault;
-- 
1.8.3.1

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> The function do_alignment can handle misaligned address for user and
> kernel space. If it is a userspace access, do_alignment may fail on
> a low-memory situation, because page faults are disabled in
> probe_kernel_address.
> 
> Fix this by using __copy_from_user stead of probe_kernel_address.
> 
> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>

NAK.

The "scheduling while atomic warning in alignment handling code" is
caused by fixing up the page fault while trying to handle the
mis-alignment fault generated from an instruction in atomic context.

Your patch re-introduces that bug.

> ---
>  arch/arm/mm/alignment.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 04b3643..2ccabd3 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -774,6 +774,7 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
>  	unsigned long instr = 0, instrptr;
>  	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
>  	unsigned int type;
> +	mm_segment_t fs;
>  	unsigned int fault;
>  	u16 tinstr = 0;
>  	int isize = 4;
> @@ -784,16 +785,22 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
>  
>  	instrptr = instruction_pointer(regs);
>  
> +	fs = get_fs();
> +	set_fs(KERNEL_DS);
>  	if (thumb_mode(regs)) {
>  		u16 *ptr = (u16 *)(instrptr & ~1);
> -		fault = probe_kernel_address(ptr, tinstr);
> +		fault = __copy_from_user(tinstr,
> +				(__force const void __user *)ptr,
> +				sizeof(tinstr));
>  		tinstr = __mem_to_opcode_thumb16(tinstr);
>  		if (!fault) {
>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
>  			    IS_T32(tinstr)) {
>  				/* Thumb-2 32-bit */
>  				u16 tinst2 = 0;
> -				fault = probe_kernel_address(ptr + 1, tinst2);
> +				fault = __copy_from_user(tinst2,
> +						(__force const void __user *)(ptr+1),
> +						sizeof(tinst2));
>  				tinst2 = __mem_to_opcode_thumb16(tinst2);
>  				instr = __opcode_thumb32_compose(tinstr, tinst2);
>  				thumb2_32b = 1;
> @@ -803,10 +810,13 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
>  			}
>  		}
>  	} else {
> -		fault = probe_kernel_address((void *)instrptr, instr);
> +		fault = __copy_from_user(instr,
> +				(__force const void __user *)instrptr,
> +				sizeof(instr));
>  		instr = __mem_to_opcode_arm(instr);
>  	}
>  
> +	set_fs(fs);
>  	if (fault) {
>  		type = TYPE_FAULT;
>  		goto bad_or_fault;
> -- 
> 1.8.3.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

Please fix your email.

  jingxiangfeng@huawei.com
      host mx7.huawei.com [168.195.93.46]
      SMTP error from remote mail server after pipelined DATA:
      554 5.7.1 spf check result is none

SPF is *not* required for email.

If you wish to impose such restrictions on email, then I reserve the
right to ignore your patches until this issue is resolved! ;)

On Fri, Aug 30, 2019 at 02:35:22PM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> > The function do_alignment can handle misaligned address for user and
> > kernel space. If it is a userspace access, do_alignment may fail on
> > a low-memory situation, because page faults are disabled in
> > probe_kernel_address.
> > 
> > Fix this by using __copy_from_user stead of probe_kernel_address.
> > 
> > Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> > Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> 
> NAK.
> 
> The "scheduling while atomic warning in alignment handling code" is
> caused by fixing up the page fault while trying to handle the
> mis-alignment fault generated from an instruction in atomic context.
> 
> Your patch re-introduces that bug.
> 
> > ---
> >  arch/arm/mm/alignment.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> > index 04b3643..2ccabd3 100644
> > --- a/arch/arm/mm/alignment.c
> > +++ b/arch/arm/mm/alignment.c
> > @@ -774,6 +774,7 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
> >  	unsigned long instr = 0, instrptr;
> >  	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
> >  	unsigned int type;
> > +	mm_segment_t fs;
> >  	unsigned int fault;
> >  	u16 tinstr = 0;
> >  	int isize = 4;
> > @@ -784,16 +785,22 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
> >  
> >  	instrptr = instruction_pointer(regs);
> >  
> > +	fs = get_fs();
> > +	set_fs(KERNEL_DS);
> >  	if (thumb_mode(regs)) {
> >  		u16 *ptr = (u16 *)(instrptr & ~1);
> > -		fault = probe_kernel_address(ptr, tinstr);
> > +		fault = __copy_from_user(tinstr,
> > +				(__force const void __user *)ptr,
> > +				sizeof(tinstr));
> >  		tinstr = __mem_to_opcode_thumb16(tinstr);
> >  		if (!fault) {
> >  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
> >  			    IS_T32(tinstr)) {
> >  				/* Thumb-2 32-bit */
> >  				u16 tinst2 = 0;
> > -				fault = probe_kernel_address(ptr + 1, tinst2);
> > +				fault = __copy_from_user(tinst2,
> > +						(__force const void __user *)(ptr+1),
> > +						sizeof(tinst2));
> >  				tinst2 = __mem_to_opcode_thumb16(tinst2);
> >  				instr = __opcode_thumb32_compose(tinstr, tinst2);
> >  				thumb2_32b = 1;
> > @@ -803,10 +810,13 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
> >  			}
> >  		}
> >  	} else {
> > -		fault = probe_kernel_address((void *)instrptr, instr);
> > +		fault = __copy_from_user(instr,
> > +				(__force const void __user *)instrptr,
> > +				sizeof(instr));
> >  		instr = __mem_to_opcode_arm(instr);
> >  	}
> >  
> > +	set_fs(fs);
> >  	if (fault) {
> >  		type = TYPE_FAULT;
> >  		goto bad_or_fault;
> > -- 
> > 1.8.3.1
> > 
> > 
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Eric W. Biederman]

Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:

> On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>> The function do_alignment can handle misaligned address for user and
>> kernel space. If it is a userspace access, do_alignment may fail on
>> a low-memory situation, because page faults are disabled in
>> probe_kernel_address.
>> 
>> Fix this by using __copy_from_user stead of probe_kernel_address.
>> 
>> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>
> NAK.
>
> The "scheduling while atomic warning in alignment handling code" is
> caused by fixing up the page fault while trying to handle the
> mis-alignment fault generated from an instruction in atomic context.
>
> Your patch re-introduces that bug.

And the patch that fixed scheduling while atomic apparently introduced a
regression.  Admittedly a regression that took 6 years to track down but
still.

So it looks like the code needs to do something like:

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 04b36436cbc0..5e2b8623851e 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -784,6 +784,9 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 
 	instrptr = instruction_pointer(regs);
 
+	if (user_mode(regs))
+		goto user;
+
 	if (thumb_mode(regs)) {
 		u16 *ptr = (u16 *)(instrptr & ~1);
 		fault = probe_kernel_address(ptr, tinstr);
@@ -933,6 +936,34 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	return 1;
 
  user:
+	if (thumb_mode(regs)) {
+		u16 *ptr = (u16 *)(instrptr & ~1);
+		fault = get_user(tinstr, ptr);
+		tinstr = __mem_to_opcode_thumb16(tinstr);
+		if (!fault) {
+			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
+			    IS_T32(tinstr)) {
+				/* Thumb-2 32-bit */
+				u16 tinst2 = 0;
+				fault = get_user(ptr + 1, tinst2);
+				tinst2 = __mem_to_opcode_thumb16(tinst2);
+				instr = __opcode_thumb32_compose(tinstr, tinst2);
+				thumb2_32b = 1;
+			} else {
+				isize = 2;
+				instr = thumb2arm(tinstr);
+			}
+		}
+	} else {
+		fault = get_user(instr, (u32*)instrptr);
+		instr = __mem_to_opcode_arm(instr);
+	}
+
+	if (fault) {
+		type = TYPE_FAULT;
+		goto bad_or_fault;
+	}
+
 	ai_user += 1;
 
 	if (ai_usermode & UM_WARN)

Eric

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> 
> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> >> The function do_alignment can handle misaligned address for user and
> >> kernel space. If it is a userspace access, do_alignment may fail on
> >> a low-memory situation, because page faults are disabled in
> >> probe_kernel_address.
> >> 
> >> Fix this by using __copy_from_user stead of probe_kernel_address.
> >> 
> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> >
> > NAK.
> >
> > The "scheduling while atomic warning in alignment handling code" is
> > caused by fixing up the page fault while trying to handle the
> > mis-alignment fault generated from an instruction in atomic context.
> >
> > Your patch re-introduces that bug.
> 
> And the patch that fixed scheduling while atomic apparently introduced a
> regression.  Admittedly a regression that took 6 years to track down but
> still.

Right, and given the number of years, we are trading one regression for
a different regression.  If we revert to the original code where we
fix up, we will end up with people complaining about a "new" regression
caused by reverting the previous fix.  Follow this policy and we just
end up constantly reverting the previous revert.

The window is very small - the page in question will have had to have
instructions read from it immediately prior to the handler being entered,
and would have had to be made "old" before subsequently being unmapped.

Rather than excessively complicating the code and making it even more
inefficient (as in your patch), we could instead retry executing the
instruction when we discover that the page is unavailable, which should
cause the page to be paged back in.

If the page really is unavailable, the prefetch abort should cause a
SEGV to be raised, otherwise the re-execution should replace the page.

The danger to that approach is we page it back in, and it gets paged
back out before we're able to read the instruction indefinitely.

However, as it's impossible for me to contact the submitter, anything
I do will be poking about in the dark and without any way to validate
that it does fix the problem, so I think apart from reviewing of any
patches, there's not much I can do.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Eric W. Biederman]

Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:

> On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>> 
>> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>> >> The function do_alignment can handle misaligned address for user and
>> >> kernel space. If it is a userspace access, do_alignment may fail on
>> >> a low-memory situation, because page faults are disabled in
>> >> probe_kernel_address.
>> >> 
>> >> Fix this by using __copy_from_user stead of probe_kernel_address.
>> >> 
>> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>> >
>> > NAK.
>> >
>> > The "scheduling while atomic warning in alignment handling code" is
>> > caused by fixing up the page fault while trying to handle the
>> > mis-alignment fault generated from an instruction in atomic context.
>> >
>> > Your patch re-introduces that bug.
>> 
>> And the patch that fixed scheduling while atomic apparently introduced a
>> regression.  Admittedly a regression that took 6 years to track down but
>> still.
>
> Right, and given the number of years, we are trading one regression for
> a different regression.  If we revert to the original code where we
> fix up, we will end up with people complaining about a "new" regression
> caused by reverting the previous fix.  Follow this policy and we just
> end up constantly reverting the previous revert.
>
> The window is very small - the page in question will have had to have
> instructions read from it immediately prior to the handler being entered,
> and would have had to be made "old" before subsequently being unmapped.

> Rather than excessively complicating the code and making it even more
> inefficient (as in your patch), we could instead retry executing the
> instruction when we discover that the page is unavailable, which should
> cause the page to be paged back in.

My patch does not introduce any inefficiencies.  It onlys moves the
check for user_mode up a bit.  My patch did duplicate the code.

> If the page really is unavailable, the prefetch abort should cause a
> SEGV to be raised, otherwise the re-execution should replace the page.
>
> The danger to that approach is we page it back in, and it gets paged
> back out before we're able to read the instruction indefinitely.

I would think either a little code duplication or a function that looks
at user_mode(regs) and picks the appropriate kind of copy to do would be
the best way to go.  Because what needs to happen in the two cases for
reading the instruction are almost completely different.

> However, as it's impossible for me to contact the submitter, anything
> I do will be poking about in the dark and without any way to validate
> that it does fix the problem, so I think apart from reviewing of any
> patches, there's not much I can do.

I didn't realize your emails to him were bouncing.  That is odd.  Mine
don't appear to be.

Eric

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> 
> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> >> 
> >> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> >> >> The function do_alignment can handle misaligned address for user and
> >> >> kernel space. If it is a userspace access, do_alignment may fail on
> >> >> a low-memory situation, because page faults are disabled in
> >> >> probe_kernel_address.
> >> >> 
> >> >> Fix this by using __copy_from_user stead of probe_kernel_address.
> >> >> 
> >> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> >> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> >> >
> >> > NAK.
> >> >
> >> > The "scheduling while atomic warning in alignment handling code" is
> >> > caused by fixing up the page fault while trying to handle the
> >> > mis-alignment fault generated from an instruction in atomic context.
> >> >
> >> > Your patch re-introduces that bug.
> >> 
> >> And the patch that fixed scheduling while atomic apparently introduced a
> >> regression.  Admittedly a regression that took 6 years to track down but
> >> still.
> >
> > Right, and given the number of years, we are trading one regression for
> > a different regression.  If we revert to the original code where we
> > fix up, we will end up with people complaining about a "new" regression
> > caused by reverting the previous fix.  Follow this policy and we just
> > end up constantly reverting the previous revert.
> >
> > The window is very small - the page in question will have had to have
> > instructions read from it immediately prior to the handler being entered,
> > and would have had to be made "old" before subsequently being unmapped.
> 
> > Rather than excessively complicating the code and making it even more
> > inefficient (as in your patch), we could instead retry executing the
> > instruction when we discover that the page is unavailable, which should
> > cause the page to be paged back in.
> 
> My patch does not introduce any inefficiencies.  It onlys moves the
> check for user_mode up a bit.  My patch did duplicate the code.
> 
> > If the page really is unavailable, the prefetch abort should cause a
> > SEGV to be raised, otherwise the re-execution should replace the page.
> >
> > The danger to that approach is we page it back in, and it gets paged
> > back out before we're able to read the instruction indefinitely.
> 
> I would think either a little code duplication or a function that looks
> at user_mode(regs) and picks the appropriate kind of copy to do would be
> the best way to go.  Because what needs to happen in the two cases for
> reading the instruction are almost completely different.

That is what I mean.  I'd prefer to avoid that with the large chunk of
code.  How about instead adding a local replacement for
probe_kernel_address() that just sorts out the reading, rather than
duplicating all the code to deal with thumb fixup. 

> > However, as it's impossible for me to contact the submitter, anything
> > I do will be poking about in the dark and without any way to validate
> > that it does fix the problem, so I think apart from reviewing of any
> > patches, there's not much I can do.
> 
> I didn't realize your emails to him were bouncing.  That is odd.  Mine
> don't appear to be.

Hmm, so the fact I posted publically in reply to my reply with the MTA
bounce message didn't give you a clue?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Jing Xiangfeng]

On 2019/8/30 21:35, Russell King - ARM Linux admin wrote:
> On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>> The function do_alignment can handle misaligned address for user and
>> kernel space. If it is a userspace access, do_alignment may fail on
>> a low-memory situation, because page faults are disabled in
>> probe_kernel_address.
>>
>> Fix this by using __copy_from_user stead of probe_kernel_address.
>>
>> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> 
> NAK.
> 
> The "scheduling while atomic warning in alignment handling code" is
> caused by fixing up the page fault while trying to handle the
> mis-alignment fault generated from an instruction in atomic context.

__might_sleep is called in the function  __get_user which lead to that bug.
And that bug is triggered in a kernel space. Page fault can not be generated.
Right?

> Your patch re-introduces that bug.
> 
>> ---
>>  arch/arm/mm/alignment.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
>> index 04b3643..2ccabd3 100644
>> --- a/arch/arm/mm/alignment.c
>> +++ b/arch/arm/mm/alignment.c
>> @@ -774,6 +774,7 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
>>  	unsigned long instr = 0, instrptr;
>>  	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
>>  	unsigned int type;
>> +	mm_segment_t fs;
>>  	unsigned int fault;
>>  	u16 tinstr = 0;
>>  	int isize = 4;
>> @@ -784,16 +785,22 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
>>  
>>  	instrptr = instruction_pointer(regs);
>>  
>> +	fs = get_fs();
>> +	set_fs(KERNEL_DS);
>>  	if (thumb_mode(regs)) {
>>  		u16 *ptr = (u16 *)(instrptr & ~1);
>> -		fault = probe_kernel_address(ptr, tinstr);
>> +		fault = __copy_from_user(tinstr,
>> +				(__force const void __user *)ptr,
>> +				sizeof(tinstr));
>>  		tinstr = __mem_to_opcode_thumb16(tinstr);
>>  		if (!fault) {
>>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
>>  			    IS_T32(tinstr)) {
>>  				/* Thumb-2 32-bit */
>>  				u16 tinst2 = 0;
>> -				fault = probe_kernel_address(ptr + 1, tinst2);
>> +				fault = __copy_from_user(tinst2,
>> +						(__force const void __user *)(ptr+1),
>> +						sizeof(tinst2));
>>  				tinst2 = __mem_to_opcode_thumb16(tinst2);
>>  				instr = __opcode_thumb32_compose(tinstr, tinst2);
>>  				thumb2_32b = 1;
>> @@ -803,10 +810,13 @@ static ssize_t alignment_proc_write(struct file *file, const char __user *buffer
>>  			}
>>  		}
>>  	} else {
>> -		fault = probe_kernel_address((void *)instrptr, instr);
>> +		fault = __copy_from_user(instr,
>> +				(__force const void __user *)instrptr,
>> +				sizeof(instr));
>>  		instr = __mem_to_opcode_arm(instr);
>>  	}
>>  
>> +	set_fs(fs);
>>  	if (fault) {
>>  		type = TYPE_FAULT;
>>  		goto bad_or_fault;
>> -- 
>> 1.8.3.1
>>
>>
> 

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

On Sat, Aug 31, 2019 at 09:49:45AM +0800, Jing Xiangfeng wrote:
> On 2019/8/30 21:35, Russell King - ARM Linux admin wrote:
> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> >> The function do_alignment can handle misaligned address for user and
> >> kernel space. If it is a userspace access, do_alignment may fail on
> >> a low-memory situation, because page faults are disabled in
> >> probe_kernel_address.
> >>
> >> Fix this by using __copy_from_user stead of probe_kernel_address.
> >>
> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> > 
> > NAK.
> > 
> > The "scheduling while atomic warning in alignment handling code" is
> > caused by fixing up the page fault while trying to handle the
> > mis-alignment fault generated from an instruction in atomic context.
> 
> __might_sleep is called in the function  __get_user which lead to that bug.
> And that bug is triggered in a kernel space. Page fault can not be generated.
> Right?

Your email is now fixed?

All of get_user(), __get_user(), copy_from_user() and __copy_from_user()
_can_ cause a page fault, which might need to fetch the page from disk.
All these four functions are equivalent as far as that goes - and indeed
as are their versions that write as well.

If the page needs to come from disk, all of these functions _will_
sleep.  If they are called from an atomic context, and the page fault
handler needs to fetch data from disk, they will attempt to sleep,
which will issue a warning.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Jing Xiangfeng]

On 2019/8/31 15:55, Russell King - ARM Linux admin wrote:
> On Sat, Aug 31, 2019 at 09:49:45AM +0800, Jing Xiangfeng wrote:
>> On 2019/8/30 21:35, Russell King - ARM Linux admin wrote:
>>> On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>>>> The function do_alignment can handle misaligned address for user and
>>>> kernel space. If it is a userspace access, do_alignment may fail on
>>>> a low-memory situation, because page faults are disabled in
>>>> probe_kernel_address.
>>>>
>>>> Fix this by using __copy_from_user stead of probe_kernel_address.
>>>>
>>>> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>>>> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>>>
>>> NAK.
>>>
>>> The "scheduling while atomic warning in alignment handling code" is
>>> caused by fixing up the page fault while trying to handle the
>>> mis-alignment fault generated from an instruction in atomic context.
>>
>> __might_sleep is called in the function  __get_user which lead to that bug.
>> And that bug is triggered in a kernel space. Page fault can not be generated.
>> Right?
> 
> Your email is now fixed?

Yeah, I just checked the mailbox, it is normal now.

> 
> All of get_user(), __get_user(), copy_from_user() and __copy_from_user()
> _can_ cause a page fault, which might need to fetch the page from disk.
> All these four functions are equivalent as far as that goes - and indeed
> as are their versions that write as well.
> 
> If the page needs to come from disk, all of these functions _will_
> sleep.  If they are called from an atomic context, and the page fault
> handler needs to fetch data from disk, they will attempt to sleep,
> which will issue a warning.
> 
 I understand.

	Thanks

Re: [PATCH] arm: fix page faults in do_alignment

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9518 bytes --]

Hi Jing,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on arm/for-next]
[cannot apply to v5.3-rc6 next-20190830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jing-Xiangfeng/arm-fix-page-faults-in-do_alignment/20190831-173417
base:   git://git.armlinux.org.uk/~rmk/linux-arm.git for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   arch/arm/mm/alignment.c: In function 'do_alignment':
>> arch/arm/mm/alignment.c:792:28: warning: passing argument 1 of '__copy_from_user' makes pointer from integer without a cast [-Wint-conversion]
      fault = __copy_from_user(tinstr,
                               ^~~~~~
   In file included from include/linux/sched/task.h:11:0,
                    from include/linux/sched/signal.h:9,
                    from arch/arm/mm/alignment.c:20:
   include/linux/uaccess.h:67:1: note: expected 'void *' but argument is of type 'u16 {aka short unsigned int}'
    __copy_from_user(void *to, const void __user *from, unsigned long n)
    ^~~~~~~~~~~~~~~~
   arch/arm/mm/alignment.c:801:30: warning: passing argument 1 of '__copy_from_user' makes pointer from integer without a cast [-Wint-conversion]
        fault = __copy_from_user(tinst2,
                                 ^~~~~~
   In file included from include/linux/sched/task.h:11:0,
                    from include/linux/sched/signal.h:9,
                    from arch/arm/mm/alignment.c:20:
   include/linux/uaccess.h:67:1: note: expected 'void *' but argument is of type 'u16 {aka short unsigned int}'
    __copy_from_user(void *to, const void __user *from, unsigned long n)
    ^~~~~~~~~~~~~~~~
   arch/arm/mm/alignment.c:813:28: warning: passing argument 1 of '__copy_from_user' makes pointer from integer without a cast [-Wint-conversion]
      fault = __copy_from_user(instr,
                               ^~~~~
   In file included from include/linux/sched/task.h:11:0,
                    from include/linux/sched/signal.h:9,
                    from arch/arm/mm/alignment.c:20:
   include/linux/uaccess.h:67:1: note: expected 'void *' but argument is of type 'long unsigned int'
    __copy_from_user(void *to, const void __user *from, unsigned long n)
    ^~~~~~~~~~~~~~~~

vim +/__copy_from_user +792 arch/arm/mm/alignment.c

   769	
   770	static int
   771	do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
   772	{
   773		union offset_union uninitialized_var(offset);
   774		unsigned long instr = 0, instrptr;
   775		int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
   776		unsigned int type;
   777		mm_segment_t fs;
   778		unsigned int fault;
   779		u16 tinstr = 0;
   780		int isize = 4;
   781		int thumb2_32b = 0;
   782	
   783		if (interrupts_enabled(regs))
   784			local_irq_enable();
   785	
   786		instrptr = instruction_pointer(regs);
   787	
   788		fs = get_fs();
   789		set_fs(KERNEL_DS);
   790		if (thumb_mode(regs)) {
   791			u16 *ptr = (u16 *)(instrptr & ~1);
 > 792			fault = __copy_from_user(tinstr,
   793					(__force const void __user *)ptr,
   794					sizeof(tinstr));
   795			tinstr = __mem_to_opcode_thumb16(tinstr);
   796			if (!fault) {
   797				if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
   798				    IS_T32(tinstr)) {
   799					/* Thumb-2 32-bit */
   800					u16 tinst2 = 0;
   801					fault = __copy_from_user(tinst2,
   802							(__force const void __user *)(ptr+1),
   803							sizeof(tinst2));
   804					tinst2 = __mem_to_opcode_thumb16(tinst2);
   805					instr = __opcode_thumb32_compose(tinstr, tinst2);
   806					thumb2_32b = 1;
   807				} else {
   808					isize = 2;
   809					instr = thumb2arm(tinstr);
   810				}
   811			}
   812		} else {
   813			fault = __copy_from_user(instr,
   814					(__force const void __user *)instrptr,
   815					sizeof(instr));
   816			instr = __mem_to_opcode_arm(instr);
   817		}
   818	
   819		set_fs(fs);
   820		if (fault) {
   821			type = TYPE_FAULT;
   822			goto bad_or_fault;
   823		}
   824	
   825		if (user_mode(regs))
   826			goto user;
   827	
   828		ai_sys += 1;
   829		ai_sys_last_pc = (void *)instruction_pointer(regs);
   830	
   831	 fixup:
   832	
   833		regs->ARM_pc += isize;
   834	
   835		switch (CODING_BITS(instr)) {
   836		case 0x00000000:	/* 3.13.4 load/store instruction extensions */
   837			if (LDSTHD_I_BIT(instr))
   838				offset.un = (instr & 0xf00) >> 4 | (instr & 15);
   839			else
   840				offset.un = regs->uregs[RM_BITS(instr)];
   841	
   842			if ((instr & 0x000000f0) == 0x000000b0 || /* LDRH, STRH */
   843			    (instr & 0x001000f0) == 0x001000f0)   /* LDRSH */
   844				handler = do_alignment_ldrhstrh;
   845			else if ((instr & 0x001000f0) == 0x000000d0 || /* LDRD */
   846				 (instr & 0x001000f0) == 0x000000f0)   /* STRD */
   847				handler = do_alignment_ldrdstrd;
   848			else if ((instr & 0x01f00ff0) == 0x01000090) /* SWP */
   849				goto swp;
   850			else
   851				goto bad;
   852			break;
   853	
   854		case 0x04000000:	/* ldr or str immediate */
   855			if (COND_BITS(instr) == 0xf0000000) /* NEON VLDn, VSTn */
   856				goto bad;
   857			offset.un = OFFSET_BITS(instr);
   858			handler = do_alignment_ldrstr;
   859			break;
   860	
   861		case 0x06000000:	/* ldr or str register */
   862			offset.un = regs->uregs[RM_BITS(instr)];
   863	
   864			if (IS_SHIFT(instr)) {
   865				unsigned int shiftval = SHIFT_BITS(instr);
   866	
   867				switch(SHIFT_TYPE(instr)) {
   868				case SHIFT_LSL:
   869					offset.un <<= shiftval;
   870					break;
   871	
   872				case SHIFT_LSR:
   873					offset.un >>= shiftval;
   874					break;
   875	
   876				case SHIFT_ASR:
   877					offset.sn >>= shiftval;
   878					break;
   879	
   880				case SHIFT_RORRRX:
   881					if (shiftval == 0) {
   882						offset.un >>= 1;
   883						if (regs->ARM_cpsr & PSR_C_BIT)
   884							offset.un |= 1 << 31;
   885					} else
   886						offset.un = offset.un >> shiftval |
   887								  offset.un << (32 - shiftval);
   888					break;
   889				}
   890			}
   891			handler = do_alignment_ldrstr;
   892			break;
   893	
   894		case 0x08000000:	/* ldm or stm, or thumb-2 32bit instruction */
   895			if (thumb2_32b) {
   896				offset.un = 0;
   897				handler = do_alignment_t32_to_handler(&instr, regs, &offset);
   898			} else {
   899				offset.un = 0;
   900				handler = do_alignment_ldmstm;
   901			}
   902			break;
   903	
   904		default:
   905			goto bad;
   906		}
   907	
   908		if (!handler)
   909			goto bad;
   910		type = handler(addr, instr, regs);
   911	
   912		if (type == TYPE_ERROR || type == TYPE_FAULT) {
   913			regs->ARM_pc -= isize;
   914			goto bad_or_fault;
   915		}
   916	
   917		if (type == TYPE_LDST)
   918			do_alignment_finish_ldst(addr, instr, regs, offset);
   919	
   920		return 0;
   921	
   922	 bad_or_fault:
   923		if (type == TYPE_ERROR)
   924			goto bad;
   925		/*
   926		 * We got a fault - fix it up, or die.
   927		 */
   928		do_bad_area(addr, fsr, regs);
   929		return 0;
   930	
   931	 swp:
   932		pr_err("Alignment trap: not handling swp instruction\n");
   933	
   934	 bad:
   935		/*
   936		 * Oops, we didn't handle the instruction.
   937		 */
   938		pr_err("Alignment trap: not handling instruction "
   939			"%0*lx at [<%08lx>]\n",
   940			isize << 1,
   941			isize == 2 ? tinstr : instr, instrptr);
   942		ai_skipped += 1;
   943		return 1;
   944	
   945	 user:
   946		ai_user += 1;
   947	
   948		if (ai_usermode & UM_WARN)
   949			printk("Alignment trap: %s (%d) PC=0x%08lx Instr=0x%0*lx "
   950			       "Address=0x%08lx FSR 0x%03x\n", current->comm,
   951				task_pid_nr(current), instrptr,
   952				isize << 1,
   953				isize == 2 ? tinstr : instr,
   954			        addr, fsr);
   955	
   956		if (ai_usermode & UM_FIXUP)
   957			goto fixup;
   958	
   959		if (ai_usermode & UM_SIGNAL) {
   960			force_sig_fault(SIGBUS, BUS_ADRALN, (void __user *)addr);
   961		} else {
   962			/*
   963			 * We're about to disable the alignment trap and return to
   964			 * user space.  But if an interrupt occurs before actually
   965			 * reaching user space, then the IRQ vector entry code will
   966			 * notice that we were still in kernel space and therefore
   967			 * the alignment trap won't be re-enabled in that case as it
   968			 * is presumed to be always on from kernel space.
   969			 * Let's prevent that race by disabling interrupts here (they
   970			 * are disabled on the way back to user space anyway in
   971			 * entry-common.S) and disable the alignment trap only if
   972			 * there is no work pending for this thread.
   973			 */
   974			raw_local_irq_disable();
   975			if (!(current_thread_info()->flags & _TIF_WORK_MASK))
   976				set_cr(cr_no_alignment);
   977		}
   978	
   979		return 0;
   980	}
   981	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 71386 bytes --]

Re: [PATCH] arm: fix page faults in do_alignment

[Eric W. Biederman]

Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:

> On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>> 
>> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>> >> 
>> >> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>> >> >> The function do_alignment can handle misaligned address for user and
>> >> >> kernel space. If it is a userspace access, do_alignment may fail on
>> >> >> a low-memory situation, because page faults are disabled in
>> >> >> probe_kernel_address.
>> >> >> 
>> >> >> Fix this by using __copy_from_user stead of probe_kernel_address.
>> >> >> 
>> >> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>> >> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>> >> >
>> >> > NAK.
>> >> >
>> >> > The "scheduling while atomic warning in alignment handling code" is
>> >> > caused by fixing up the page fault while trying to handle the
>> >> > mis-alignment fault generated from an instruction in atomic context.
>> >> >
>> >> > Your patch re-introduces that bug.
>> >> 
>> >> And the patch that fixed scheduling while atomic apparently introduced a
>> >> regression.  Admittedly a regression that took 6 years to track down but
>> >> still.
>> >
>> > Right, and given the number of years, we are trading one regression for
>> > a different regression.  If we revert to the original code where we
>> > fix up, we will end up with people complaining about a "new" regression
>> > caused by reverting the previous fix.  Follow this policy and we just
>> > end up constantly reverting the previous revert.
>> >
>> > The window is very small - the page in question will have had to have
>> > instructions read from it immediately prior to the handler being entered,
>> > and would have had to be made "old" before subsequently being unmapped.
>> 
>> > Rather than excessively complicating the code and making it even more
>> > inefficient (as in your patch), we could instead retry executing the
>> > instruction when we discover that the page is unavailable, which should
>> > cause the page to be paged back in.
>> 
>> My patch does not introduce any inefficiencies.  It onlys moves the
>> check for user_mode up a bit.  My patch did duplicate the code.
>> 
>> > If the page really is unavailable, the prefetch abort should cause a
>> > SEGV to be raised, otherwise the re-execution should replace the page.
>> >
>> > The danger to that approach is we page it back in, and it gets paged
>> > back out before we're able to read the instruction indefinitely.
>> 
>> I would think either a little code duplication or a function that looks
>> at user_mode(regs) and picks the appropriate kind of copy to do would be
>> the best way to go.  Because what needs to happen in the two cases for
>> reading the instruction are almost completely different.
>
> That is what I mean.  I'd prefer to avoid that with the large chunk of
> code.  How about instead adding a local replacement for
> probe_kernel_address() that just sorts out the reading, rather than
> duplicating all the code to deal with thumb fixup.

So something like this should be fine?

Jing Xiangfeng can you test this please?  I think this fixes your issue
but I don't currently have an arm development box where I could test this.

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 04b36436cbc0..b07d17ca0ae5 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -767,6 +767,23 @@ do_alignment_t32_to_handler(unsigned long *pinstr, struct pt_regs *regs,
 	return NULL;
 }
 
+static inline unsigned long
+copy_instr(bool umode, void *dst, unsigned long instrptr, size_t size)
+{
+	unsigned long result;
+	if (umode) {
+		void __user *src = (void *)instrptr;
+		result = copy_from_user(dst, src, size);
+	} else {
+		void *src = (void *)instrptr;
+		result = probe_kernel_read(dst, src, size);
+	}
+	/* Convert short reads into -EFAULT */
+	if ((result >= 0) && (result < size))
+		result = -EFAULT;
+	return result;
+}
+
 static int
 do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 {
@@ -778,22 +795,24 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	u16 tinstr = 0;
 	int isize = 4;
 	int thumb2_32b = 0;
+	bool umode;
 
 	if (interrupts_enabled(regs))
 		local_irq_enable();
 
 	instrptr = instruction_pointer(regs);
+	umode = user_mode(regs);
 
 	if (thumb_mode(regs)) {
-		u16 *ptr = (u16 *)(instrptr & ~1);
-		fault = probe_kernel_address(ptr, tinstr);
+		unsigned long tinstrptr = instrptr & ~1;
+		fault = copy_instr(umode, &tinstr, tinstrptr, 2);
 		tinstr = __mem_to_opcode_thumb16(tinstr);
 		if (!fault) {
 			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
 			    IS_T32(tinstr)) {
 				/* Thumb-2 32-bit */
 				u16 tinst2 = 0;
-				fault = probe_kernel_address(ptr + 1, tinst2);
+				fault = copy_instr(umode, &tinst2, tinstrptr + 2, 2);
 				tinst2 = __mem_to_opcode_thumb16(tinst2);
 				instr = __opcode_thumb32_compose(tinstr, tinst2);
 				thumb2_32b = 1;
@@ -803,7 +822,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 			}
 		}
 	} else {
-		fault = probe_kernel_address((void *)instrptr, instr);
+		fault = copy_instr(umode, &instr, instrptr, 4);
 		instr = __mem_to_opcode_arm(instr);
 	}
 
@@ -812,7 +831,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		goto bad_or_fault;
 	}
 
-	if (user_mode(regs))
+	if (umode)
 		goto user;
 
 	ai_sys += 1;

Re: [PATCH] arm: fix page faults in do_alignment

[Jing Xiangfeng]

On 2019/9/3 1:36, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> 
>> On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>>> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>>>
>>>> On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>>>>> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>>>>>
>>>>>> On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>>>>>>> The function do_alignment can handle misaligned address for user and
>>>>>>> kernel space. If it is a userspace access, do_alignment may fail on
>>>>>>> a low-memory situation, because page faults are disabled in
>>>>>>> probe_kernel_address.
>>>>>>>
>>>>>>> Fix this by using __copy_from_user stead of probe_kernel_address.
>>>>>>>
>>>>>>> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>>>>>>> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>>>>>>
>>>>>> NAK.
>>>>>>
>>>>>> The "scheduling while atomic warning in alignment handling code" is
>>>>>> caused by fixing up the page fault while trying to handle the
>>>>>> mis-alignment fault generated from an instruction in atomic context.
>>>>>>
>>>>>> Your patch re-introduces that bug.
>>>>>
>>>>> And the patch that fixed scheduling while atomic apparently introduced a
>>>>> regression.  Admittedly a regression that took 6 years to track down but
>>>>> still.
>>>>
>>>> Right, and given the number of years, we are trading one regression for
>>>> a different regression.  If we revert to the original code where we
>>>> fix up, we will end up with people complaining about a "new" regression
>>>> caused by reverting the previous fix.  Follow this policy and we just
>>>> end up constantly reverting the previous revert.
>>>>
>>>> The window is very small - the page in question will have had to have
>>>> instructions read from it immediately prior to the handler being entered,
>>>> and would have had to be made "old" before subsequently being unmapped.
>>>
>>>> Rather than excessively complicating the code and making it even more
>>>> inefficient (as in your patch), we could instead retry executing the
>>>> instruction when we discover that the page is unavailable, which should
>>>> cause the page to be paged back in.
>>>
>>> My patch does not introduce any inefficiencies.  It onlys moves the
>>> check for user_mode up a bit.  My patch did duplicate the code.
>>>
>>>> If the page really is unavailable, the prefetch abort should cause a
>>>> SEGV to be raised, otherwise the re-execution should replace the page.
>>>>
>>>> The danger to that approach is we page it back in, and it gets paged
>>>> back out before we're able to read the instruction indefinitely.
>>>
>>> I would think either a little code duplication or a function that looks
>>> at user_mode(regs) and picks the appropriate kind of copy to do would be
>>> the best way to go.  Because what needs to happen in the two cases for
>>> reading the instruction are almost completely different.
>>
>> That is what I mean.  I'd prefer to avoid that with the large chunk of
>> code.  How about instead adding a local replacement for
>> probe_kernel_address() that just sorts out the reading, rather than
>> duplicating all the code to deal with thumb fixup.
> 
> So something like this should be fine?
> 
> Jing Xiangfeng can you test this please?  I think this fixes your issue
> but I don't currently have an arm development box where I could test this.
> 
Yes, I have tested and it can fix my issue in kernel 4.19.

> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 04b36436cbc0..b07d17ca0ae5 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -767,6 +767,23 @@ do_alignment_t32_to_handler(unsigned long *pinstr, struct pt_regs *regs,
>  	return NULL;
>  }
>  
> +static inline unsigned long
> +copy_instr(bool umode, void *dst, unsigned long instrptr, size_t size)
> +{
> +	unsigned long result;
> +	if (umode) {
> +		void __user *src = (void *)instrptr;
> +		result = copy_from_user(dst, src, size);
> +	} else {
> +		void *src = (void *)instrptr;
> +		result = probe_kernel_read(dst, src, size);
> +	}
> +	/* Convert short reads into -EFAULT */
> +	if ((result >= 0) && (result < size))
> +		result = -EFAULT;
> +	return result;
> +}
> +
>  static int
>  do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  {
> @@ -778,22 +795,24 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  	u16 tinstr = 0;
>  	int isize = 4;
>  	int thumb2_32b = 0;
> +	bool umode;
>  
>  	if (interrupts_enabled(regs))
>  		local_irq_enable();
>  
>  	instrptr = instruction_pointer(regs);
> +	umode = user_mode(regs);
>  
>  	if (thumb_mode(regs)) {
> -		u16 *ptr = (u16 *)(instrptr & ~1);
> -		fault = probe_kernel_address(ptr, tinstr);
> +		unsigned long tinstrptr = instrptr & ~1;
> +		fault = copy_instr(umode, &tinstr, tinstrptr, 2);
>  		tinstr = __mem_to_opcode_thumb16(tinstr);
>  		if (!fault) {
>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
>  			    IS_T32(tinstr)) {
>  				/* Thumb-2 32-bit */
>  				u16 tinst2 = 0;
> -				fault = probe_kernel_address(ptr + 1, tinst2);
> +				fault = copy_instr(umode, &tinst2, tinstrptr + 2, 2);
>  				tinst2 = __mem_to_opcode_thumb16(tinst2);
>  				instr = __opcode_thumb32_compose(tinstr, tinst2);
>  				thumb2_32b = 1;
> @@ -803,7 +822,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  			}
>  		}
>  	} else {
> -		fault = probe_kernel_address((void *)instrptr, instr);
> +		fault = copy_instr(umode, &instr, instrptr, 4);
>  		instr = __mem_to_opcode_arm(instr);
>  	}
>  
> @@ -812,7 +831,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  		goto bad_or_fault;
>  	}
>  
> -	if (user_mode(regs))
> +	if (umode)
>  		goto user;
>  
>  	ai_sys += 1;
> 
> .
> 

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

On Mon, Sep 02, 2019 at 12:36:56PM -0500, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> 
> > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
> >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> >> 
> >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> >> >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> >> >> 
> >> >> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> >> >> >> The function do_alignment can handle misaligned address for user and
> >> >> >> kernel space. If it is a userspace access, do_alignment may fail on
> >> >> >> a low-memory situation, because page faults are disabled in
> >> >> >> probe_kernel_address.
> >> >> >> 
> >> >> >> Fix this by using __copy_from_user stead of probe_kernel_address.
> >> >> >> 
> >> >> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> >> >> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> >> >> >
> >> >> > NAK.
> >> >> >
> >> >> > The "scheduling while atomic warning in alignment handling code" is
> >> >> > caused by fixing up the page fault while trying to handle the
> >> >> > mis-alignment fault generated from an instruction in atomic context.
> >> >> >
> >> >> > Your patch re-introduces that bug.
> >> >> 
> >> >> And the patch that fixed scheduling while atomic apparently introduced a
> >> >> regression.  Admittedly a regression that took 6 years to track down but
> >> >> still.
> >> >
> >> > Right, and given the number of years, we are trading one regression for
> >> > a different regression.  If we revert to the original code where we
> >> > fix up, we will end up with people complaining about a "new" regression
> >> > caused by reverting the previous fix.  Follow this policy and we just
> >> > end up constantly reverting the previous revert.
> >> >
> >> > The window is very small - the page in question will have had to have
> >> > instructions read from it immediately prior to the handler being entered,
> >> > and would have had to be made "old" before subsequently being unmapped.
> >> 
> >> > Rather than excessively complicating the code and making it even more
> >> > inefficient (as in your patch), we could instead retry executing the
> >> > instruction when we discover that the page is unavailable, which should
> >> > cause the page to be paged back in.
> >> 
> >> My patch does not introduce any inefficiencies.  It onlys moves the
> >> check for user_mode up a bit.  My patch did duplicate the code.
> >> 
> >> > If the page really is unavailable, the prefetch abort should cause a
> >> > SEGV to be raised, otherwise the re-execution should replace the page.
> >> >
> >> > The danger to that approach is we page it back in, and it gets paged
> >> > back out before we're able to read the instruction indefinitely.
> >> 
> >> I would think either a little code duplication or a function that looks
> >> at user_mode(regs) and picks the appropriate kind of copy to do would be
> >> the best way to go.  Because what needs to happen in the two cases for
> >> reading the instruction are almost completely different.
> >
> > That is what I mean.  I'd prefer to avoid that with the large chunk of
> > code.  How about instead adding a local replacement for
> > probe_kernel_address() that just sorts out the reading, rather than
> > duplicating all the code to deal with thumb fixup.
> 
> So something like this should be fine?
> 
> Jing Xiangfeng can you test this please?  I think this fixes your issue
> but I don't currently have an arm development box where I could test this.

Sorry, only just got around to this again.  What I came up with is this:

8<===
From: Russell King <rmk+kernel@armlinux.org.uk>
Subject: [PATCH] ARM: mm: fix alignment

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/mm/alignment.c | 44 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
index 6067fa4de22b..529f54d94709 100644
--- a/arch/arm/mm/alignment.c
+++ b/arch/arm/mm/alignment.c
@@ -765,6 +765,36 @@ do_alignment_t32_to_handler(unsigned long *pinstr, struct pt_regs *regs,
 	return NULL;
 }
 
+static int alignment_get_arm(struct pt_regs *regs, u32 *ip, unsigned long *inst)
+{
+	u32 instr = 0;
+	int fault;
+
+	if (user_mode(regs))
+		fault = get_user(instr, ip);
+	else
+		fault = probe_kernel_address(ip, instr);
+
+	*inst = __mem_to_opcode_arm(instr);
+
+	return fault;
+}
+
+static int alignment_get_thumb(struct pt_regs *regs, u16 *ip, u16 *inst)
+{
+	u16 instr = 0;
+	int fault;
+
+	if (user_mode(regs))
+		fault = get_user(instr, ip);
+	else
+		fault = probe_kernel_address(ip, instr);
+
+	*inst = __mem_to_opcode_thumb16(instr);
+
+	return fault;
+}
+
 static int
 do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 {
@@ -772,10 +802,10 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	unsigned long instr = 0, instrptr;
 	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
 	unsigned int type;
-	unsigned int fault;
 	u16 tinstr = 0;
 	int isize = 4;
 	int thumb2_32b = 0;
+	int fault;
 
 	if (interrupts_enabled(regs))
 		local_irq_enable();
@@ -784,15 +814,14 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 
 	if (thumb_mode(regs)) {
 		u16 *ptr = (u16 *)(instrptr & ~1);
-		fault = probe_kernel_address(ptr, tinstr);
-		tinstr = __mem_to_opcode_thumb16(tinstr);
+
+		fault = alignment_get_thumb(regs, ptr, &tinstr);
 		if (!fault) {
 			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
 			    IS_T32(tinstr)) {
 				/* Thumb-2 32-bit */
-				u16 tinst2 = 0;
-				fault = probe_kernel_address(ptr + 1, tinst2);
-				tinst2 = __mem_to_opcode_thumb16(tinst2);
+				u16 tinst2;
+				fault = alignment_get_thumb(regs, ptr + 1, &tinst2);
 				instr = __opcode_thumb32_compose(tinstr, tinst2);
 				thumb2_32b = 1;
 			} else {
@@ -801,8 +830,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 			}
 		}
 	} else {
-		fault = probe_kernel_address((void *)instrptr, instr);
-		instr = __mem_to_opcode_arm(instr);
+		fault = alignment_get_arm(regs, (void *)instrptr, &instr);
 	}
 
 	if (fault) {
-- 
2.7.4

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Russell King - ARM Linux admin]

On Fri, Sep 06, 2019 at 04:17:59PM +0100, Russell King - ARM Linux admin wrote:
> On Mon, Sep 02, 2019 at 12:36:56PM -0500, Eric W. Biederman wrote:
> > Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> > 
> > > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
> > >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> > >> 
> > >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> > >> >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
> > >> >> 
> > >> >> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
> > >> >> >> The function do_alignment can handle misaligned address for user and
> > >> >> >> kernel space. If it is a userspace access, do_alignment may fail on
> > >> >> >> a low-memory situation, because page faults are disabled in
> > >> >> >> probe_kernel_address.
> > >> >> >> 
> > >> >> >> Fix this by using __copy_from_user stead of probe_kernel_address.
> > >> >> >> 
> > >> >> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
> > >> >> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
> > >> >> >
> > >> >> > NAK.
> > >> >> >
> > >> >> > The "scheduling while atomic warning in alignment handling code" is
> > >> >> > caused by fixing up the page fault while trying to handle the
> > >> >> > mis-alignment fault generated from an instruction in atomic context.
> > >> >> >
> > >> >> > Your patch re-introduces that bug.
> > >> >> 
> > >> >> And the patch that fixed scheduling while atomic apparently introduced a
> > >> >> regression.  Admittedly a regression that took 6 years to track down but
> > >> >> still.
> > >> >
> > >> > Right, and given the number of years, we are trading one regression for
> > >> > a different regression.  If we revert to the original code where we
> > >> > fix up, we will end up with people complaining about a "new" regression
> > >> > caused by reverting the previous fix.  Follow this policy and we just
> > >> > end up constantly reverting the previous revert.
> > >> >
> > >> > The window is very small - the page in question will have had to have
> > >> > instructions read from it immediately prior to the handler being entered,
> > >> > and would have had to be made "old" before subsequently being unmapped.
> > >> 
> > >> > Rather than excessively complicating the code and making it even more
> > >> > inefficient (as in your patch), we could instead retry executing the
> > >> > instruction when we discover that the page is unavailable, which should
> > >> > cause the page to be paged back in.
> > >> 
> > >> My patch does not introduce any inefficiencies.  It onlys moves the
> > >> check for user_mode up a bit.  My patch did duplicate the code.
> > >> 
> > >> > If the page really is unavailable, the prefetch abort should cause a
> > >> > SEGV to be raised, otherwise the re-execution should replace the page.
> > >> >
> > >> > The danger to that approach is we page it back in, and it gets paged
> > >> > back out before we're able to read the instruction indefinitely.
> > >> 
> > >> I would think either a little code duplication or a function that looks
> > >> at user_mode(regs) and picks the appropriate kind of copy to do would be
> > >> the best way to go.  Because what needs to happen in the two cases for
> > >> reading the instruction are almost completely different.
> > >
> > > That is what I mean.  I'd prefer to avoid that with the large chunk of
> > > code.  How about instead adding a local replacement for
> > > probe_kernel_address() that just sorts out the reading, rather than
> > > duplicating all the code to deal with thumb fixup.
> > 
> > So something like this should be fine?
> > 
> > Jing Xiangfeng can you test this please?  I think this fixes your issue
> > but I don't currently have an arm development box where I could test this.
> 
> Sorry, only just got around to this again.  What I came up with is this:

I've heard nothing, so I've done nothing...

> 8<===
> From: Russell King <rmk+kernel@armlinux.org.uk>
> Subject: [PATCH] ARM: mm: fix alignment
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/mm/alignment.c | 44 ++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
> index 6067fa4de22b..529f54d94709 100644
> --- a/arch/arm/mm/alignment.c
> +++ b/arch/arm/mm/alignment.c
> @@ -765,6 +765,36 @@ do_alignment_t32_to_handler(unsigned long *pinstr, struct pt_regs *regs,
>  	return NULL;
>  }
>  
> +static int alignment_get_arm(struct pt_regs *regs, u32 *ip, unsigned long *inst)
> +{
> +	u32 instr = 0;
> +	int fault;
> +
> +	if (user_mode(regs))
> +		fault = get_user(instr, ip);
> +	else
> +		fault = probe_kernel_address(ip, instr);
> +
> +	*inst = __mem_to_opcode_arm(instr);
> +
> +	return fault;
> +}
> +
> +static int alignment_get_thumb(struct pt_regs *regs, u16 *ip, u16 *inst)
> +{
> +	u16 instr = 0;
> +	int fault;
> +
> +	if (user_mode(regs))
> +		fault = get_user(instr, ip);
> +	else
> +		fault = probe_kernel_address(ip, instr);
> +
> +	*inst = __mem_to_opcode_thumb16(instr);
> +
> +	return fault;
> +}
> +
>  static int
>  do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  {
> @@ -772,10 +802,10 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  	unsigned long instr = 0, instrptr;
>  	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
>  	unsigned int type;
> -	unsigned int fault;
>  	u16 tinstr = 0;
>  	int isize = 4;
>  	int thumb2_32b = 0;
> +	int fault;
>  
>  	if (interrupts_enabled(regs))
>  		local_irq_enable();
> @@ -784,15 +814,14 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  
>  	if (thumb_mode(regs)) {
>  		u16 *ptr = (u16 *)(instrptr & ~1);
> -		fault = probe_kernel_address(ptr, tinstr);
> -		tinstr = __mem_to_opcode_thumb16(tinstr);
> +
> +		fault = alignment_get_thumb(regs, ptr, &tinstr);
>  		if (!fault) {
>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
>  			    IS_T32(tinstr)) {
>  				/* Thumb-2 32-bit */
> -				u16 tinst2 = 0;
> -				fault = probe_kernel_address(ptr + 1, tinst2);
> -				tinst2 = __mem_to_opcode_thumb16(tinst2);
> +				u16 tinst2;
> +				fault = alignment_get_thumb(regs, ptr + 1, &tinst2);
>  				instr = __opcode_thumb32_compose(tinstr, tinst2);
>  				thumb2_32b = 1;
>  			} else {
> @@ -801,8 +830,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  			}
>  		}
>  	} else {
> -		fault = probe_kernel_address((void *)instrptr, instr);
> -		instr = __mem_to_opcode_arm(instr);
> +		fault = alignment_get_arm(regs, (void *)instrptr, &instr);
>  	}
>  
>  	if (fault) {
> -- 
> 2.7.4
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH] arm: fix page faults in do_alignment

[Eric W. Biederman]

Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:

> On Fri, Sep 06, 2019 at 04:17:59PM +0100, Russell King - ARM Linux admin wrote:
>> On Mon, Sep 02, 2019 at 12:36:56PM -0500, Eric W. Biederman wrote:
>> > Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>> > 
>> > > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>> > >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>> > >> 
>> > >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> > >> >> Russell King - ARM Linux admin <linux@armlinux.org.uk> writes:
>> > >> >> 
>> > >> >> > On Fri, Aug 30, 2019 at 09:31:17PM +0800, Jing Xiangfeng wrote:
>> > >> >> >> The function do_alignment can handle misaligned address for user and
>> > >> >> >> kernel space. If it is a userspace access, do_alignment may fail on
>> > >> >> >> a low-memory situation, because page faults are disabled in
>> > >> >> >> probe_kernel_address.
>> > >> >> >> 
>> > >> >> >> Fix this by using __copy_from_user stead of probe_kernel_address.
>> > >> >> >> 
>> > >> >> >> Fixes: b255188 ("ARM: fix scheduling while atomic warning in alignment handling code")
>> > >> >> >> Signed-off-by: Jing Xiangfeng <jingxiangfeng@huawei.com>
>> > >> >> >
>> > >> >> > NAK.
>> > >> >> >
>> > >> >> > The "scheduling while atomic warning in alignment handling code" is
>> > >> >> > caused by fixing up the page fault while trying to handle the
>> > >> >> > mis-alignment fault generated from an instruction in atomic context.
>> > >> >> >
>> > >> >> > Your patch re-introduces that bug.
>> > >> >> 
>> > >> >> And the patch that fixed scheduling while atomic apparently introduced a
>> > >> >> regression.  Admittedly a regression that took 6 years to track down but
>> > >> >> still.
>> > >> >
>> > >> > Right, and given the number of years, we are trading one regression for
>> > >> > a different regression.  If we revert to the original code where we
>> > >> > fix up, we will end up with people complaining about a "new" regression
>> > >> > caused by reverting the previous fix.  Follow this policy and we just
>> > >> > end up constantly reverting the previous revert.
>> > >> >
>> > >> > The window is very small - the page in question will have had to have
>> > >> > instructions read from it immediately prior to the handler being entered,
>> > >> > and would have had to be made "old" before subsequently being unmapped.
>> > >> 
>> > >> > Rather than excessively complicating the code and making it even more
>> > >> > inefficient (as in your patch), we could instead retry executing the
>> > >> > instruction when we discover that the page is unavailable, which should
>> > >> > cause the page to be paged back in.
>> > >> 
>> > >> My patch does not introduce any inefficiencies.  It onlys moves the
>> > >> check for user_mode up a bit.  My patch did duplicate the code.
>> > >> 
>> > >> > If the page really is unavailable, the prefetch abort should cause a
>> > >> > SEGV to be raised, otherwise the re-execution should replace the page.
>> > >> >
>> > >> > The danger to that approach is we page it back in, and it gets paged
>> > >> > back out before we're able to read the instruction indefinitely.
>> > >> 
>> > >> I would think either a little code duplication or a function that looks
>> > >> at user_mode(regs) and picks the appropriate kind of copy to do would be
>> > >> the best way to go.  Because what needs to happen in the two cases for
>> > >> reading the instruction are almost completely different.
>> > >
>> > > That is what I mean.  I'd prefer to avoid that with the large chunk of
>> > > code.  How about instead adding a local replacement for
>> > > probe_kernel_address() that just sorts out the reading, rather than
>> > > duplicating all the code to deal with thumb fixup.
>> > 
>> > So something like this should be fine?
>> > 
>> > Jing Xiangfeng can you test this please?  I think this fixes your issue
>> > but I don't currently have an arm development box where I could test this.
>> 
>> Sorry, only just got around to this again.  What I came up with is this:
>
> I've heard nothing, so I've done nothing...

Sorry it wasn't clear you were looking for feedback.

This looks functionally equivalent to the last test version I posted and
that Jing Xiangfeng confirms solves his issue.

So I say please merge whichever version you like.

Eric

>> 8<===
>> From: Russell King <rmk+kernel@armlinux.org.uk>
>> Subject: [PATCH] ARM: mm: fix alignment
>> 
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> ---
>>  arch/arm/mm/alignment.c | 44 ++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 36 insertions(+), 8 deletions(-)
>> 
>> diff --git a/arch/arm/mm/alignment.c b/arch/arm/mm/alignment.c
>> index 6067fa4de22b..529f54d94709 100644
>> --- a/arch/arm/mm/alignment.c
>> +++ b/arch/arm/mm/alignment.c
>> @@ -765,6 +765,36 @@ do_alignment_t32_to_handler(unsigned long *pinstr, struct pt_regs *regs,
>>  	return NULL;
>>  }
>>  
>> +static int alignment_get_arm(struct pt_regs *regs, u32 *ip, unsigned long *inst)
>> +{
>> +	u32 instr = 0;
>> +	int fault;
>> +
>> +	if (user_mode(regs))
>> +		fault = get_user(instr, ip);
>> +	else
>> +		fault = probe_kernel_address(ip, instr);
>> +
>> +	*inst = __mem_to_opcode_arm(instr);
>> +
>> +	return fault;
>> +}
>> +
>> +static int alignment_get_thumb(struct pt_regs *regs, u16 *ip, u16 *inst)
>> +{
>> +	u16 instr = 0;
>> +	int fault;
>> +
>> +	if (user_mode(regs))
>> +		fault = get_user(instr, ip);
>> +	else
>> +		fault = probe_kernel_address(ip, instr);
>> +
>> +	*inst = __mem_to_opcode_thumb16(instr);
>> +
>> +	return fault;
>> +}
>> +
>>  static int
>>  do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>>  {
>> @@ -772,10 +802,10 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>>  	unsigned long instr = 0, instrptr;
>>  	int (*handler)(unsigned long addr, unsigned long instr, struct pt_regs *regs);
>>  	unsigned int type;
>> -	unsigned int fault;
>>  	u16 tinstr = 0;
>>  	int isize = 4;
>>  	int thumb2_32b = 0;
>> +	int fault;
>>  
>>  	if (interrupts_enabled(regs))
>>  		local_irq_enable();
>> @@ -784,15 +814,14 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>>  
>>  	if (thumb_mode(regs)) {
>>  		u16 *ptr = (u16 *)(instrptr & ~1);
>> -		fault = probe_kernel_address(ptr, tinstr);
>> -		tinstr = __mem_to_opcode_thumb16(tinstr);
>> +
>> +		fault = alignment_get_thumb(regs, ptr, &tinstr);
>>  		if (!fault) {
>>  			if (cpu_architecture() >= CPU_ARCH_ARMv7 &&
>>  			    IS_T32(tinstr)) {
>>  				/* Thumb-2 32-bit */
>> -				u16 tinst2 = 0;
>> -				fault = probe_kernel_address(ptr + 1, tinst2);
>> -				tinst2 = __mem_to_opcode_thumb16(tinst2);
>> +				u16 tinst2;
>> +				fault = alignment_get_thumb(regs, ptr + 1, &tinst2);
>>  				instr = __opcode_thumb32_compose(tinstr, tinst2);
>>  				thumb2_32b = 1;
>>  			} else {
>> @@ -801,8 +830,7 @@ do_alignment(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>>  			}
>>  		}
>>  	} else {
>> -		fault = probe_kernel_address((void *)instrptr, instr);
>> -		instr = __mem_to_opcode_arm(instr);
>> +		fault = alignment_get_arm(regs, (void *)instrptr, &instr);
>>  	}
>>  
>>  	if (fault) {
>> -- 
>> 2.7.4
>> 
>> -- 
>> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
>> According to speedtest.net: 11.9Mbps down 500kbps up
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>