Re: [PATCH] powerpc: Fix possible deadlock on page fault

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2013-09-05 at 17:18 +0530, Aneesh Kumar K.V wrote:
> Paul Mackerras <paulus@samba.org> writes:
> 
> > On Thu, Sep 05, 2013 at 12:47:02PM +0530, Aneesh Kumar K.V wrote:
> >
> >> @@ -280,6 +280,13 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
> >>  
> >>  	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
> >>  
> >> +	/*
> >> +	 * We want to do this outside mmap_sem, because reading code around nip
> >> +	 * can result in fault, which will cause a deadlock when called with
> >> +	 * mmap_sem held
> >> +	 */
> >> +	store_update = store_updates_sp(regs);
> >
> > We should only call store_updates_sp() if user_mode(regs); that was
> > the previous behaviour.
> 
> Updated to 
> 
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 8726779..fad7af6 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -206,7 +206,7 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  	int trap = TRAP(regs);
>   	int is_exec = trap == 0x400;
>  	int fault;
> -	int rc = 0;
> +	int rc = 0, store_update = 0;

Keep the "sp", in the name, it's confusing otherwise. It's not just
about "store update", it's about specifically recognizing instructions
used to update the stack frame in order to let them and only them
significantly lower the stack pointer.

Cheers,
Ben.

>  #if !(defined(CONFIG_4xx) || defined(CONFIG_BOOKE))
>  	/*
> @@ -280,6 +280,14 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
>  
>  	perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
>  
> +	/*
> +	 * We want to do this outside mmap_sem, because reading code around nip
> +	 * can result in fault, which will cause a deadlock when called with
> +	 * mmap_sem held
> +	 */
> +	if (user_mode(regs))
> +		store_update = store_updates_sp(regs);
> +
>  	/* When running in the kernel we expect faults to occur only to
>  	 * addresses in user space.  All other faults represent errors in the
>  	 * kernel and should generate an OOPS.  Unfortunately, in the case of an
> @@ -345,8 +353,7 @@ retry:
>  		 * between the last mapped region and the stack will
>  		 * expand the stack rather than segfaulting.
>  		 */
> -		if (address + 2048 < uregs->gpr[1]
> -		    && (!user_mode(regs) || !store_updates_sp(regs)))
> +		if (address + 2048 < uregs->gpr[1] && !store_update)
>  			goto bad_area;
>  	}
>  	if (expand_stack(vma, address))