Russell King - ARM Linux admin <li...@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 <li...@armlinux.org.uk> writes:
>> >
>> > > On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>> > >> Russell King - ARM Linux admin <li...@armlinux.org.uk> writes:
>> > >>
>> > >> > On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>> > >> >> Russell King - ARM Linux admin <li...@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 <jingxiangf...@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+ker...@armlinux.org.uk>
>> Subject: [PATCH] ARM: mm: fix alignment
>>
>> Signed-off-by: Russell King <rmk+ker...@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-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
