linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal
@ 2020-04-08 15:59 Peter Xu
  2020-04-08 16:20 ` Linus Torvalds
  2020-04-08 17:27 ` Peter Zijlstra
  0 siblings, 2 replies; 5+ messages in thread
From: Peter Xu @ 2020-04-08 15:59 UTC (permalink / raw)
  To: linux-mm, Linus Torvalds, linux-kernel
  Cc: Andrew Morton, peterx, Hillf Danton, Thomas Gleixner,
	Peter Zijlstra, syzbot+3be1a33f04dc782e9fd5, Michal Hocko

From: Hillf Danton <hdanton@sina.com>

__get_user_pages_locked() will return 0 instead of -EINTR after commit
4426e945df588 which added extra code to allow gup detect fatal signal
faster.  Restore that behavior.

Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Fixes: 4426e945df58 ("mm/gup: allow VM_FAULT_RETRY for multiple times")
Reported-by: syzbot+3be1a33f04dc782e9fd5@syzkaller.appspotmail.com
Signed-off-by: Hillf Danton <hdanton@sina.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---

PS. Patch verified with syzbot.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/gup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index afce0bc47e70..6076df8e04a4 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1326,8 +1326,11 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
 		 * start trying again otherwise it can loop forever.
 		 */
 
-		if (fatal_signal_pending(current))
+		if (fatal_signal_pending(current)) {
+			if (!pages_done)
+				pages_done = -EINTR;
 			break;
+		}
 
 		ret = down_read_killable(&mm->mmap_sem);
 		if (ret) {
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal
  2020-04-08 15:59 [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal Peter Xu
@ 2020-04-08 16:20 ` Linus Torvalds
  2020-04-08 17:13   ` Michal Hocko
  2020-04-08 17:27 ` Peter Zijlstra
  1 sibling, 1 reply; 5+ messages in thread
From: Linus Torvalds @ 2020-04-08 16:20 UTC (permalink / raw)
  To: Peter Xu
  Cc: Linux-MM, Linux Kernel Mailing List, Andrew Morton, Hillf Danton,
	Thomas Gleixner, Peter Zijlstra, syzbot+3be1a33f04dc782e9fd5,
	Michal Hocko

On Wed, Apr 8, 2020 at 8:59 AM Peter Xu <peterx@redhat.com> wrote:
>
> __get_user_pages_locked() will return 0 instead of -EINTR after commit
> 4426e945df588 which added extra code to allow gup detect fatal signal
> faster.  Restore that behavior.

I've applied this, but it's worth noting that
__get_user_pages_locked() can still return 0 in other situations.

I realize that "I got zero pages" is a valid return value, but I do
wonder if we should make the rule be that a zero return value isn't
possible (return -EAGAIN or whatever if you doin't have the
EFAULT/EINTR conditions).

So that you'd always get either an error, or a successful number of pages.

The only case where __get_user_pages_locked() might return zero is if
you pass in a zero 'nr_pages', although I suspect even for that case
returning -EINVAL might be a better option.

Anyway, this is not a new issue, but I thought I'd mention it.

             Linus

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal
  2020-04-08 16:20 ` Linus Torvalds
@ 2020-04-08 17:13   ` Michal Hocko
  0 siblings, 0 replies; 5+ messages in thread
From: Michal Hocko @ 2020-04-08 17:13 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Peter Xu, Linux-MM, Linux Kernel Mailing List, Andrew Morton,
	Hillf Danton, Thomas Gleixner, Peter Zijlstra,
	syzbot+3be1a33f04dc782e9fd5

On Wed 08-04-20 09:20:00, Linus Torvalds wrote:
> On Wed, Apr 8, 2020 at 8:59 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > __get_user_pages_locked() will return 0 instead of -EINTR after commit
> > 4426e945df588 which added extra code to allow gup detect fatal signal
> > faster.  Restore that behavior.
> 
> I've applied this, but it's worth noting that
> __get_user_pages_locked() can still return 0 in other situations.
> 
> I realize that "I got zero pages" is a valid return value, but I do
> wonder if we should make the rule be that a zero return value isn't
> possible (return -EAGAIN or whatever if you doin't have the
> EFAULT/EINTR conditions).
> 
> So that you'd always get either an error, or a successful number of pages.
> 
> The only case where __get_user_pages_locked() might return zero is if
> you pass in a zero 'nr_pages', although I suspect even for that case
> returning -EINVAL might be a better option.

Yeah, that makes sense to me. And get_user_pages_locked documentation
would benefit from an actual documentation as well. What we have right
now is more suited to the lower level (__get_user_pages_locked) and have
a high level user oriented documentation.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal
  2020-04-08 15:59 [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal Peter Xu
  2020-04-08 16:20 ` Linus Torvalds
@ 2020-04-08 17:27 ` Peter Zijlstra
  2020-04-08 17:32   ` Linus Torvalds
  1 sibling, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2020-04-08 17:27 UTC (permalink / raw)
  To: Peter Xu
  Cc: linux-mm, Linus Torvalds, linux-kernel, Andrew Morton,
	Hillf Danton, Thomas Gleixner, syzbot+3be1a33f04dc782e9fd5,
	Michal Hocko

On Wed, Apr 08, 2020 at 11:59:24AM -0400, Peter Xu wrote:
> From: Hillf Danton <hdanton@sina.com>
> 
> __get_user_pages_locked() will return 0 instead of -EINTR after commit
> 4426e945df588 which added extra code to allow gup detect fatal signal
> faster.  Restore that behavior.
> 
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Fixes: 4426e945df58 ("mm/gup: allow VM_FAULT_RETRY for multiple times")
> Reported-by: syzbot+3be1a33f04dc782e9fd5@syzkaller.appspotmail.com
> Signed-off-by: Hillf Danton <hdanton@sina.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> PS. Patch verified with syzbot.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  mm/gup.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index afce0bc47e70..6076df8e04a4 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1326,8 +1326,11 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
>  		 * start trying again otherwise it can loop forever.
>  		 */
>  
> -		if (fatal_signal_pending(current))
> +		if (fatal_signal_pending(current)) {
> +			if (!pages_done)
> +				pages_done = -EINTR;

Why -EINTR here and -ERESTARTSYS at the other site?

>  			break;
> +		}
>  
>  		ret = down_read_killable(&mm->mmap_sem);
>  		if (ret) {
> -- 
> 2.24.1
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal
  2020-04-08 17:27 ` Peter Zijlstra
@ 2020-04-08 17:32   ` Linus Torvalds
  0 siblings, 0 replies; 5+ messages in thread
From: Linus Torvalds @ 2020-04-08 17:32 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Peter Xu, Linux-MM, Linux Kernel Mailing List, Andrew Morton,
	Hillf Danton, Thomas Gleixner, syzbot+3be1a33f04dc782e9fd5,
	Michal Hocko

On Wed, Apr 8, 2020 at 10:27 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > -             if (fatal_signal_pending(current))
> > +             if (fatal_signal_pending(current)) {
> > +                     if (!pages_done)
> > +                             pages_done = -EINTR;
>
> Why -EINTR here and -ERESTARTSYS at the other site?

I'd prefer EINTR for all fatal signals.

Not because it should matter (it's fatal, after all, the thread should
die before it ever sees it), but because I think it's less confusing.

If something is fatal, it sure as hell isn't going to restart any system calls.

But interrupting things because of fatal signals sounds sane (even if
the error code makes it to user space it's interrupting the flow of
code).

So I'd say that the other place should probably be EINTR too. But it
would obviously be a good idea to verify that no caller cares..

           Linus

              Linus

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-04-08 17:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 15:59 [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal Peter Xu
2020-04-08 16:20 ` Linus Torvalds
2020-04-08 17:13   ` Michal Hocko
2020-04-08 17:27 ` Peter Zijlstra
2020-04-08 17:32   ` Linus Torvalds

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).