: Crash with CONFIG_FAIR_CGROUP_SCHED=y

	$ mount -t cgroup none /cgroups
	$ ./ns_exec -cm /bin/ls
Oops: 0000 [1] Oops: 0000 [1] SMP SMP
Modules linked in:Modules linked in:
RIP: 0010:[<0000000000000000>] RIP: 0010:[<0000000000000000>]  [<0000000000000000>]
 [<0000000000000000>]
RSP: 0018:ffff8101006a6af0  EFLAGS: 00055292
RSP: 0018:ffff8101006a6af0  EFLAGS: 00055292
RAX: 0000000000000000 RBX: ffff810100d11140 RCX: ffff810101de4000
RAX: 0000000000000000 RBX: ffff810100d11140 RCX: ffff810101de4000
RDX: 0000000000000000 RSI: ffff810100d1a880 RDI: ffff810001037c00
RDX: 0000000000000000 RSI: ffff810100d1a880 RDI: ffff810001037c00
RBP: ffff810102c136c0 R08: ffff810101de4000 R09: ffff810101d31bb8
RBP: ffff810102c136c0 R08: ffff810101de4000 R09: ffff810101d31bb8
R10: 0000000000000000 R11: 00000000ffffffff R12: ffff8101034075b8
R10: 0000000000000000 R11: 00000000ffffffff R12: ffff8101034075b8
R13: ffff8101029d6028 R14: ffff810103407500 R15: ffffffff80869d00
R13: ffff8101029d6028 R14: ffff810103407500 R15: ffffffff80869d00
FS:  00002b80c2a396f0(0000) GS:ffff81010068f3c0(0000) knlGS:0000000000000000
FS:  00002b80c2a396f0(0000) GS:ffff81010068f3c0(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000000 CR3: 00000001028be000 CR4: 00000000000006e0
CR2: 0000000000000000 CR3: 00000001028be000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process ns_exec (pid: 3273, threadinfo ffff810101de4000, task ffff810100d11140)
Process ns_exec (pid: 3273, threadinfo ffff810101de4000, task ffff810100d11140)
Stack: Stack:  0000000000000000 0000000000000000 0000000000000001 0000000000000001 6c2f343662696c2f 6c2f343662696c2f 2d78756e696c2d64 2d78756e696c2d64
Call Trace:
Call Trace:




Code: Code:  Bad RIP value. Bad RIP value.
# shared options
#
CONFIG_X86_ACPI_CPUFREQ_PROC_INTF=y
# CONFIG_X86_SPEEDSTEP_LIB is not set
# CONFIG_CPU_IDLE is not set
# may also be needed; see USB_STORAGE Help for more information
#
CONFIG_USB_STORAGE=y
# CONFIG_USB_STORAGE_DEBUG is not set
# CONFIG_USB_STORAGE_DATAFAB is not set
# CONFIG_USB_STORAGE_FREECOM is not set
# CONFIG_USB_STORAGE_ISD200 is not set
# CONFIG_USB_STORAGE_DPCM is not set
# CONFIG_USB_STORAGE_USBAT is not set
# CONFIG_USB_STORAGE_SDDR09 is not set
# CONFIG_USB_STORAGE_SDDR55 is not set
# CONFIG_USB_STORAGE_JUMPSHOT is not set
# CONFIG_USB_STORAGE_ALAUDA is not set
# CONFIG_USB_STORAGE_KARMA is not set
# CONFIG_USB_LIBUSUAL is not set
{
	printf("usage: %s [-h] [-c] [-muipq]"
			"[command [arg ..]]\n", name);
	printf("\n");
	printf("  -h		this message\n");
	printf("\n");
	printf("  -c		use 'clone' rather than 'unshare' system call\n");
	printf("  -m		mount namespace\n");
	printf("  -u		utsname namespace\n");
	printf("  -i		ipc namespace\n");
	printf("  -p		pid namespace\n");
	printf("  -q		mqueue namespace\n");
	printf("\n");
	printf("(C) Copyright IBM Corp. 2006\n");
	printf("\n");
	exit(1);
}
opentty(const char * tty) {
        int i, fd, flags;

        fd = open(tty, O_RDWR | O_NONBLOCK);
        if (fd == -1) {
		printf("FATAL: can't reopen tty: %s", strerror(errno));
                sleep(1);
                exit(1);
        }

        flags = fcntl(fd, F_GETFL);
        flags &= ~O_NONBLOCK;
        fcntl(fd, F_SETFL, flags);

        for (i = 0; i < fd; i++)
                close(i);
        for (i = 0; i < 3; i++)
                if (fd != i)
                        dup2(fd, i);
        if (fd >= 3)
                close(fd);
}
{
	char **argv = (char **)vargv;
	execve(argv[0], argv, __environ);
	perror("execve");
	return 1;
}
{	
	int c;
	unsigned long flags = 0;
	char ttyname[256];
	int status;
	int ret, use_clone = 0;
	int pid;

	procname = basename(argv[0]);

	memset(ttyname, '\0', sizeof(ttyname));
	readlink("/proc/self/fd/0", ttyname, sizeof(ttyname));

	while ((c = getopt(argc, argv, "+muiphqc")) != EOF) {
		switch (c) {
		case 'm': flags |= CLONE_NEWNS;  break;
		case 'c': use_clone = 1; break;
		case 'u': flags |= CLONE_NEWUTS;  break;
		case 'i': flags |= CLONE_NEWIPC;  break;
		case 'p': flags |= CLONE_NEWPID|CLONE_NEWNS;  break;
		case 'q': flags |= CLONE_NEWMQ;  break;
		case 'h':
		default:
			usage(procname);
		}
	};
    
	argv = &argv[optind];
	argc = argc - optind;	
	
	if (use_clone) {
		void *childstack, *stack = malloc(getpagesize());

		if (!stack) {
			perror("malloc");
			return -1;
		}
		childstack = stack + getpagesize();

		printf("about to clone with %lx\n", flags);
		pid = clone(do_child, childstack, flags, (void *)argv);
		if (pid == -1) {
			perror("clone");
			return -1;
		}
	} else {
		if ((pid = fork()) == 0) {
			// Child.
			print_my_info(procname, ttyname);

			opentty(ttyname);

			printf("about to unshare with %lx\n", flags);
			ret = unshare(flags);
			if (ret < 0) {
				perror("unshare");
				return 1;
			}		
			
			return do_child((void*)argv);
		}

	}
	printf("Parent waiting for pid %d\n", pid);
	if ((ret = waitpid(pid, &status, __WALL)) < 0)
		printf("waitpid() returns %d, errno %d\n", ret, errno);

	exit(0);
}
---
 kernel/sched.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)
Index: current/kernel/sched.c
===================================================================
@@ -3986,11 +3986,13 @@ void rt_mutex_setprio(struct task_struct
 	oldprio = p->prio;
 	on_rq = p->se.on_rq;
 	running = task_running(rq, p);
-	if (on_rq) {
+	if (on_rq)
 		dequeue_task(rq, p, 0);
-		if (running)
-			p->sched_class->put_prev_task(rq, p);
-	}
+	/* current task is not kept in its runqueue in case of sched_fair class.
+	 * Hence we need the 'on_rq?' and 'running?' tests to be separate.
+	 */
+	if (running)
+		p->sched_class->put_prev_task(rq, p);
 
 	if (rt_prio(prio))
 		p->sched_class = &rt_sched_class;

@@ -3999,9 +4001,9 @@ void rt_mutex_setprio(struct task_struct
 
 	p->prio = prio;
 
+	if (running)
+		p->sched_class->set_curr_task(rq);
 	if (on_rq) {
-		if (running)
-			p->sched_class->set_curr_task(rq);
 		enqueue_task(rq, p, 0);
 		inc_load(rq, p);
 		/*

@@ -4298,18 +4300,20 @@ recheck:
 	update_rq_clock(rq);
 	on_rq = p->se.on_rq;
 	running = task_running(rq, p);
-	if (on_rq) {
+	if (on_rq)
 		deactivate_task(rq, p, 0);
-		if (running)
-			p->sched_class->put_prev_task(rq, p);
-	}
+	/* current task is not kept in its runqueue in case of sched_fair class.
+	 * Hence we need the 'on_rq?' and 'running?' tests to be separate.
+	 */
+	if (running)
+		p->sched_class->put_prev_task(rq, p);
 
 	oldprio = p->prio;
 	__setscheduler(rq, p, policy, param->sched_priority);
 
+	if (running)
+		p->sched_class->set_curr_task(rq);
 	if (on_rq) {
-		if (running)
-			p->sched_class->set_curr_task(rq);
 		activate_task(rq, p, 0);
 		/*
 		 * Reschedule if we are currently running on this runqueue and

@@ -7036,19 +7040,20 @@ void sched_move_task(struct task_struct 
 	running = task_running(rq, tsk);
 	on_rq = tsk->se.on_rq;
 
-	if (on_rq) {
+	if (on_rq)
 		dequeue_task(rq, tsk, 0);
-		if (unlikely(running))
-			tsk->sched_class->put_prev_task(rq, tsk);
-	}
+	/* current task is not kept in its runqueue in case of sched_fair class.
+	 * Hence we need the 'on_rq?' and 'running?' tests to be separate.
+	 */
+	if (unlikely(running))
+		tsk->sched_class->put_prev_task(rq, tsk);
 
 	set_task_cfs_rq(tsk);
 
-	if (on_rq) {
-		if (unlikely(running))
-			tsk->sched_class->set_curr_task(rq);
+	if (unlikely(running))
+		tsk->sched_class->set_curr_task(rq);
+	if (on_rq)
 		enqueue_task(rq, tsk, 0);
-	}
 
 done:
 	task_rq_unlock(rq, &flags);

Regards,
vatsa
---
 kernel/sched.c |   45 +++++++++++++++++++++++++--------------------
 1 files changed, 25 insertions(+), 20 deletions(-)
@@ -3986,11 +3986,13 @@ void rt_mutex_setprio(struct task_struct
        oldprio = p->prio;

        on_rq = p->se.on_rq;
        running = task_running(rq, p);
-       if (on_rq) {
+       if (on_rq)
                dequeue_task(rq, p, 0);
-               if (running)
-                       p->sched_class->put_prev_task(rq, p);
-       }
+       /* current task is not kept in its runqueue in case of sched_fair class.
+        * Hence we need the 'on_rq?' and 'running?' tests to be separate.

--
 Regards,

Best regards,
Dmitry Adamushko
	# mkdir /dev/cpuctl
	# mount -t cgroup -ocpu,ns none cpuctl
	(or simply)
	# mount -t cgroup none cpuctl -> Will activate all controllers
					 in same hierarchy.
---
 kernel/fork.c       |    6 +++---
 kernel/sched_fair.c |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
Index: current/kernel/fork.c
===================================================================
@@ -1121,6 +1121,9 @@ static struct task_struct *copy_process(
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+	/* Perform scheduler related setup. Assign this task to a CPU. */
+	sched_fork(p, clone_flags);
+
 	if ((retval = security_task_alloc(p)))
 		goto bad_fork_cleanup_policy;
 	if ((retval = audit_alloc(p)))

@@ -1210,9 +1213,6 @@ static struct task_struct *copy_process(
 	INIT_LIST_HEAD(&p->ptrace_children);
 	INIT_LIST_HEAD(&p->ptrace_list);
 
-	/* Perform scheduler related setup. Assign this task to a CPU. */
-	sched_fork(p, clone_flags);
-
 	/* Now that the task is set up, run cgroup callbacks if
 	 * necessary. We need to run them before the task is visible
 	 * on the tasklist. */

Index: current/kernel/sched_fair.c
===================================================================
@@ -1023,7 +1023,7 @@ static void task_new_fair(struct rq *rq,
 	place_entity(cfs_rq, se, 1);
 
 	if (sysctl_sched_child_runs_first && this_cpu == task_cpu(p) &&
-			curr->vruntime < se->vruntime) {
+			curr && curr->vruntime < se->vruntime) {
 		/*
 		 * Upon rescheduling, sched_class::put_prev_task() will place
 		 * 'current' within the tree based on its new key value.

Best regards,
Dmitry Adamushko
---
 kernel/sched_fair.c |    1 +
 1 files changed, 1 insertion(+)
Index: current/kernel/sched_fair.c
===================================================================
@@ -1022,6 +1022,7 @@ static void task_new_fair(struct rq *rq,
 	update_curr(cfs_rq);
 	place_entity(cfs_rq, se, 1);
 
+	/* 'curr' will be NULL if the child belongs to a different group */
 	if (sysctl_sched_child_runs_first && this_cpu == task_cpu(p) &&
 			curr && curr->vruntime < se->vruntime) {
 		/*

Regards,
vatsa
	# mkdir /dev/cpuctl
	# mount -t cgroup -ocpu,ns none cpuctl
	(or simply)
	# mount -t cgroup none cpuctl -> Will activate all controllers
					 in same hierarchy.
---
 kernel/fork.c       |    6 +++---
 kernel/sched_fair.c |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
@@ -1121,6 +1121,9 @@ static struct task_struct *copy_process(
 	p->blocked_on = NULL; /* not blocked yet */
 #endif
 
+	/* Perform scheduler related setup. Assign this task to a CPU. */
+	sched_fork(p, clone_flags);
+
 	if ((retval = security_task_alloc(p)))
 		goto bad_fork_cleanup_policy;
 	if ((retval = audit_alloc(p)))

@@ -1210,9 +1213,6 @@ static struct task_struct *copy_process(
 	INIT_LIST_HEAD(&p->ptrace_children);
 	INIT_LIST_HEAD(&p->ptrace_list);
 
-	/* Perform scheduler related setup. Assign this task to a CPU. */
-	sched_fork(p, clone_flags);
-
 	/* Now that the task is set up, run cgroup callbacks if
 	 * necessary. We need to run them before the task is visible
 	 * on the tasklist. */

@@ -1023,7 +1023,7 @@ static void task_new_fair(struct rq *rq,
 	place_entity(cfs_rq, se, 1);
 
 	if (sysctl_sched_child_runs_first && this_cpu == task_cpu(p) &&
-			curr->vruntime < se->vruntime) {
+			curr && curr->vruntime < se->vruntime) {
 		/*
 		 * Upon rescheduling, sched_class::put_prev_task() will place
 		 * 'current' within the tree based on its new key value.

| Quoting Srivatsa Vaddagiri ([email protected]):
| > On Fri, Nov 09, 2007 at 09:45:21AM +0100, Dmitry Adamushko wrote:
| > > Humm... the 'current' is not kept within the tree but
| > > current->se.on_rq is supposed to be '1' ,
| > > so the old code looks ok to me (at least for the 'leaf' elements).
| > 
| > You are damned right! Sorry my mistake with the previous analysis and
| > (as I now find out) testing :(
| > 
| > There are couple of problems discovered by Suka's test:
| > 
| > - The test requires the cgroup filesystem to be mounted with
| >   atleast the cpu and ns options (i.e both namespace and cpu 
| >   controllers are active in the same hierarchy). 
| > 
| > 	# mkdir /dev/cpuctl
| > 	# mount -t cgroup -ocpu,ns none cpuctl
| > 	(or simply)
| > 	# mount -t cgroup none cpuctl -> Will activate all controllers
| > 					 in same hierarchy.
| > 
| > - The test invokes clone() with CLONE_NEWNS set. This causes a a new child
| >   to be created, also a new group (do_fork->copy_namespaces->ns_cgroup_clone->
| >   cgroup_clone) and the child is attached to the new group (cgroup_clone->
| >   attach_task->sched_move_task). At this point in time, the child's scheduler 
| >   related fields are uninitialized (including its on_rq field, which it has
| >   inherited from parent). As a result sched_move_task thinks its on
| >   runqueue, when it isn't.
| > 
| >   As a solution to this problem, I moved sched_fork() call, which
| >   initializes scheduler related fields on a new task, before
| >   copy_namespaces(). I am not sure though whether moving up will
| >   cause other side-effects. Do you see any issue?
| > 
| > - The second problem exposed by this test is that task_new_fair()
| >   assumes that parent and child will be part of the same group (which 
| >   needn't be as this test shows). As a result, cfs_rq->curr can be NULL
| >   for the child.
| > 
| >   The solution is to test for curr pointer being NULL in
| >   task_new_fair().
| > 
| > 
| > With the patch below, I could run ns_exec() fine w/o a crash.
| > 
| > Suka, can you verify whether this patch fixes your problem?
| 
| Works on my machine.  Thanks!
| 
| > --
| > 
| > Fix copy_namespace() <-> sched_fork() dependency in do_fork, by moving
| > up sched_fork().
| > 
| > Also introduce a NULL pointer check for 'curr' in task_new_fair().
| > 
| > Signed-off-by : Srivatsa Vaddagiri <[email protected]>
| 
| Tested-by: Serge Hallyn <[email protected]>

: Crash with CONFIG_FAIR_CGROUP_SCHED=y

[email protected]

Srivatsa Vaddagiri

Dmitry Adamushko

Srivatsa Vaddagiri

Ingo Molnar

Dmitry Adamushko

Srivatsa Vaddagiri

Serge E. Hallyn

[email protected]