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 <[email protected]>
---
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
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 <[email protected]>
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
Please fix your email.
[email protected]
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 <[email protected]>
>
> 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
> [email protected]
> 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
Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
>
> 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
On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
> >
> > 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
Russell King - ARM Linux admin <[email protected]> writes:
> On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
>> >
>> > 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
On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <[email protected]> writes:
>
> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> >> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
> >> >
> >> > 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
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 <[email protected]>
>
> 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
>>
>>
>
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 <[email protected]>
> >
> > 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
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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
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
Russell King - ARM Linux admin <[email protected]> writes:
> On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>> Russell King - ARM Linux admin <[email protected]> writes:
>>
>> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> >> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
>> >> >
>> >> > 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;
On 2019/9/3 1:36, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <[email protected]> writes:
>
>> On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>>> Russell King - ARM Linux admin <[email protected]> writes:
>>>
>>>> On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>>>>> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
>>>>>>
>>>>>> 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;
>
> .
>
On Mon, Sep 02, 2019 at 12:36:56PM -0500, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <[email protected]> writes:
>
> > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
> >> Russell King - ARM Linux admin <[email protected]> writes:
> >>
> >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> >> >> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
> >> >> >
> >> >> > 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 <[email protected]>
Subject: [PATCH] ARM: mm: fix alignment
Signed-off-by: Russell King <[email protected]>
---
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
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 <[email protected]> writes:
> >
> > > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
> > >> Russell King - ARM Linux admin <[email protected]> writes:
> > >>
> > >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
> > >> >> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
> > >> >> >
> > >> >> > 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 <[email protected]>
> Subject: [PATCH] ARM: mm: fix alignment
>
> Signed-off-by: Russell King <[email protected]>
> ---
> 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
> [email protected]
> 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
Russell King - ARM Linux admin <[email protected]> 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 <[email protected]> writes:
>> >
>> > > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>> > >> Russell King - ARM Linux admin <[email protected]> writes:
>> > >>
>> > >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> > >> >> Russell King - ARM Linux admin <[email protected]> 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 <[email protected]>
>> > >> >> >
>> > >> >> > 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 <[email protected]>
>> Subject: [PATCH] ARM: mm: fix alignment
>>
>> Signed-off-by: Russell King <[email protected]>
>> ---
>> 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
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>