Re: [PATCH v8] powerpc/mm: Only read faulting instruction when necessary in do_page_fault()

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, 23 May 2018 09:31:33 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Le 23/05/2018 à 09:17, Nicholas Piggin a écrit :
> > On Wed, 23 May 2018 09:01:19 +0200 (CEST)
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >   

> >> @@ -264,8 +266,30 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> >>   		 * between the last mapped region and the stack will
> >>   		 * expand the stack rather than segfaulting.
> >>   		 */
> >> -		if (address + 2048 < uregs->gpr[1] && !store_update_sp)
> >> -			return true;
> >> +		if (address + 2048 >= uregs->gpr[1])
> >> +			return false;
> >> +		if (is_retry)
> >> +			return false;
> >> +
> >> +		if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
> >> +		    access_ok(VERIFY_READ, nip, sizeof(inst))) {
> >> +			int res;
> >> +
> >> +			pagefault_disable();
> >> +			res = __get_user_inatomic(inst, nip);
> >> +			pagefault_enable();
> >> +			if (res) {
> >> +				up_read(&mm->mmap_sem);
> >> +				res = __get_user(inst, nip);
> >> +				if (!res && store_updates_sp(inst))
> >> +					return -1;
> >> +				return true;
> >> +			}
> >> +			if (store_updates_sp(inst))
> >> +				return false;
> >> +		}
> >> +		up_read(&mm->mmap_sem);  
> > 
> > Starting to look pretty good... I think probably I prefer the mmap_sem
> > drop going into the caller so we don't don't drop in the child function.  
> 
> Yes I can do that. I though it was ok as the drop is already done in 
> children functions like bad_area(), bad_access(), ...

That's true, all exit functions though. I think it may end up being a
bit nicer with the up_read in the caller, but see what you think.

> > I thought the retry logic was a little bit complex too, what do you
> > think of using fault_in_pages_readable and just doing a full retry to
> > avoid some of this complexity?  
> 
> Yes lets try that way, allthough fault_in_pages_readable() is nothing 
> else than a get_user().
> Should we take any precaution to avoid retrying forever or is it just 
> not worth it ?

generic_perform_write() the core of the data copying for write(2)
syscall does this retry, so I think it's okay... Although I think I
wrote that so maybe that's a circular justification.

I think if we end up thrashing on this type of loop for a long time,
the system will already be basically dead.


> >>   	/* The stack is being expanded, check if it's valid */
> >> -	if (unlikely(bad_stack_expansion(regs, address, vma, store_update_sp)))
> >> -		return bad_area(regs, address);
> >> +	is_bad = bad_stack_expansion(regs, address, vma, flags, is_retry);
> >> +	if (unlikely(is_bad == -1)) {
> >> +		is_retry = true;
> >> +		goto retry;
> >> +	}
> >> +	if (unlikely(is_bad))
> >> +		return bad_area_nosemaphore(regs, address);  
> > 
> > Suggest making the return so that you can do a single unlikely test for
> > the retry or bad case, and then distinguish the retry in there. Code
> > generation should be better.  
> 
> Ok. I'll try and come with v9 during this morning.

Thanks,
Nick