fs: uninterruptible hang in handle_userfault

[Dmitry Vyukov]

Hello,

The following program creates an unkillable process in D state:

// autogenerated by syzkaller (http://github.com/google/syzkaller)
#include <pthread.h>
#include <stdint.h>
#include <string.h>
#include <sys/syscall.h>
#include <unistd.h>

#ifndef SYS_userfaultfd
#define SYS_userfaultfd 323
#endif

long r[38];

int main()
{
memset(r, -1, sizeof(r));
r[0] = syscall(SYS_mmap, 0x20000000ul, 0xc40000ul, 0x3ul, 0x32ul,
0xfffffffffffffffful, 0x0ul);
r[1] = syscall(SYS_userfaultfd, 0x800ul, 0, 0, 0, 0, 0);
*(uint64_t*)0x20c32000 = (uint64_t)0xaa;
*(uint64_t*)0x20c32008 = (uint64_t)0x0;
*(uint64_t*)0x20c32010 = (uint64_t)0x0;
r[5] = syscall(SYS_ioctl, r[1], 0xc018aa3ful, 0x20c32000ul, 0, 0, 0);
r[6] = syscall(SYS_munmap, 0x20879000ul, 0x3000ul, 0, 0, 0, 0);
r[7] =
syscall(SYS_sigaltstack, 0x208e1000ul, 0x2087affeul, 0, 0, 0, 0);
r[8] = syscall(SYS_socketpair, 0x1ul, 0xaul, 0x7ffffffful,
0x201d7ffcul, 0, 0);
if (r[8] != -1)
r[9] = *(uint32_t*)0x201d7ffc;
if (r[8] != -1)
r[10] = *(uint32_t*)0x201d8000;
r[11] = syscall(SYS_getsockopt, r[10], 0x1ul, 0xdul, 0x2087a000ul,
0x2087a000ul, 0);
*(uint64_t*)0x20c2c9d2 = (uint64_t)0x200cb000;
*(uint64_t*)0x20c2c9da = (uint64_t)0x800000;
*(uint64_t*)0x20c2c9e2 = (uint64_t)0x1;
*(uint64_t*)0x20c2c9ea = (uint64_t)0x0;
r[16] = syscall(SYS_ioctl, r[1], 0xc020aa00ul, 0x20c2c9d2ul, 0, 0, 0);
*(uint64_t*)0x20c35000 = (uint64_t)0x2028b000;
*(uint64_t*)0x20c35008 = (uint64_t)0x81;
*(uint64_t*)0x20c35010 = (uint64_t)0x201de000;
r[20] =
syscall(SYS_set_robust_list, 0x20c35000ul, 0x18ul, 0, 0, 0, 0);
memcpy((void*)0x2087bffa, "\x2e\x2f\x62\x75\x73\x00", 6);
memcpy((void*)0x2087b000, "\x2e\x2f\x62\x75\x73\x00", 6);
r[23] = syscall(SYS_linkat, r[10], 0x2087bffaul, r[9], 0x2087b000ul,
0x1400ul, 0);
r[24] = syscall(SYS_pipe2, 0x201d77ceul, 0x800ul, 0, 0, 0, 0);
if (r[24] != -1)
r[25] = *(uint32_t*)0x201d77ce;
if (r[24] != -1)
r[26] = *(uint32_t*)0x201d77d2;
*(uint64_t*)0x2087bff9 = (uint64_t)0x20bff000;
*(uint64_t*)0x2087c001 = (uint64_t)0x1000;
r[29] = syscall(SYS_ioctl, r[9], 0x8010aa02ul, 0x2087bff9ul, 0, 0, 0);
r[30] = syscall(SYS_fstat, r[26], 0x2037463bul, 0, 0, 0, 0);
memcpy((void*)0x20693fed, "\x73\x65\x6c\x69\x6e\x75\x78\x2c\x65\x6d"
"\x30\x09\x70\x70\x70\x30\x2f\x29\x00",
19);
r[32] = syscall(SYS_fgetxattr, r[25], 0x20693fedul, 0x20c366c0ul,
0x7dul, 0, 0);
r[33] = syscall(SYS_dup3, r[26], r[1], 0x80000ul, 0, 0, 0);
*(uint64_t*)0x2087bff5 = (uint64_t)0xaa;
*(uint64_t*)0x2087bffd = (uint64_t)0x0;
*(uint64_t*)0x2087c005 = (uint64_t)0x0;
r[37] =
syscall(SYS_ioctl, r[26], 0xc018aa3ful, 0x2087bff5ul, 0, 0, 0);
return 0;
}


The hang stack is:

[<ffffffff818bc380>] handle_userfault+0x680/0xe50 fs/userfaultfd.c:357
[<ffffffff8179249d>] do_huge_pmd_anonymous_page+0x7bd/0xd90 mm/huge_memory.c:904
[< inline >] create_huge_pmd mm/memory.c:3252
[< inline >] __handle_mm_fault mm/memory.c:3371
[<ffffffff816fb47d>] handle_mm_fault+0x30fd/0x4a10 mm/memory.c:3457
[<ffffffff812834c6>] __do_page_fault+0x376/0x960 arch/x86/mm/fault.c:1245
[<ffffffff81283c14>] trace_do_page_fault+0xf4/0x4f0 arch/x86/mm/fault.c:1338
[<ffffffff81274da4>] do_async_page_fault+0x14/0xd0 arch/x86/kernel/kvm.c:264
[<ffffffff866a5578>] async_page_fault+0x28/0x30 arch/x86/entry/entry_64.S:986
[<ffffffff81352874>] mm_release+0x2e4/0x410 kernel/fork.c:864
[< inline >] exit_mm kernel/exit.c:391
[<ffffffff813635df>] do_exit+0x42f/0x2d20 kernel/exit.c:735
[<ffffffff81366048>] do_group_exit+0x108/0x330 kernel/exit.c:878
[<ffffffff81389598>] get_signal+0x628/0x1560 kernel/signal.c:2307
[<ffffffff811a4db3>] do_signal+0x83/0x1c90 arch/x86/kernel/signal.c:712
[<ffffffff81006685>] exit_to_usermode_loop+0x1a5/0x210
arch/x86/entry/common.c:247
[<ffffffff810082cc>] prepare_exit_to_usermode+0x10c/0x130
arch/x86/entry/common.c:282
[<ffffffff866a3d74>] retint_user+0x8/0x23 arch/x86/entry/entry_64.S:559
[<ffffffffffffffff>] 0xffffffffffffffff

strace output:

mmap(0x20000000, 12845056, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x20000000
syscall_323(0x800, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0) = 0x3
ioctl(3, 0xc018aa3f, 0x20c32000) = 0
munmap(0x20879000, 12288) = 0
sigaltstack({ss_sp=0, ss_flags=0, ss_size=0}, ) = ?
socketpair(PF_FILE, SOCK_PACKET, 2147483647, 0x201d7ffc) = -1
EPROTONOSUPPORT (Protocol not supported)
getsockopt(-1, SOL_SOCKET, SO_LINGER, 0x2087a000, 0x2087a000) = -1
EBADF (Bad file descriptor)
ioctl(3, 0xc020aa00, 0x20c2c9d2) = 0
set_robust_list(0x20c35000, 0x18) = 0
--- SIGSEGV (Segmentation fault) @ 0 (0) ---


On commit fc77dbd34c5c99bce46d40a2491937c3bcbd10af (4.5-rc6).

[Linus Torvalds]

On Tue, Mar 1, 2016 at 3:29 AM, Dmitry Vyukov <dvy...@google.com> wrote:
>
> The following program creates an unkillable process in D state:

It seems to be usefaultfd that *tries* to handle signals, but there's
one special fault case where signals won't make it through: when we're
exiting and doing the final child pid clearing access.

We could do this two ways:

(a) special-case the PF_EXITING case for usefaultfd, something like

diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 50311703135b..66cdb44616d5 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -287,6 +287,12 @@ int handle_userfault(struct vm_area_struct
*vma, unsigned long address,
goto out;

/*
+ * We don't do userfault handling for the final child pid update.
+ */
+ if (current->flags & PF_EXITING)
+ goto out;
+
+ /*
* Check that we can return VM_FAULT_RETRY.
*
* NOTE: it should become possible to return VM_FAULT_RETRY

or (b) always consider the exiting case be "fatal signal pending"

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a10494a94cc3..5adf9f001df3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2924,7 +2924,7 @@ static inline int
__fatal_signal_pending(struct task_struct *p)

static inline int fatal_signal_pending(struct task_struct *p)
{
- return signal_pending(p) && __fatal_signal_pending(p);
+ return (p->flags & PF_EXITING) || (signal_pending(p) &&
__fatal_signal_pending(p));
}

static inline int signal_pending_state(long state, struct task_struct *p)

either of which feels a bit hacky to me.

That general "consider the final exit always as if we have a fatal
signal pending" feels like a more generic fix, but it makes me think
that it will fail on NFS-backed mmap's too. That could be seen as a
good thing (avoiding hangs when the NFS server dies), but it also
means that the patch clearly changes *other* semantics too, not just
the usefaultfd case.

So (a) is more targeted, and might be safer.

Does anybody have any other suggestions?

(The above patches are entirely untested, maybe I misread the reason
it might be hanging and it's something else going on).

Linus

[Al Viro]

On Tue, Mar 01, 2016 at 11:56:22AM -0800, Linus Torvalds wrote:
> (a) special-case the PF_EXITING case for usefaultfd, something like
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 50311703135b..66cdb44616d5 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -287,6 +287,12 @@ int handle_userfault(struct vm_area_struct
> *vma, unsigned long address,
> goto out;
>
> /*
> + * We don't do userfault handling for the final child pid update.
> + */
> + if (current->flags & PF_EXITING)
> + goto out;

Umm... Probably a dumb question, but would that suffice when e.g. another
thread is just starting to dump core?

[Linus Torvalds]

So the only access we really care about is the child tid-pointer
clearing one, and that always happens after PF_EXITING has been set
afaik.

No other case really matters. If somebody accesses a userfault region
just as another thread is exiting, we don't care. I don't think it
would necessarily be wrong to ignore the fault, but I don't think it's
relevant either, since at that stage the normal "you can signal the
thread" still works. It's only the child tid access that comes *after*
we have stopped acceping signals, and that's marked by that
PF_EXITING.

Or maybe I misunderstood your worry entirely or missed something, and
my answer above is entirely beside your point. Did you have something
else in mind?

Linus

[Linus Torvalds]

On Tue, Mar 1, 2016 at 11:56 AM, Linus Torvalds
<torv...@linux-foundation.org> wrote:
>
> (The above patches are entirely untested, maybe I misread the reason
> it might be hanging and it's something else going on).

Ok, I did some half-arsed testing. I didn't have a kernel with
USERFAULT enabled, but I did compile one with both patches in place
and I don't see a hang with the test program. Of course, since I
didn't test with an unpatched kernel that would show the hang in the
first place, it's entirely possible that my "no hang" result is due to
something else. And I didn't bother to test the two patches
independently either.

But at least on the face of it, those patches fix the problem. Dmitry?

Linus

[Al Viro]

On Tue, Mar 01, 2016 at 12:06:49PM -0800, Linus Torvalds wrote:

> So the only access we really care about is the child tid-pointer
> clearing one, and that always happens after PF_EXITING has been set
> afaik.
>
> No other case really matters. If somebody accesses a userfault region
> just as another thread is exiting, we don't care. I don't think it
> would necessarily be wrong to ignore the fault, but I don't think it's
> relevant either, since at that stage the normal "you can signal the
> thread" still works. It's only the child tid access that comes *after*
> we have stopped acceping signals, and that's marked by that
> PF_EXITING.
>
> Or maybe I misunderstood your worry entirely or missed something, and
> my answer above is entirely beside your point. Did you have something
> else in mind?

No, I've misread de_thread()/zap_other_threads(). No objections to the
patch.

[Dmitry Vyukov]

Both patches separately fix the hang.

Tested-by: Dmitry Vyukov <dvy...@google.com>

Regarding the more general fix, yesterday I've found a very similar
hang: unkillable processed hanged in snd_seq_oss_writeq_sync during
exit:
https://groups.google.com/forum/#!topic/syzkaller/bUvgnh0owos
Takashi fixed it in a better way (by deleting some code), but I though
that maybe the general fix will fix that hang as well. But no, it does
not. Still, since there are two such hangs, there can be more. Just
for what it's worth, I understand that changing fatal_signal_pending
can have far reaching implications.

[Andrea Arcangeli]

Hello,

I reviewed the fix (a) too and it's sure fine with me too.

So I evaluated if we could fix the deadlock by simply preventing to
run userland page faults while SIGKILL cannot be delivered anymore. I
think skipping those userland accesses wouldn't be strictly required
if they were run by a legit app with a userfaultfd manager thread
alive and well.

Running page faults that late in the exit path with signal disabled
was frankly unexpected. So I did a quick attempt to test such an
exit-code reordering change, but it's overall more risky and so far I
didn't succeed to have a SIGKILL reach handle_userfault() despite I
cleaned up that futex code into a proper futex_exit run just before
exit_signals instead of inside mm_release. Apparently it's not just
PF_EXITING that prevents SIGKILL to reach handle_userfault(). The
below change still didn't allow to kill the task:

+ exit_futex(tsk); /* run before setting PF_EXITING */
exit_signals(tsk); /* sets PF_EXITING */

So again I think for now fix (a) is preferable and I verified it too
with the test program. As far as the primary production user of
userfaulfd is concerned, no futex will run in the userfaultfd tracked
region so no matter what the futex exit code is there for, there's no
risk of memory corruption.

Thanks,
Andrea

[Linus Torvalds]

On Wed, Mar 2, 2016 at 6:55 AM, Andrea Arcangeli <aarc...@redhat.com> wrote:
>
> Running page faults that late in the exit path with signal disabled
> was frankly unexpected.

I agree that it's less than wonderful.

> Apparently it's not just
> PF_EXITING that prevents SIGKILL to reach handle_userfault(). The
> below change still didn't allow to kill the task:
>
> + exit_futex(tsk); /* run before setting PF_EXITING */
> exit_signals(tsk); /* sets PF_EXITING */

It's not just "exit_futex()" (what is that? I assume you mean
exit_robust_list()) that triggers the problem, it's also the

put_user(0, tsk->clear_child_tid);

in mm_release().

So it's not just about futexes.

The might be other final user space accesses lurking too that I didn't
even think about.

Anyway, I committed (a) as the safest version with the least side
effects. If people think some more about this and come up with
solutions how to avoid these kinds of "very late user space accesses"
cleanly, I think that would be great.

Linus

[Andrea Arcangeli]

On Wed, Mar 02, 2016 at 09:03:01AM -0800, Linus Torvalds wrote:
> It's not just "exit_futex()" (what is that? I assume you mean

That come from a cleanup (appended below but not very well tested) I
did initially to consolidate the futex exit code before attempting to
relocate its call location.

> exit_robust_list()) that triggers the problem, it's also the
>
> put_user(0, tsk->clear_child_tid);
>
> in mm_release().

From the stack trace it didn't appear to refault there and it was
still stuck in exit_futex, but this could end up in the same problem
and your fix already took care of this one as well.

> So it's not just about futexes.
>
> The might be other final user space accesses lurking too that I didn't
> even think about.
>
> Anyway, I committed (a) as the safest version with the least side
> effects. If people think some more about this and come up with
> solutions how to avoid these kinds of "very late user space accesses"
> cleanly, I think that would be great.

Agreed.

Thanks,
Andrea

From 03f7e43aab4e4b6f02599f4ffffe4675581f691e Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarc...@redhat.com>
Date: Wed, 2 Mar 2016 18:25:50 +0100
Subject: [PATCH 1/1] futex: cleanup #ifdef FUTEX and consolidate futex exit
path

There's no reason to expose futex internal details to the common exit path.

Signed-off-by: Andrea Arcangeli <aarc...@redhat.com>
---
include/linux/futex.h | 8 ++------
kernel/fork.c | 16 +---------------
kernel/futex.c | 22 ++++++++++++++++++++--
3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/include/linux/futex.h b/include/linux/futex.h
index 6435f46..89da7d6 100644
--- a/include/linux/futex.h
+++ b/include/linux/futex.h
@@ -53,18 +53,14 @@ union futex_key {
#define FUTEX_KEY_INIT (union futex_key) { .both = { .ptr = NULL } }

#ifdef CONFIG_FUTEX
-extern void exit_robust_list(struct task_struct *curr);
-extern void exit_pi_state_list(struct task_struct *curr);
+extern void exit_futex(struct task_struct *tsk);
#ifdef CONFIG_HAVE_FUTEX_CMPXCHG
#define futex_cmpxchg_enabled 1
#else
extern int futex_cmpxchg_enabled;
#endif
#else
-static inline void exit_robust_list(struct task_struct *curr)
-{
-}
-static inline void exit_pi_state_list(struct task_struct *curr)
+static inline void exit_futex(struct task_struct *tsk)
{
}
#endif
diff --git a/kernel/fork.c b/kernel/fork.c
index 2e391c7..81974dd 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -855,21 +855,7 @@ static int wait_for_vfork_done(struct task_struct *child,
*/
void mm_release(struct task_struct *tsk, struct mm_struct *mm)
{
- /* Get rid of any futexes when releasing the mm */
-#ifdef CONFIG_FUTEX
- if (unlikely(tsk->robust_list)) {
- exit_robust_list(tsk);
- tsk->robust_list = NULL;
- }
-#ifdef CONFIG_COMPAT
- if (unlikely(tsk->compat_robust_list)) {
- compat_exit_robust_list(tsk);
- tsk->compat_robust_list = NULL;
- }
-#endif
- if (unlikely(!list_empty(&tsk->pi_state_list)))
- exit_pi_state_list(tsk);
-#endif
+ exit_futex(tsk);

uprobe_free_utask(tsk);

diff --git a/kernel/futex.c b/kernel/futex.c
index 5d6ce64..eb2ea48 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -65,6 +65,7 @@
#include <linux/freezer.h>
#include <linux/bootmem.h>
#include <linux/fault-inject.h>
+#include <linux/compat.h>

#include <asm/futex.h>

@@ -752,7 +753,7 @@ static struct task_struct * futex_find_get_task(pid_t pid)
* Kernel cleans up PI-state, but userspace is likely hosed.
* (Robust-futex cleanup is separate and might save the day for userspace.)
*/
-void exit_pi_state_list(struct task_struct *curr)
+static void exit_pi_state_list(struct task_struct *curr)
{
struct list_head *next, *head = &curr->pi_state_list;
struct futex_pi_state *pi_state;
@@ -2975,7 +2976,7 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
*
* We silently return on any sign of list-walking problem.
*/
-void exit_robust_list(struct task_struct *curr)
+static void exit_robust_list(struct task_struct *curr)
{
struct robust_list_head __user *head = curr->robust_list;
struct robust_list __user *entry, *next_entry, *pending;
@@ -3038,6 +3039,23 @@ void exit_robust_list(struct task_struct *curr)
curr, pip);
}

+void exit_futex(struct task_struct *tsk)
+{
+ /* Get rid of any futexes when releasing the mm */
+ if (unlikely(tsk->robust_list)) {
+ exit_robust_list(tsk);
+ tsk->robust_list = NULL;
+ }
+#ifdef CONFIG_COMPAT
+ if (unlikely(tsk->compat_robust_list)) {
+ compat_exit_robust_list(tsk);
+ tsk->compat_robust_list = NULL;
+ }
+#endif
+ if (unlikely(!list_empty(&tsk->pi_state_list)))
+ exit_pi_state_list(tsk);
+}
+
long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
u32 __user *uaddr2, u32 val2, u32 val3)
{

[Sedat Dilek]

Is that commit [1] Linux-4.5 material or affects other versions, too?

commit 39680f50ae54cbbb6e72ac38b8329dd3eb9105f4
"userfaultfd: don't block on the last VM updates at exit time"

- Sedat -

[1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=39680f50ae54cbbb6e72ac38b8329dd3eb9105f4

[Linus Torvalds]

On Mar 2, 2016 23:14, "Sedat Dilek" <sedat...@gmail.com> wrote:
>
> Is that commit [1] Linux-4.5 material or affects other versions, too?

Hmm. I guess this affects anything with userfaultfd.

     Linus

[Sedat Dilek]

OK, Linux v4.4.y LTS has userfaultfd - is affected.

Just anorganizational question:
As this commit has no CC:stable tag, how do release-manager get the
information to include that in their series of patches?

One technical question:
How do I get the latest Linux version shipped userfaultfd first?
( Maybe there exist more elegant ways I do. Always open to improve my
Git knowledge. )

- Sedat -

[Andrea Arcangeli]

Hello,

On Thu, Mar 03, 2016 at 08:46:41AM +0100, Sedat Dilek wrote:
> One technical question:
> How do I get the latest Linux version shipped userfaultfd first?
> ( Maybe there exist more elegant ways I do. Always open to improve my
> Git knowledge. )

Perhaps there are cleaner ways, I would do this:

git describe `git log --pretty=%H fs/userfaultfd.c | tail -1`
v4.2-7031-g86039bd
git show v4.2-7031-g86039bd