Fix race between attach_task and cpuset_exit

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +4 -6

 	cpumask_t cpus;
 	nodemask_t from, to;
 	struct mm_struct *mm;

@@ -1237,6 +1238,8 @@ static int attach_task(struct cpuset *cs
 	}
 	atomic_inc(&cs->count);
 	rcu_assign_pointer(tsk->cpuset, cs);
+	if (atomic_dec_and_test(&oldcs->count))
+		oldcs_tobe_released = oldcs;
 	task_unlock(tsk);
 
 	guarantee_online_cpus(cs, &cpus);

@@ -1257,8 +1260,8 @@ static int attach_task(struct cpuset *cs
 
 	put_task_struct(tsk);
 	synchronize_rcu();
-	if (atomic_dec_and_test(&oldcs->count))
-		check_for_release(oldcs, ppathbuf);
+	if (oldcs_tobe_released)
+		check_for_release(oldcs_tobe_released, ppathbuf);
 	return 0;
 }
 

@@ -2200,10 +2203,6 @@ void cpuset_fork(struct task_struct *chi
  * it is holding that mutex while calling check_for_release(),
  * which calls kmalloc(), so can't be called holding callback_mutex().
  *
- * We don't need to task_lock() this reference to tsk->cpuset,
- * because tsk is already marked PF_EXITING, so attach_task() won't
- * mess with it, or task is a failed fork, never visible to attach_task.
- *
  * the_top_cpuset_hack:
  *
  *    Set the exiting tasks cpuset to the root cpuset (top_cpuset).

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +0 -2

 			check_for_release(cs, &pathbuf);
 		mutex_unlock(&manage_mutex);
 		cpuset_release_agent(pathbuf);
-	} else {
-		atomic_dec(&cs->count);
 	}
 }
 

How about using a local variable like ref_count and using

ref_count = atomic_dec_and_test(&cs->count); This will avoid the two atomic operations, atomic_dec() and atomic_read() below.

kernel/cpuset.c static int attach_task(struct cpuset *cs · line 1185

Quoted 1 line

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +1 -0

+	struct cpuset *oldcs_tobe_released = NULL;

How about oldcs_to_be_released?

kernel/cpuset.c void cpuset_exit(struct task_struct *tsk · line 2247

Quoted 8 lines

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +2 -0

@@ -2242,19 +2241,20 @@ void cpuset_exit(struct task_struct *tsk
 {
 	struct cpuset *cs;
 
+	task_lock(tsk);
 	cs = tsk->cpuset;
 	tsk->cpuset = &top_cpuset;	/* the_top_cpuset_hack - see above */
+	atomic_dec(&cs->count);

How about using a local variable like ref_count and using

ref_count = atomic_dec_and_test(&cs->count); This will avoid the two atomic operations, atomic_dec() and atomic_read() below.

Well, someone may have attached to this cpuset while we were waiting on the mutex_lock(). So we need to do a atomic_read again to ensure it is still unused.

Quoted 3 lines · vatsa

Well, someone may have attached to this cpuset while we were waiting on the mutex_lock(). So we need to do a atomic_read again to ensure it is still unused.

I don't see how this could happen. If we hold the task lock that now (thanks to your good work) guards this pointer, and if we decrement to zero the reference count on the cpuset to which it points and then -overwrite- this last remaining visible pointer to that cpuset with a pointer to a different cpuset, then aren't we guaranteed to be holding the last remaining reference to the old cpuset in our local variable, making it impossible for anyone else to attach to it in any way?

Yes, but the cpuset is not made invisible to userspace (in filesystem) yet. So as cpuset_exit() discovers that cpuset B has zero refcount now and blocks on mutex_lock(&manage_mutex) [ to do a check_for_release later ], someone could have done a attach_task to that cpuset.

Well, someone may have attached to this cpuset while we were waiting on the mutex_lock(). So we need to do a atomic_read again to ensure it is still unused

If we hold the task lock that now
(thanks to your good work) guards this pointer, and if we decrement to zero the reference count on the cpuset to which it points

Is there perhaps another race here? Could it happen that:
1) the atomic_dec() lowers the count to say one (any value > zero)
2) after we drop the task lock, some other task or tasks decrement the count to zero
3) we catch that zero when we atomic_read the count, and issue a spurious check_for_release().

Is there perhaps another race here?

Quoted 1 line · Paul Jackson

Is there perhaps another race here?

Yes, we have!

Modified patch below. Compile/boot tested on a x86_64 box.

Currently cpuset_exit() changes the exiting task's ->cpuset pointer w/o taking task_lock(). This can lead to ugly races between attach_task and cpuset_exit. Details of the races are described at
http://lkml.org/lkml/2007/3/24/132.

Patch below closes those races. It is against 2.6.21-rc4 and has undergone a simple compile/boot test on a x86_64 box.

Signed-off-by Srivatsa Vaddagiri <[email protected]>

---

diff -puN kernel/cpuset.c~cpuset_race_fix kernel/cpuset.c

kernel/cpuset.c 2007-03-26 16:48:24.000000000 +0530 renamed from kernel/cpuset.c~cpuset_race_fix 2007-03-25 21:08:27.000000000 +0530 +3 -0

@@ -1182,6 +1182,7 @@ static int attach_task(struct cpuset *cs
        pid_t pid;
        struct task_struct *tsk;
        struct cpuset *oldcs;
+       struct cpuset *oldcs_to_be_released = NULL;
        cpumask_t cpus;
        nodemask_t from, to;
        struct mm_struct *mm;

@@ -1237,6 +1238,8 @@ static int attach_task(struct cpuset *cs
        }
        atomic_inc(&cs->count);
        rcu_assign_pointer(tsk->cpuset, cs);
+       if (atomic_dec_and_test(&oldcs->count))
+               oldcs_to_be_released = oldcs;
        task_unlock(tsk);
 
        guarantee_online_cpus(cs, &cpus);

kernel/cpuset.c 2007-03-26 16:48:24.000000000 +0530 renamed from kernel/cpuset.c~cpuset_race_fix 2007-03-25 21:08:27.000000000 +0530 +0 -4

@@ -2200,10 +2203,6 @@ void cpuset_fork(struct task_struct *chi
  * it is holding that mutex while calling check_for_release(),
  * which calls kmalloc(), so can't be called holding callback_mutex().
  *
- * We don't need to task_lock() this reference to tsk->cpuset,
- * because tsk is already marked PF_EXITING, so attach_task() won't
- * mess with it, or task is a failed fork, never visible to attach_task.
- *
  * the_top_cpuset_hack:
  *
  *    Set the exiting tasks cpuset to the root cpuset (top_cpuset).

@@ -2241,20 +2240,23 @@ void cpuset_fork(struct task_struct *chi
 void cpuset_exit(struct task_struct *tsk)
 {

kernel/cpuset.c static int attach_task(struct cpuset *cs · line 1266

Quoted 10 lines

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +2 -2

@@ -1257,8 +1260,8 @@ static int attach_task(struct cpuset *cs
 
       put_task_struct(tsk);
       synchronize_rcu();
-       if (atomic_dec_and_test(&oldcs->count))
-               check_for_release(oldcs, ppathbuf);
+       if (oldcs_to_be_released)
+               check_for_release(oldcs_to_be_released, ppathbuf);
       return 0;
 }

Is this part of the patch necessary? If we're adding a task_lock() in cpuset_exit(), then the problem that Vatsa described (both cpuset_attach_task() and cpuset_exit() decrementing the same cpuset count, and cpuset_attach_task() incrementing the count on a cpuset that the task doesn't eventually end up in) go away, since only one thread will retrieve the old value of the task's cpuset in order to decrement its count.

kernel/cpuset.c void cpuset_fork(struct task_struct *chi · line 2250

Quoted 13 lines

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +0 -0

 void cpuset_exit(struct task_struct *tsk)

{
       struct cpuset *cs;

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +5 -0

+       struct cpuset *oldcs_to_be_released = NULL;
 
+       task_lock(tsk);
       cs = tsk->cpuset;
       tsk->cpuset = &top_cpuset;      /* the_top_cpuset_hack - see above 
       */
+       if (atomic_dec_and_test(&cs->count))
+               oldcs_to_be_released = cs;
+       task_unlock(tsk);

I think this is still racy - at this point we're holding a reference on a cpuset that could have a zero count,

and we don't hold
manage_mutex or callback_mutex. So a concurrent rmdir could zap the directory and free the cpuset.

Shouldn't we just put a task_lock()/task_unlock() around these lines and leave everything else as-is?

	task_lock(tsk);
	cs = tsk->cpuset;
	tsk->cpuset = &top_cpuset;	/* the_top_cpuset_hack - see above */
	task_unlock(tsk)

Quoted 18 lines · Paul Menage

kernel/cpuset.c static int attach_task(struct cpuset *cs · line 1266

Quoted 10 lines

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +2 -2

@@ -1257,8 +1260,8 @@ static int attach_task(struct cpuset *cs
 
       put_task_struct(tsk);
       synchronize_rcu();
-       if (atomic_dec_and_test(&oldcs->count))
-               check_for_release(oldcs, ppathbuf);
+       if (oldcs_to_be_released)
+               check_for_release(oldcs_to_be_released, ppathbuf);
       return 0;
 }

Is this part of the patch necessary? If we're adding a task_lock() in cpuset_exit(), then the problem that Vatsa described (both cpuset_attach_task() and cpuset_exit() decrementing the same cpuset count, and cpuset_attach_task() incrementing the count on a cpuset that the task doesn't eventually end up in) go away, since only one thread will retrieve the old value of the task's cpuset in order to decrement its count.

You *have* to drop/inc the refcount inside the task_lock, otherwise it is racy.

        task_lock(T1);
        old_cs = T1->cputset (C1)
        atomic_inc(&C2->count);
        T1->cputset = C2;
        task_unlock();

        ...

        synchronize_rcu();

        if (atomic_dec_and_test(&C1->count))
                check_for_release(..)

is incorrect. For ex: T1's refcount on C1 may have already been dropped by now in cpuset_exit() and dropping the refcount again can lead to negative refcounts.

                .

Quoted 14 lines

Quoted 13 lines

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +0 -0

 void cpuset_exit(struct task_struct *tsk)

{
       struct cpuset *cs;

kernel/cpuset.c renamed from kernel/cpuset.c~cpuset_race_fix +5 -0

+       struct cpuset *oldcs_to_be_released = NULL;
 
+       task_lock(tsk);
       cs = tsk->cpuset;
       tsk->cpuset = &top_cpuset;      /* the_top_cpuset_hack - see above
       */
+       if (atomic_dec_and_test(&cs->count))
+               oldcs_to_be_released = cs;
+       task_unlock(tsk);

Quoted 2 lines

I think this is still racy - at this point we're holding a reference on a cpuset that could have a zero count,

How's that possible? That you have a zero-refcount cpuset with non empty tasks in it?

Quoted 4 lines

Shouldn't we just put a task_lock()/task_unlock() around these lines and leave everything else as-is?

Quoted 4 lines

      task_lock(tsk);
      cs = tsk->cpuset;
      tsk->cpuset = &top_cpuset;      /* the_top_cpuset_hack - see above */
      task_unlock(tsk)

If we don't drop refcount inside task_lock() it makes it racy with attach_task(). 'cs' derived above may not be the right cpuset to drop refcount on later in cpuset_exit.

I don't see how that could happen. Assuming we add the task_lock()/task_unlock() in cpuset_exit(), then only one of the two threads (either cpuset_exit() or attach_task() ) can copy C1 from T1->cpuset and replace it with something new, and hence only one of them can drop the refcount.

Quoted 2 lines

How's that possible? That you have a zero-refcount cpuset with non empty tasks in it?

If this is the last task in cs, then cs->count will be 1. We remove this task from cs, and decrement its count to 0. Then another cpu does cpuset_rmdir(), takes manage_mutex, sees that the count is 0, cleans up the cpuset, drops the dentry, and the cpuset gets freed. Then we get to run again, and we dereference an invalid cpuset.

Hmm yes ..I am surprised we arent doing a synhronize_rcu in cpuset_rmdir() before dropping the dentry. Did you want to send a patch for that?

Shouldn't we just put a task_lock()/task_unlock() around these lines and leave everything else as-is?

	task_lock(tsk);
	cs = tsk->cpuset;
	tsk->cpuset = &top_cpuset;	/* the_top_cpuset_hack - see above */
	task_unlock(tsk)