sched/autogroup: race if !sysctl_sched_autogroup_enabled ?

* sched/autogroup: race if !sysctl_sched_autogroup_enabled ?
@ 2016-11-09 16:59 Oleg Nesterov
  2016-11-09 17:50 ` Peter Zijlstra
  0 siblings, 1 reply; 7+ messages in thread
From: Oleg Nesterov @ 2016-11-09 16:59 UTC (permalink / raw)
  To: Ingo Molnar, Linus Torvalds, Mike Galbraith, Peter Zijlstra
  Cc: hartsjc, vbendel, vlovejoy, linux-kernel

I am trying to investigate a bug-report which looks as an autogroup bug,
and it seems I found the race which _might_ explain the problem. I'll
try to make the fix tomorrow but could you confirm I got it right and
answer the question below?

Let's look at task_wants_autogroup()

	/*
	 * We can only assume the task group can't go away on us if
	 * autogroup_move_group() can see us on ->thread_group list.
	 */
	if (p->flags & PF_EXITING)
		return false;

Firstly, I think that this PF_EXITING check is no longer needed.
sched_change_group() can be called by autogroup or cgroups code.
autogroup is obviously fine, cgroup-attach will never try to migrate
the exiting task (cgroup_threadgroup_rwsem helps), cpu_cgroup_fork()
is obviously fine too.

But!!! at the same time the comment is _correct_ even if very cryptic ;)

We need to ensure that autogroup/tg returned by autogroup_task_group()
can't go away if we race with autogroup_move_group(), and unless the
caller holds ->siglock we rely on fact that autogroup_move_group()
will a) see this task and b) do sched_move_task() which needs the same
same rq->lock.

However. autogroup_move_group() skips for_each_thread/sched_move_task
if sysctl_sched_autogroup_enabled == 0.

So. Doesn't this mean that cgroup migration to the root cgroup can race
with autogroup_move_group() and use the soon-to-be-freed autogroup->tg?

Or, even simpler, cgroup_post_fork()->cpu_cgroup_fork() can hit the
same race if CLONE_TRHEAD?

Or I am totally confused?

---------------------------------------------------------------------
Now the qustions. Can't we simplify autogroup_task_get() and avoid
lock_task_sighand() ?

	struct autogroup *autogroup_task_get(struct task_struct *p)
	{
		struct autogroup *ag

		rcu_read_lock();
		for (;;) {
			// it is freed by sched_free_group_rcu() path
			// and thus ->autogroup is rcu-safe too.
			ag = READ_ONCE(p->signal->autogroup);
			if (kref_get_unless_zero(&ag->kref))
				break;
		}
		rcu_read_unlock();

		return ag;
	}

although this is a bit off-topic. Another question is that I fail to
understand why sched_autogroup_create_attach() does autogroup_create()
and changes signal->autogroup even if !sysctl_sched_autogroup_enabled.

IOW, even ignoring the problem above, what is wrong with this patch?

	--- x/kernel/sched/auto_group.c
	+++ x/kernel/sched/auto_group.c
	@@ -152,8 +152,12 @@ out:
	 /* Allocates GFP_KERNEL, cannot be called under any spinlock */
	 void sched_autogroup_create_attach(struct task_struct *p)
	 {
	-	struct autogroup *ag = autogroup_create();
	+	struct autogroup *ag;
	+
	+	if (!sysctl_sched_autogroup_enabled)
	+		return;

	+	ag = autogroup_create();
		autogroup_move_group(p, ag);
		/* drop extra reference added by autogroup_create() */
		autogroup_kref_put(ag);

or even

	--- x/kernel/sched/auto_group.c
	+++ x/kernel/sched/auto_group.c
	@@ -64,9 +64,13 @@ static inline struct autogroup *autogrou

	 static inline struct autogroup *autogroup_create(void)
	 {
	-	struct autogroup *ag = kzalloc(sizeof(*ag), GFP_KERNEL);
	+	struct autogroup *ag;
		struct task_group *tg;

	+	if (!sysctl_sched_autogroup_enabled)
	+		goto xxx;
	+
	+	ag = kzalloc(sizeof(*ag), GFP_KERNEL);
		if (!ag)
			goto out_fail;

	@@ -103,7 +107,7 @@ out_fail:
			printk(KERN_WARNING "autogroup_create: %s failure.\n",
				ag ? "sched_create_group()" : "kmalloc()");
		}
	-
	+xxx:
		return autogroup_kref_get(&autogroup_default);
	 }

Oleg.

^ permalink raw reply	[flat|nested] 7+ messages in thread