| Message ID | 20200408155924.107722-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: Let __get_user_pages_locked() return -EINTR for fatal signal | expand |
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
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.
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 >
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
Hi Peter On Wed, 8 Apr 2020 19:27:23 +0200 Peter Zijlstra wrote: > > > --- 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? EINTR was selected because it goes in the direction of mutex_lock_killable() and down_read_killable() as well. Be open to s/ERESTARTSYS/EINTR/ in gup if it makes a sense to you. Thanks Hillf
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) {