On Wed, 23 May 2018 09:31:33 +0200
Christophe LEROY <christophe.le...@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.le...@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
