proc_flush_task oops

I've hit this twice today. It's odd, because afaics, none of this code has really changed in a long time.

Quoted 2 lines · Dave Jones

I've hit this twice today. It's odd, because afaics, none of this code has really changed in a long time.

Which tree had that been?

Quoted 5 lines · Al Viro

Quoted 2 lines · Dave Jones

I've hit this twice today. It's odd, because afaics, none of this code has really changed in a long time.

Which tree had that been?

Linus, rc4.

Quoted 8 lines · Dave Jones

Quoted 5 lines · Al Viro

Quoted 2 lines · Dave Jones

I've hit this twice today. It's odd, because afaics, none of this code has really changed in a long time.

Which tree had that been?

Linus, rc4.

Ok, so the original report was marked as spam for me for whatever reason. I ended up re-analyzing the oops, but came to the same conclusion you did: it's a NULL mnt pointer in proc_flush_task_mnt(). ..
But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Quoted 4 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Telling it to just fuzz execveat doesn't instantly trigger it, so it must be a combination of some other syscall. I'll leave a script running overnight to see if I can binary search the other syscalls in combination with it.

One other thing: I said this was rc4, but it was actually rc4 + all the x86 stuff from today. There's enough creepy stuff in that pile, that I'll try with just plain rc4 tomorrow too.

Dave

Quoted 15 lines · Dave Jones

Quoted 4 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Telling it to just fuzz execveat doesn't instantly trigger it, so it must be a combination of some other syscall. I'll leave a script running overnight to see if I can binary search the other syscalls in combination with it.

Quoted 6 lines

One other thing: I said this was rc4, but it was actually rc4 + all the x86 stuff from today. There's enough creepy stuff in that pile, that I'll try with just plain rc4 tomorrow too.

Dave

When hitting an oops at finalization function using fuzzing tool, checking for corresponding initialization function and previous error messages might help.

It seems to me that there are error paths which allow nsproxy to be replaced without assigning ->pid_ns_for_children->proc_mnt.

----------
static __latent_entropy struct task_struct *copy_process(
unsigned long clone_flags, unsigned long stack_start, unsigned long stack_size, int __user *child_tidptr, struct pid *pid, int trace, unsigned long tls, int node)
{
(...snipped...)

        retval = copy_namespaces(clone_flags, p); // Allocates p->nsproxy->pid_ns_for_children
        if (retval)
                goto bad_fork_cleanup_mm;
        retval = copy_io(clone_flags, p);
        if (retval)
                goto bad_fork_cleanup_namespaces; // p->nsproxy->pid_ns_for_children->proc_mnt == NULL.
        retval = copy_thread_tls(clone_flags, stack_start, stack_size, p, tls);
        if (retval)
                goto bad_fork_cleanup_io; // p->nsproxy->pid_ns_for_children->proc_mnt == NULL.

        if (pid != &init_struct_pid) {
                pid = alloc_pid(p->nsproxy->pid_ns_for_children); // Initializes p->nsproxy->pid_ns_for_children->proc_mnt upon success.
                if (IS_ERR(pid)) {
                        retval = PTR_ERR(pid);
                        goto bad_fork_cleanup_thread; // p->nsproxy->pid_ns_for_children->proc_mnt == NULL.
                }
        }

(...snipped...)
}

int copy_namespaces(unsigned long flags, struct task_struct *tsk)

{
        struct nsproxy *old_ns = tsk->nsproxy;
        struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
        struct nsproxy *new_ns;

        if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
                              CLONE_NEWPID | CLONE_NEWNET |
                              CLONE_NEWCGROUP)))) {
                get_nsproxy(old_ns);
                return 0;
        }

        if (!ns_capable(user_ns, CAP_SYS_ADMIN))
                return -EPERM;

        /*
         * CLONE_NEWIPC must detach from the undolist: after switching
         * to a new ipc namespace, the semaphore arrays from the old
         * namespace are unreachable.  In clone parlance, CLONE_SYSVSEM
         * means share undolist with parent, so we must forbid using
         * it along with CLONE_NEWIPC.
         */
        if ((flags & (CLONE_NEWIPC | CLONE_SYSVSEM)) ==
                (CLONE_NEWIPC | CLONE_SYSVSEM))
                return -EINVAL;

        new_ns = create_new_namespaces(flags, tsk, user_ns, tsk->fs); // Allocates new nsproxy.
        if (IS_ERR(new_ns))
                return  PTR_ERR(new_ns);

        tsk->nsproxy = new_ns; // p->nsproxy is updated with ->pid_ns_for_children->proc_mnt == NULL.
        return 0;
}

static struct nsproxy *create_new_namespaces(unsigned long flags,
struct task_struct *tsk, struct user_namespace *user_ns, struct fs_struct *new_fs)

{
        struct nsproxy *new_nsp;
        int err;

        new_nsp = create_nsproxy();
        if (!new_nsp)
                return ERR_PTR(-ENOMEM);

        new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns, user_ns, new_fs);
        if (IS_ERR(new_nsp->mnt_ns)) {
                err = PTR_ERR(new_nsp->mnt_ns);
                goto out_ns;
        }

        new_nsp->uts_ns = copy_utsname(flags, user_ns, tsk->nsproxy->uts_ns);
        if (IS_ERR(new_nsp->uts_ns)) {
                err = PTR_ERR(new_nsp->uts_ns);
                goto out_uts;
        }

        new_nsp->ipc_ns = copy_ipcs(flags, user_ns, tsk->nsproxy->ipc_ns);
        if (IS_ERR(new_nsp->ipc_ns)) {
                err = PTR_ERR(new_nsp->ipc_ns);
                goto out_ipc;
        }

        new_nsp->pid_ns_for_children =
                copy_pid_ns(flags, user_ns, tsk->nsproxy->pid_ns_for_children); // Returns with ->proc_mnt == NULL.
        if (IS_ERR(new_nsp->pid_ns_for_children)) {
                err = PTR_ERR(new_nsp->pid_ns_for_children);
                goto out_pid;
        }

        new_nsp->cgroup_ns = copy_cgroup_ns(flags, user_ns,
                                            tsk->nsproxy->cgroup_ns);
        if (IS_ERR(new_nsp->cgroup_ns)) {
                err = PTR_ERR(new_nsp->cgroup_ns);
                goto out_cgroup;
        }

        new_nsp->net_ns = copy_net_ns(flags, user_ns, tsk->nsproxy->net_ns);
        if (IS_ERR(new_nsp->net_ns)) {
                err = PTR_ERR(new_nsp->net_ns);
                goto out_net;
        }

        return new_nsp; // Returns with ->pid_ns_for_children->proc_mnt == NULL.

out_net:

        put_cgroup_ns(new_nsp->cgroup_ns);
out_cgroup:
        if (new_nsp->pid_ns_for_children)
                put_pid_ns(new_nsp->pid_ns_for_children);
out_pid:
        if (new_nsp->ipc_ns)
                put_ipc_ns(new_nsp->ipc_ns);
out_ipc:
        if (new_nsp->uts_ns)
                put_uts_ns(new_nsp->uts_ns);
out_uts:
        if (new_nsp->mnt_ns)
                put_mnt_ns(new_nsp->mnt_ns);
out_ns:
        kmem_cache_free(nsproxy_cachep, new_nsp);
        return ERR_PTR(err);
}

struct pid_namespace *copy_pid_ns(unsigned long flags,
struct user_namespace *user_ns, struct pid_namespace *old_ns)

{
        if (!(flags & CLONE_NEWPID))
                return get_pid_ns(old_ns);
        if (task_active_pid_ns(current) != old_ns)
                return ERR_PTR(-EINVAL);
        return create_pid_namespace(user_ns, old_ns); // Returns with ->proc_mnt == NULL.
}

static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns,
struct pid_namespace *parent_pid_ns)

{
        struct pid_namespace *ns;
        unsigned int level = parent_pid_ns->level + 1;
        struct ucounts *ucounts;
        int err;

        err = -EINVAL;
        if (!in_userns(parent_pid_ns->user_ns, user_ns))
                goto out;

        err = -ENOSPC;
        if (level > MAX_PID_NS_LEVEL)
                goto out;
        ucounts = inc_pid_namespaces(user_ns);
        if (!ucounts)
                goto out;

        err = -ENOMEM;
        ns = kmem_cache_zalloc(pid_ns_cachep, GFP_KERNEL); // Initializes ->proc_mnt with NULL.
        if (ns == NULL)
                goto out_dec;

        idr_init(&ns->idr);

        ns->pid_cachep = create_pid_cachep(level + 1);
        if (ns->pid_cachep == NULL)
                goto out_free_idr;

        err = ns_alloc_inum(&ns->ns);
        if (err)
                goto out_free_idr;
        ns->ns.ops = &pidns_operations;

        kref_init(&ns->kref);
        ns->level = level;
        ns->parent = get_pid_ns(parent_pid_ns);
        ns->user_ns = get_user_ns(user_ns);
        ns->ucounts = ucounts;
        ns->pid_allocated = PIDNS_ADDING;
        INIT_WORK(&ns->proc_work, proc_cleanup_work);

        return ns; // Returns with ->proc_mnt == NULL.

out_free_idr:

        idr_destroy(&ns->idr);
        kmem_cache_free(pid_ns_cachep, ns);
out_dec:
        dec_pid_namespaces(ucounts);
out:
        return ERR_PTR(err);
}

struct pid *alloc_pid(struct pid_namespace *ns)
{
(...snipped...)

        if (unlikely(is_child_reaper(pid))) {
                if (pid_ns_prepare_proc(ns)) {
                        disable_pid_allocation(ns);
                        goto out_free;
                }
        }

(...snipped...)
}

int pid_ns_prepare_proc(struct pid_namespace *ns)

{
        struct vfsmount *mnt;

        mnt = kern_mount_data(&proc_fs_type, ns);
        if (IS_ERR(mnt))
                return PTR_ERR(mnt);

        ns->proc_mnt = mnt;
        return 0;
}

----------

If one of copy_io(), copy_thread_tls() or alloc_pid() returns an error, creation of a child fails with p->nsproxy->pid_ns_for_children->proc_mnt == NULL.

Then, when the child exits, the parent waiting at wait() calls proc_flush_task() which assumes that everything was set up properly.

----------
void proc_flush_task(struct task_struct *task)

{
        int i;
        struct pid *pid, *tgid;
        struct upid *upid;

        pid = task_pid(task);
        tgid = task_tgid(task);

        for (i = 0; i <= pid->level; i++) {
                upid = &pid->numbers[i];
                proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
                                        tgid->numbers[i].nr);
        }
}

----------

Quoted 4 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Telling it to just fuzz execveat doesn't instantly trigger it, so it must be a combination of some other syscall. I'll leave a script running overnight to see if I can binary search the other syscalls in combination with it.

One other thing: I said this was rc4, but it was actually rc4 + all the x86 stuff from today. There's enough creepy stuff in that pile, that I'll try with just plain rc4 tomorrow too.

Quoted 4 lines · Dave Jones

Quoted 2 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Quoted 5 lines

Quoted 2 lines

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Is there any chance the excveat might be called from a child thread?

That switching pids between tasks of a process during exec can get a little bit tricky.

Quoted 4 lines

Telling it to just fuzz execveat doesn't instantly trigger it, so it must be a combination of some other syscall. I'll leave a script running overnight to see if I can binary search the other syscalls in combination with it.

Could we have a buggy syscall that is stomping something?

Quoted 9 lines · Eric W. Biederman

Quoted 7 lines · Dave Jones

Quoted 4 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Quoted 4 lines

Quoted 2 lines

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Is there any chance the excveat might be called from a child thread?

If trinity choose one of the exec syscalls, it forks off an extra child to do it in, on the off-chance that it succeeds, and we never return.
https://github.com/kernelslacker/trinity/blob/master/syscall.c#L139

Quoted 10 lines · Dave Jones

Quoted 9 lines · Eric W. Biederman

Quoted 7 lines · Dave Jones

Quoted 4 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Is there any chance the excveat might be called from a child thread?

If trinity choose one of the exec syscalls, it forks off an extra child to do it in, on the off-chance that it succeeds, and we never return.
https://github.com/kernelslacker/trinity/blob/master/syscall.c#L139

	extrapid = fork();
	if (extrapid == 0) {
		/* grand-child */
		char childname[]="trinity-subchild";
		prctl(PR_SET_NAME, (unsigned long) &childname);

		__do_syscall(rec, GOING_AWAY);
		/* if this was for eg. an successful execve, we should never get here.
		 * if it failed though... */
		_exit(EXIT_SUCCESS);
	}

That is interesting.

So the system call sequence is a fork which just succeeded and than an exec. That reduces the possibilities quite a lot.

With pids there was a recent change that just replaced the pid hash table and the pid bitmap with and idr. It changes the locking somewhat and probably changes the timing so that might be the culprit.

I am trying to figure out if there is an interface that would let ns_last_pid for a pid namespace be accessed before the first pid is allocated and I am not seeing it. It does not appear to be possible to mount a proc for a pid namespace you are not currently in.

*Scratches my head* I am not seeing anything obvious.

Quoted 1 line

*Scratches my head* I am not seeing anything obvious.

Can you try this patch as you reproduce this issue?

kernel/pid.c +1 -0

index b13b624e2c49..df9e5d4d8f83 100644
@@ -210,6 +210,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
                goto out_unlock;
        for ( ; upid >= pid->numbers; --upid) {
                /* Make the PID visible to find_pid_ns. */
+               WARN_ON(!upid->ns->proc_mnt);
                idr_replace(&upid->ns->idr, pid, upid->nr);
                upid->ns->pid_allocated++;
        }

If the warning triggers it means the bug is in alloc_pid and somehow something has gotten past the is_child_reaper check.

Quoted 43 lines · [email protected] (Eric W. Biederman) writes:

Quoted 10 lines · Dave Jones

Quoted 9 lines · Eric W. Biederman

Quoted 7 lines · Dave Jones

Quoted 4 lines · Linus Torvalds

But I don't see what would have changed in this area recently.

Do you end up saving the seeds that cause crashes? Is this reproducible? (Other than seeing it twoce, of course)

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Only clue so far, is every time I'm able to trigger it, the last thing the child process that triggers it did, was an execveat.

Is there any chance the excveat might be called from a child thread?

If trinity choose one of the exec syscalls, it forks off an extra child to do it in, on the off-chance that it succeeds, and we never return.
https://github.com/kernelslacker/trinity/blob/master/syscall.c#L139

      extrapid = fork();
      if (extrapid == 0) {
              /* grand-child */
              char childname[]="trinity-subchild";
              prctl(PR_SET_NAME, (unsigned long) &childname);

              __do_syscall(rec, GOING_AWAY);
              /* if this was for eg. an successful execve, we should never get here.
               * if it failed though... */
              _exit(EXIT_SUCCESS);
      }

That is interesting.

So the system call sequence is a fork which just succeeded and than an exec. That reduces the possibilities quite a lot.

With pids there was a recent change that just replaced the pid hash table and the pid bitmap with and idr. It changes the locking somewhat and probably changes the timing so that might be the culprit.

Quoted 6 lines

I am trying to figure out if there is an interface that would let ns_last_pid for a pid namespace be accessed before the first pid is allocated and I am not seeing it. It does not appear to be possible to mount a proc for a pid namespace you are not currently in.

*Scratches my head* I am not seeing anything obvious.

Can you try this patch as you reproduce this issue?

kernel/pid.c +1 -0

index b13b624e2c49..df9e5d4d8f83 100644
@@ -210,6 +210,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
                goto out_unlock;
        for ( ; upid >= pid->numbers; --upid) {
                /* Make the PID visible to find_pid_ns. */
+               WARN_ON(!upid->ns->proc_mnt);
                idr_replace(&upid->ns->idr, pid, upid->nr);
                upid->ns->pid_allocated++;
        }

If the warning triggers it means the bug is in alloc_pid and somehow something has gotten past the is_child_reaper check.

If the warning does not trigger it means something is stomping proc_mnt.

In the entire kernel there are exactly two assignments to proc_mnt.
- kmem_cache_zalloc in create_pid_namespace.
- In pid_ns_prepare_proc where proc_mnt is set to a non-zero value.

On the 29th of Nov syzkaller also hit this and gave me this reproducer that I can't figure out heads or tails of.

#{Threaded:true Collide:true Repeat:true Procs:8 Sandbox:namespace Fault:false FaultCall:-1 FaultNth:0 EnableTun:true UseTmpDir:true HandleSegv:true WaitRepeat:true Debug:false Repro:false}
mmap(&(0x7f0000000000/0xfff000)=nil, 0xfff000, 0x3, 0x32, 0xffffffffffffffff, 0x0)
perf_event_open(&(0x7f000025c000)={0x2, 0x78, 0x3e3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xf72, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, 0x0, 0xffffffffffffffff, 0xffffffffffffffff, 0x0)
socket$inet6_dccp(0xa, 0x6, 0x0)
unshare(0x20000400)
sendmsg$unix(0xffffffffffffffff, &(0x7f0000001000-0x38)={&(0x7f0000239000-0x8)=@abs={0x0, 0x0, 0x0}, 0x8, &(0x7f0000008000)=[], 0x0, &(0x7f0000001000-0x10)=[@rights={0x200, 0x1, 0x1, [0xffffffffffffffff]}], 0x1, 0x0}, 0x0)
process_vm_writev(0x0, &(0x7f0000699000-0x70)=[{&(0x7f00006a5000)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x4c}, {&(0x7f00007b9000-0x54)="", 0x0}, {&(0x7f00004f3000)="", 0x0}, {&(0x7f00002e3000-0xd6)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0xd6}, {&(0x7f0000f2e000)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x52}, {&(0x7f00008e5000-0x10)="00000000000000000000000000000000", 0x10}, {&(0x7f0000a3a000)="", 0x0}], 0x7, &(0x7f0000d05000)=[{&(0x7f0000d64000)="", 0x0}, {&(0x7f0000062000-0x93)="", 0x0}, {&(0x7f0000a16000-0x7e)="000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x7e}, {&(0x7f00003dc000-0x9a)="", 0x0}, {&(0x7f0000fe3000-0xc7)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0xc7}], 0x5, 0x0) pselect6(0x40, &(0x7f0000cc9000-0x40)={0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x20000, 0x0}, &(0x7f0000cc9000-0x40)={0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, &(0x7f00000de000-0x40)={0xffffffffffffffe1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1}, &(0x7f00008e6000-0x10)={0x0, 0x989680}, &(0x7f0000205000-0x10)={&(0x7f00006e4000-0x8)={0x0}, 0x8})
clone(0x20900, &(0x7f0000a94000-0x1)="6f", &(0x7f00002b8000-0x4)=0x0, &(0x7f000029e000)=0x0, &(0x7f00006fe000)="")
ioctl$KVM_ENABLE_CAP_CPU(0xffffffffffffffff, 0x4068aea3, &(0x7f0000e48000)={0x7b, 0x0, [0x1, 0x1, 0x800, 0x1], [0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0]})
epoll_ctl$EPOLL_CTL_DEL(0xffffffffffffffff, 0x2, 0xffffffffffffffff)

#{Threaded:true Collide:true Repeat:true Procs:8 Sandbox:namespace Fault:false FaultCall:-1 FaultNth:0 EnableTun:true UseTmpDir:true HandleSegv:true WaitRepeat:true Debug:false Repro:false}
mmap(&(0x7f0000000000/0xfff000)=nil, 0xfff000, 0x3, 0x32, 0xffffffffffffffff, 0x0)
perf_event_open(&(0x7f000025c000)={0x2, 0x78, 0x3e3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xf72, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, 0x0, 0xffffffffffffffff, 0xffffffffffffffff, 0x0)
socket$inet6_dccp(0xa, 0x6, 0x0)
unshare(0x20000400)
sendmsg$unix(0xffffffffffffffff, &(0x7f0000001000-0x38)={&(0x7f0000239000-0x8)=@abs={0x0, 0x0, 0x0}, 0x8, &(0x7f0000008000)=[], 0x0, &(0x7f0000001000-0x10)=[@rights={0x200, 0x1, 0x1, [0xffffffffffffffff]}], 0x1, 0x0}, 0x0)
process_vm_writev(0x0, &(0x7f0000699000-0x70)=[{&(0x7f00006a5000)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x4c}, {&(0x7f00007b9000-0x54)="", 0x0}, {&(0x7f00004f3000)="", 0x0}, {&(0x7f00002e3000-0xd6)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0xd6}, {&(0x7f0000f2e000)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x52}, {&(0x7f00008e5000-0x10)="00000000000000000000000000000000", 0x10}, {&(0x7f0000a3a000)="", 0x0}], 0x7, &(0x7f0000d05000)=[{&(0x7f0000d64000)="", 0x0}, {&(0x7f0000062000-0x93)="", 0x0}, {&(0x7f0000a16000-0x7e)="000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0x7e}, {&(0x7f00003dc000-0x9a)="", 0x0}, {&(0x7f0000fe3000-0xc7)="00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", 0xc7}], 0x5, 0x0) pselect6(0x40, &(0x7f0000cc9000-0x40)={0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x20000, 0x0}, &(0x7f0000cc9000-0x40)={0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, &(0x7f00000de000-0x40)={0xffffffffffffffe1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1}, &(0x7f00008e6000-0x10)={0x0, 0x989680}, &(0x7f0000205000-0x10)={&(0x7f00006e4000-0x8)={0x0}, 0x8})
clone(0x20900, &(0x7f0000a94000-0x1)="6f", &(0x7f00002b8000-0x4)=0x0, &(0x7f000029e000)=0x0, &(0x7f00006fe000)="")
ioctl$KVM_ENABLE_CAP_CPU(0xffffffffffffffff, 0x4068aea3, &(0x7f0000e48000)={0x7b, 0x0, [0x1, 0x1, 0x800, 0x1], [0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0]})
epoll_ctl$EPOLL_CTL_DEL(0xffffffffffffffff, 0x2, 0xffffffffffffffff)

Eric

--
You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/87mv2e17vz.fsf%40xmission.com.
For more options, visit https://groups.google.com/d/optout.

Quoted 18 lines · Eric W. Biederman

Quoted 1 line

*Scratches my head* I am not seeing anything obvious.

Can you try this patch as you reproduce this issue?

kernel/pid.c +1 -0

index b13b624e2c49..df9e5d4d8f83 100644
@@ -210,6 +210,7 @@ struct pid *alloc_pid(struct pid_namespace *ns)
                goto out_unlock;
        for ( ; upid >= pid->numbers; --upid) {
                /* Make the PID visible to find_pid_ns. */
+               WARN_ON(!upid->ns->proc_mnt);
                idr_replace(&upid->ns->idr, pid, upid->nr);
                upid->ns->pid_allocated++;
        }

Quoted 2 lines

If the warning triggers it means the bug is in alloc_pid and somehow something has gotten past the is_child_reaper check.

You're onto something.

WARNING: CPU: 1 PID: 12020 at kernel/pid.c:213 alloc_pid+0x230/0x280
CPU: 1 PID: 12020 Comm: trinity-c29 Not tainted 4.15.0-rc4-think+ #3 
RIP: 0010:alloc_pid+0x230/0x280
RSP: 0018:ffffc90009977d48 EFLAGS: 00010046
RAX: 0000000000000030 RBX: ffff8804fb431280 RCX: 8f5c28f5c28f5c29
RDX: ffff88050a00de40 RSI: ffffffff82005218 RDI: ffff8804fc6aa9a8
RBP: ffff8804fb431270 R08: 0000000000000000 R09: 0000000000000001
R10: ffffc90009977cc0 R11: eab94e31da7171b7 R12: ffff8804fb431260
R13: ffff8804fb431240 R14: ffffffff82005200 R15: ffff8804fb431268
FS:  00007f49b9065700(0000) GS:ffff88050a000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f49b906a000 CR3: 00000004f7446001 CR4: 00000000001606e0
DR0: 00007f0b4c405000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
Call Trace:
 copy_process.part.41+0x14fa/0x1e30
 _do_fork+0xe7/0x720
 ? rcu_read_lock_sched_held+0x6c/0x80
 ? syscall_trace_enter+0x2d7/0x340
 do_syscall_64+0x60/0x210
 entry_SYSCALL64_slow_path+0x25/0x25

followed immediately by...

Oops: 0000 [#1] SMP
CPU: 1 PID: 12020 Comm: trinity-c29 Tainted: G        W        4.15.0-rc4-think+ #3 
RIP: 0010:proc_flush_task+0x8e/0x1b0
RSP: 0018:ffffc90009977c40 EFLAGS: 00010286
RAX: 0000000000000001 RBX: 0000000000000001 RCX: 00000000fffffffb
RDX: 0000000000000000 RSI: ffffc90009977c50 RDI: 0000000000000000
RBP: ffffc90009977c63 R08: 0000000000000000 R09: 0000000000000002
R10: ffffc90009977b70 R11: ffffc90009977c64 R12: 0000000000000004
R13: 0000000000000000 R14: 0000000000000004 R15: ffff8804fb431240
FS:  00007f49b9065700(0000) GS:ffff88050a000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 00000004f7446001 CR4: 00000000001606e0
DR0: 00007f0b4c405000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000600
Call Trace:
 ? release_task+0xaf/0x680
 release_task+0xd2/0x680
 ? wait_consider_task+0xb82/0xce0
 wait_consider_task+0xbe9/0xce0
 ? do_wait+0xe1/0x330
 do_wait+0x151/0x330

 kernel_wait4+0x8d/0x150
 ? task_stopped_code+0x50/0x50
 SYSC_wait4+0x95/0xa0
 ? rcu_read_lock_sched_held+0x6c/0x80
 ? syscall_trace_enter+0x2d7/0x340
 ? do_syscall_64+0x60/0x210
 do_syscall_64+0x60/0x210
 entry_SYSCALL64_slow_path+0x25/0x25

Quoted 6 lines

Quoted 3 lines

If the warning triggers it means the bug is in alloc_pid and somehow something has gotten past the is_child_reaper check.

You're onto something.

I am not seeing where things go wrong, but that puts the recent pid bitmap, bit hash to idr change in the suspect zone.

Can you try reverting that change:

e8cfbc245e24 ("pid: remove pidhash")
95846ecf9dac ("pid: replace pid bitmap implementation with IDR API")

While keeping the warning in place so we can see if this fixes the allocation problem?

Quoted 14 lines · Eric W. Biederman

Quoted 6 lines

Quoted 3 lines

If the warning triggers it means the bug is in alloc_pid and somehow something has gotten past the is_child_reaper check.

You're onto something.

I am not seeing where things go wrong, but that puts the recent pid bitmap, bit hash to idr change in the suspect zone.

Can you try reverting that change:

e8cfbc245e24 ("pid: remove pidhash")
95846ecf9dac ("pid: replace pid bitmap implementation with IDR API")

Quoted 2 lines

While keeping the warning in place so we can see if this fixes the allocation problem?

So I can't trigger this any more with those reverted. I seem to hit a bunch of other long-standing bugs first. I'll keep running it overnight, but it looks like this is where the problem lies.

I have stared at this code, and written some test programs and I can't see what is going on. alloc_pid by design and in implementation (as far as I can see) is always single threaded when allocating the first pid in a pid namespace. idr_init always initialized idr_next to 0.

So how we can get past:

	if (unlikely(is_child_reaper(pid))) {
		if (pid_ns_prepare_proc(ns)) {
			disable_pid_allocation(ns);
			goto out_free;
		}
	}

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Quoted 14 lines · Eric W. Biederman

I have stared at this code, and written some test programs and I can't see what is going on. alloc_pid by design and in implementation (as far as I can see) is always single threaded when allocating the first pid in a pid namespace. idr_init always initialized idr_next to 0.

So how we can get past:

	if (unlikely(is_child_reaper(pid))) {
		if (pid_ns_prepare_proc(ns)) {
			disable_pid_allocation(ns);
			goto out_free;
		}
	}

Quoted 4 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

There is a "valid" way to break the code via kernel.ns_last_pid:
unshare+write+fork but the reproducer doesn't seem to use it (or it does?)

Quoted 20 lines · Alexey Dobriyan

Quoted 18 lines · Eric W. Biederman

I have stared at this code, and written some test programs and I can't see what is going on. alloc_pid by design and in implementation (as far as I can see) is always single threaded when allocating the first pid in a pid namespace. idr_init always initialized idr_next to 0.

So how we can get past:

	if (unlikely(is_child_reaper(pid))) {
		if (pid_ns_prepare_proc(ns)) {
			disable_pid_allocation(ns);
			goto out_free;
		}
	}

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Quoted 1 line

Well, this theory is easy to test (attached).

I'll give this a shot and report back when I get to the office.

Quoted 2 lines

There is a "valid" way to break the code via kernel.ns_last_pid:
unshare+write+fork but the reproducer doesn't seem to use it (or it does?)

that sysctl is root only, so that isn't at play here.

Quoted 4 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

Quoted 6 lines · Alexey Dobriyan

Quoted 4 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

I didn't hit this BUG, but I hit the same oops in proc_flush_task.

Quoted 10 lines · Dave Jones

Quoted 6 lines · Alexey Dobriyan

Quoted 4 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

I didn't hit this BUG, but I hit the same oops in proc_flush_task.

Scratch one idea.

If it isn't too much trouble can you try this.

I am wondering if somehow the proc_mnt that is NULL is somewhere in the middle of the stack of pid namespaces.

This adds two warnings. The first just reports which pid namespace in the stack of pid namespaces is problematic, and the pid number in that pid namespace. Which should give a whole lot more to go by.

The second warning complains if we manage to create a pid namespace where the parent pid namespace is not properly set up. The test to prevent that looks quite robust, but at this point I don't know where to look.

Quoted 20 lines · Eric W. Biederman

Quoted 10 lines · Dave Jones

Quoted 6 lines · Alexey Dobriyan

Quoted 4 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the lowest valid free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

I didn't hit this BUG, but I hit the same oops in proc_flush_task.

Scratch one idea.

If it isn't too much trouble can you try this.

I am wondering if somehow the proc_mnt that is NULL is somewhere in the middle of the stack of pid namespaces.

Quoted 8 lines

This adds two warnings. The first just reports which pid namespace in the stack of pid namespaces is problematic, and the pid number in that pid namespace. Which should give a whole lot more to go by.

The second warning complains if we manage to create a pid namespace where the parent pid namespace is not properly set up. The test to prevent that looks quite robust, but at this point I don't know where to look.

Progress ?

[ 1653.030190] ------------[ cut here ]------------
[ 1653.030852] 1/1: 2 no proc_mnt
[ 1653.030946] WARNING: CPU: 2 PID: 4420 at kernel/pid.c:213 alloc_pid+0x24f/0x2a0

Quoted 31 lines · Dave Jones

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Dave Jones

Quoted 3 lines · Alexey Dobriyan

Quoted 3 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the

lowest valid

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

I didn't hit this BUG, but I hit the same oops in proc_flush_task.

Scratch one idea.

If it isn't too much trouble can you try this.

I am wondering if somehow the proc_mnt that is NULL is somewhere in

the

Quoted 9 lines

middle of the stack of pid namespaces.

This adds two warnings. The first just reports which pid namespace in the stack of pid namespaces is problematic, and the pid number in that pid namespace. Which should give a whole lot more to go by.

The second warning complains if we manage to create a pid namespace where the parent pid namespace is not properly set up. The test to prevent that looks quite robust, but at this point I don't know where

to

Quoted 8 lines

Quoted 1 line

look.

Progress ?

[ 1653.030190] ------------[ cut here ]------------
[ 1653.030852] 1/1: 2 no proc_mnt
[ 1653.030946] WARNING: CPU: 2 PID: 4420 at kernel/pid.c:213

alloc_pid+0x24f/0x2a0

Yes. I don't know why Alexey's patch did not fire but this is confirmation that the first pid allocated was #2 and not #1.

Which explains the pid_mnt not being set, and it is definitely the new code, changing from the pid bitmap+hash table to an idr.

So it looks like idr_alloc_cyclic in some configuration for the first allocation returns value #2 instead of value #1.

I don't know that code, and it is quite complicated so I will have to stare at it some more to even guess why it is doing that.

This is confirmation that reverting those pid changes will fix the problem. As they are definitely at fault.

Hmm. After a little more staring I have a hunch what is going wrong. It is just possible that there is a failure in alloc_pid during the first pid allocation and then idr_next gets left at 2. I need to sleep before I can think of a patch to test that.

Hmm. A failure and then restart would also explain why Alexey's patch did not fire. An incomplete reset of state.

Quoted 58 lines · Eric W. Biederman

Quoted 31 lines · Dave Jones

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Dave Jones

Quoted 3 lines · Alexey Dobriyan

Quoted 3 lines

with proc_mnt still set to NULL is a mystery to me.

Is there any chance the idr code doesn't always return the

lowest valid

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

free number? So init gets assigned something other than 1?

Well, this theory is easy to test (attached).

I didn't hit this BUG, but I hit the same oops in proc_flush_task.

Scratch one idea.

If it isn't too much trouble can you try this.

I am wondering if somehow the proc_mnt that is NULL is somewhere in

the

Quoted 9 lines

middle of the stack of pid namespaces.

This adds two warnings. The first just reports which pid namespace in the stack of pid namespaces is problematic, and the pid number in that pid namespace. Which should give a whole lot more to go by.

The second warning complains if we manage to create a pid namespace where the parent pid namespace is not properly set up. The test to prevent that looks quite robust, but at this point I don't know where

to

Quoted 8 lines

Quoted 1 line

look.

Progress ?

[ 1653.030190] ------------[ cut here ]------------
[ 1653.030852] 1/1: 2 no proc_mnt
[ 1653.030946] WARNING: CPU: 2 PID: 4420 at kernel/pid.c:213

alloc_pid+0x24f/0x2a0

Yes. I don't know why Alexey's patch did not fire but this is confirmation that the first pid allocated was #2 and not #1.

Which explains the pid_mnt not being set, and it is definitely the new code, changing from the pid bitmap+hash table to an idr.

So it looks like idr_alloc_cyclic in some configuration for the first allocation returns value #2 instead of value #1.

I don't know that code, and it is quite complicated so I will have to stare at it some more to even guess why it is doing that.

This is confirmation that reverting those pid changes will fix the problem. As they are definitely at fault.

Quoted 7 lines

Hmm. After a little more staring I have a hunch what is going wrong. It is just possible that there is a failure in alloc_pid during the first pid allocation and then idr_next gets left at 2. I need to sleep before I can think of a patch to test that.

Hmm. A failure and then restart would also explain why Alexey's patch did not fire. An incomplete reset of state.

You are right (you are also right about sysctl :-\)

unshare
fork
alloc_pid in level 1 succeeds
alloc_pid in level 0 fails, ->idr_next is 2
fork
alloc pid 2
exit

Reliable reproducer and fail injection patch attached

I'd say proper fix is allocating pids in the opposite order so that failure in the last layer doesn't move IDR cursor in baby child pidns.

Quoted 10 lines · Alexey Dobriyan

unshare
fork
alloc_pid in level 1 succeeds
alloc_pid in level 0 fails, ->idr_next is 2
fork
alloc pid 2
exit

Reliable reproducer and fail injection patch attached

Quoted 3 lines

I'd say proper fix is allocating pids in the opposite order so that failure in the last layer doesn't move IDR cursor in baby child pidns.

I agree with you about changing the order. That will make the code simpler and in the second loop actually conforming C code, and fix the immediate problem.

Quoted 16 lines · Eric W. Biederman

Quoted 13 lines · Alexey Dobriyan

unshare
fork
alloc_pid in level 1 succeeds
alloc_pid in level 0 fails, ->idr_next is 2
fork
alloc pid 2
exit

Reliable reproducer and fail injection patch attached

I'd say proper fix is allocating pids in the opposite order so that failure in the last layer doesn't move IDR cursor in baby child pidns.

Quoted 3 lines

I agree with you about changing the order. That will make the code simpler and in the second loop actually conforming C code, and fix the immediate problem.

Something like that (barely tested)