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

[Jing Xiangfeng]

On 2019/9/3 1:36, Eric W. Biederman wrote:
> Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx> writes:
> 
>> On Fri, Aug 30, 2019 at 04:02:48PM -0500, Eric W. Biederman wrote:
>>> Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx> writes:
>>>
>>>> On Fri, Aug 30, 2019 at 02:45:36PM -0500, Eric W. Biederman wrote:
>>>>> Russell King - ARM Linux admin <linux@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>>
>>>>>> 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;
> 
> .
>