LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@linux.ibm.com>,
	Andrea Arcangeli <aarcange@redhat.com>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Borislav Petkov <bp@suse.de>, Pavel Machek <pavel@ucw.cz>
Subject: Re: [PATCH] mm/gup: continue VM_FAULT_RETRY processing event for pre-faults
Date: Sat, 25 May 2019 10:45:46 +0200	[thread overview]
Message-ID: <20190525084546.fap2wkefepeia22f@linutronix.de> (raw)
In-Reply-To: <alpine.LSU.2.11.1905241429460.1141@eggly.anvils>

On 2019-05-24 15:22:51 [-0700], Hugh Dickins wrote:
> I've now run a couple of hours of load successfully with Mike's patch
> to GUP, no problem; but whatever the merits of that patch in general,
> I agree with Andrew that fault_in_pages_writeable() seems altogether
> more appropriate for copy_fpstate_to_sigframe(), and have now run a
> couple of hours of load successfully with this instead (rewrite to taste):

so this patch instead of Mike's GUP patch fixes the issue you observed?
Is this just a taste question or limitation of the function in general?

I'm asking because it has been suggested and is used in MPX code (in the
signal path but .mmap) and I'm not aware of any limitation. But as I
wrote earlier to akpm, if the MM folks suggest to use this instead I am
happy to switch.

> --- 5.2-rc1/arch/x86/kernel/fpu/signal.c
> +++ linux/arch/x86/kernel/fpu/signal.c
> @@ -3,6 +3,7 @@
>   * FPU signal frame handling routines.
>   */
>  
> +#include <linux/pagemap.h>
>  #include <linux/compat.h>
>  #include <linux/cpu.h>
>  
> @@ -189,15 +190,7 @@ retry:
>  	fpregs_unlock();
>  
>  	if (ret) {
> -		int aligned_size;
> -		int nr_pages;
> -
> -		aligned_size = offset_in_page(buf_fx) + fpu_user_xstate_size;
> -		nr_pages = DIV_ROUND_UP(aligned_size, PAGE_SIZE);
> -
> -		ret = get_user_pages_unlocked((unsigned long)buf_fx, nr_pages,
> -					      NULL, FOLL_WRITE);
> -		if (ret == nr_pages)
> +		if (!fault_in_pages_writeable(buf_fx, fpu_user_xstate_size))
>  			goto retry;
>  		return -EFAULT;
>  	}
> 
> (I did wonder whether there needs to be an access_ok() check on buf_fx;
> but if so, then I think it would already have been needed before the
> earlier copy_fpregs_to_sigframe(); but I didn't get deep enough into
> that to be sure, nor into whether access_ok() check on buf covers buf_fx.)

There is an access_ok() at the begin of copy_fpregs_to_sigframe(). The
memory is allocated from user's stack and there is (later) an
access_ok() for the whole region (which can be more than the memory used
by the FPU code).

> Hugh

Sebastian

  reply	other threads:[~2019-05-25  8:45 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-26 17:33 [PATCH] x86/fpu: Use fault_in_pages_writeable() for pre-faulting Sebastian Andrzej Siewior
2019-05-26 17:35 ` Sebastian Andrzej Siewior
2019-05-26 19:25   ` Hugh Dickins
2019-05-28 11:54     ` My emacs problem -- was " Pavel Machek
2019-05-29  4:18 ` Andrew Morton
2019-05-29  7:25   ` [PATCH v2] " Sebastian Andrzej Siewior
2019-05-14 14:29     ` [PATCH] mm/gup: continue VM_FAULT_RETRY processing event for pre-faults Mike Rapoport
2019-05-16 16:25       ` Andrei Vagin
2019-05-21 15:53       ` Mike Rapoport
2019-05-22 19:21       ` Andrew Morton
2019-05-22 19:43         ` Sebastian Andrzej Siewior
2019-05-24 22:22           ` Hugh Dickins
2019-05-25  8:45             ` Sebastian Andrzej Siewior [this message]
2019-05-25 18:09               ` Hugh Dickins
2019-05-26 19:36                 ` Sebastian Andrzej Siewior
2019-05-26 20:17                   ` Hugh Dickins
2019-05-26 17:25             ` Pavel Machek
2019-05-22 20:38         ` Mike Rapoport
2019-05-22 21:18           ` Andrew Morton
2019-06-22 17:51             ` Andrea Arcangeli
2019-06-06 17:25       ` [tip:x86/urgent] x86/fpu: Use fault_in_pages_writeable() for pre-faulting tip-bot for Hugh Dickins
2019-05-29 21:29     ` [PATCH v2] " Chris Wilson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190525084546.fap2wkefepeia22f@linutronix.de \
    --to=bigeasy@linutronix.de \
    --cc=aarcange@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=bp@suse.de \
    --cc=hughd@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=pavel@ucw.cz \
    --cc=rppt@linux.ibm.com \
    --subject='Re: [PATCH] mm/gup: continue VM_FAULT_RETRY processing event for pre-faults' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).