* [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 13:31 ` Jing Xiangfeng 0 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-08-30 13:31 UTC (permalink / raw) To: linux, ebiederm, kstewart, gregkh, gustavo, bhelgaas, jingxiangfeng, tglx, sakari.ailus Cc: linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 13:31 ` Jing Xiangfeng 0 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-08-30 13:31 UTC (permalink / raw) To: linux, ebiederm, kstewart, gregkh, gustavo, bhelgaas, jingxiangfeng, tglx, sakari.ailus Cc: linux-mm, linux-kernel, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 13:31 ` Jing Xiangfeng @ 2019-08-30 13:35 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 13:35 UTC (permalink / raw) To: Jing Xiangfeng Cc: ebiederm, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 13:35 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 13:35 UTC (permalink / raw) To: Jing Xiangfeng Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, ebiederm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 13:35 ` Russell King - ARM Linux admin @ 2019-08-30 13:48 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 13:48 UTC (permalink / raw) To: Jing Xiangfeng Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, ebiederm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 13:48 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 13:48 UTC (permalink / raw) To: Jing Xiangfeng Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, ebiederm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 13:35 ` Russell King - ARM Linux admin (?) @ 2019-08-30 19:45 ` Eric W. Biederman -1 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-08-30 19:45 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 19:45 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-08-30 19:45 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 19:45 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-08-30 19:45 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 19:45 ` Eric W. Biederman @ 2019-08-30 20:30 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 20:30 UTC (permalink / raw) To: Eric W. Biederman Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 20:30 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 20:30 UTC (permalink / raw) To: Eric W. Biederman Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 20:30 ` Russell King - ARM Linux admin (?) @ 2019-08-30 21:02 ` Eric W. Biederman -1 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-08-30 21:02 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 21:02 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-08-30 21:02 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 21:02 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-08-30 21:02 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 21:02 ` Eric W. Biederman @ 2019-08-30 22:29 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 22:29 UTC (permalink / raw) To: Eric W. Biederman Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-30 22:29 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-30 22:29 UTC (permalink / raw) To: Eric W. Biederman Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 22:29 ` Russell King - ARM Linux admin (?) @ 2019-09-02 17:36 ` Eric W. Biederman -1 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-09-02 17:36 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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; ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-02 17:36 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-09-02 17:36 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-02 17:36 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-09-02 17:36 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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; ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-09-02 17:36 ` Eric W. Biederman @ 2019-09-04 2:17 ` Jing Xiangfeng -1 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-09-04 2:17 UTC (permalink / raw) To: Eric W. Biederman, Russell King - ARM Linux admin Cc: kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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; > > . > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-04 2:17 ` Jing Xiangfeng 0 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-09-04 2:17 UTC (permalink / raw) To: Eric W. Biederman, Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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; > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-09-02 17:36 ` Eric W. Biederman @ 2019-09-06 15:17 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-09-06 15:17 UTC (permalink / raw) To: Eric W. Biederman Cc: Jing Xiangfeng, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-06 15:17 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-09-06 15:17 UTC (permalink / raw) To: Eric W. Biederman Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-09-06 15:17 ` Russell King - ARM Linux admin @ 2019-09-15 18:34 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-09-15 18:34 UTC (permalink / raw) To: Eric W. Biederman Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-15 18:34 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-09-15 18:34 UTC (permalink / raw) To: Eric W. Biederman Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-09-15 18:34 ` Russell King - ARM Linux admin (?) @ 2019-09-16 14:31 ` Eric W. Biederman -1 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-09-16 14:31 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 >> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-16 14:31 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-09-16 14:31 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-09-16 14:31 ` Eric W. Biederman 0 siblings, 0 replies; 35+ messages in thread From: Eric W. Biederman @ 2019-09-16 14:31 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, Jing Xiangfeng, linux-mm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 >> ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 13:35 ` Russell King - ARM Linux admin @ 2019-08-31 1:49 ` Jing Xiangfeng -1 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-08-31 1:49 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: ebiederm, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 >> >> > ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-31 1:49 ` Jing Xiangfeng 0 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-08-31 1:49 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, ebiederm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 >> >> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-31 1:49 ` Jing Xiangfeng @ 2019-08-31 7:55 ` Russell King - ARM Linux admin -1 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-31 7:55 UTC (permalink / raw) To: Jing Xiangfeng Cc: ebiederm, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-31 7:55 ` Russell King - ARM Linux admin 0 siblings, 0 replies; 35+ messages in thread From: Russell King - ARM Linux admin @ 2019-08-31 7:55 UTC (permalink / raw) To: Jing Xiangfeng Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, ebiederm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-31 7:55 ` Russell King - ARM Linux admin @ 2019-08-31 9:16 ` Jing Xiangfeng -1 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-08-31 9:16 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: ebiederm, kstewart, gregkh, gustavo, bhelgaas, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm 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 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment @ 2019-08-31 9:16 ` Jing Xiangfeng 0 siblings, 0 replies; 35+ messages in thread From: Jing Xiangfeng @ 2019-08-31 9:16 UTC (permalink / raw) To: Russell King - ARM Linux admin Cc: kstewart, gustavo, gregkh, linux-kernel, linux-mm, ebiederm, sakari.ailus, bhelgaas, tglx, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH] arm: fix page faults in do_alignment 2019-08-30 13:31 ` Jing Xiangfeng (?) (?) @ 2019-08-31 12:48 ` kbuild test robot -1 siblings, 0 replies; 35+ messages in thread From: kbuild test robot @ 2019-08-31 12:48 UTC (permalink / raw) To: Jing Xiangfeng Cc: kbuild-all, linux, ebiederm, kstewart, gregkh, gustavo, bhelgaas, jingxiangfeng, tglx, sakari.ailus, linux-arm-kernel, linux-kernel, linux-mm [-- 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 --] ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2019-09-16 14:32 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-30 13:31 [PATCH] arm: fix page faults in do_alignment Jing Xiangfeng 2019-08-30 13:31 ` Jing Xiangfeng 2019-08-30 13:35 ` Russell King - ARM Linux admin 2019-08-30 13:35 ` Russell King - ARM Linux admin 2019-08-30 13:48 ` Russell King - ARM Linux admin 2019-08-30 13:48 ` Russell King - ARM Linux admin 2019-08-30 19:45 ` Eric W. Biederman 2019-08-30 19:45 ` Eric W. Biederman 2019-08-30 19:45 ` Eric W. Biederman 2019-08-30 20:30 ` Russell King - ARM Linux admin 2019-08-30 20:30 ` Russell King - ARM Linux admin 2019-08-30 21:02 ` Eric W. Biederman 2019-08-30 21:02 ` Eric W. Biederman 2019-08-30 21:02 ` Eric W. Biederman 2019-08-30 22:29 ` Russell King - ARM Linux admin 2019-08-30 22:29 ` Russell King - ARM Linux admin 2019-09-02 17:36 ` Eric W. Biederman 2019-09-02 17:36 ` Eric W. Biederman 2019-09-02 17:36 ` Eric W. Biederman 2019-09-04 2:17 ` Jing Xiangfeng 2019-09-04 2:17 ` Jing Xiangfeng 2019-09-06 15:17 ` Russell King - ARM Linux admin 2019-09-06 15:17 ` Russell King - ARM Linux admin 2019-09-15 18:34 ` Russell King - ARM Linux admin 2019-09-15 18:34 ` Russell King - ARM Linux admin 2019-09-16 14:31 ` Eric W. Biederman 2019-09-16 14:31 ` Eric W. Biederman 2019-09-16 14:31 ` Eric W. Biederman 2019-08-31 1:49 ` Jing Xiangfeng 2019-08-31 1:49 ` Jing Xiangfeng 2019-08-31 7:55 ` Russell King - ARM Linux admin 2019-08-31 7:55 ` Russell King - ARM Linux admin 2019-08-31 9:16 ` Jing Xiangfeng 2019-08-31 9:16 ` Jing Xiangfeng 2019-08-31 12:48 ` kbuild test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.