[PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal

[PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal

[Peter Xu]

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

Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal

[Linus Torvalds]

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

Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal

[Michal Hocko]

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

Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal

[Peter Zijlstra]

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
> 

Re: [PATCH] mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal

[Linus Torvalds]

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