linux-kernel - Re: [tip:sched/urgent] cpuset: Fix cpuset_cpus_allowed_fallback(), don't update tsk->rt.nr_cpus

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4E006A70.2070205@jp.fujitsu.com>
Date:	Tue, 21 Jun 2011 18:54:56 +0900
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	a.p.zijlstra@...llo.nl
CC:	mingo@...hat.com, hpa@...or.com, linux-kernel@...r.kernel.org,
	tglx@...utronix.de, oleg@...hat.com, mingo@...e.hu,
	linux-tip-commits@...r.kernel.org
Subject: Re: [tip:sched/urgent] cpuset: Fix cpuset_cpus_allowed_fallback(),
 don't update tsk->rt.nr_cpus_allowed

(2011/06/20 19:20), Peter Zijlstra wrote:
> On Sat, 2011-05-28 at 16:35 +0000, tip-bot for KOSAKI Motohiro wrote:
>> +++ b/kernel/kthread.c
>> @@ -202,8 +202,8 @@ void kthread_bind(struct task_struct *p, unsigned int cpu)
>>                 return;
>>         }
>>  
>> -       p->cpus_allowed = cpumask_of_cpu(cpu);
>> -       p->rt.nr_cpus_allowed = 1;
>> +       /* It's safe because the task is inactive. */
>> +       do_set_cpus_allowed(p, cpumask_of(cpu));
>>         p->flags |= PF_THREAD_BOUND;
>>  }
> 
> 
> I just happened  to be staring at this stuff again, and I'm wondering
> how and why this is correct. After kthread_create() the thread exists
> and is exposed in the pid-hash, therefore userspace can come and do
> sys_sched_setaffinity() on it, and since we're not holding any locks and
> set PF_THREAD_BOUND _after_ setting cpus_allowed, things can end up
> funny.
> 
> Hmm?

Can't we take just either rq lock or pi_lock? Layer violation?



>From 1c0874b9157f47e22d0f6499612f0f78b830f018 Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Date: Tue, 21 Jun 2011 18:15:19 +0900
Subject: [PATCH] kthread: fix kthread_bind() race

Peter Zijlstra reported kthread_bind() has a race. It doesn't hold
any locks and set PF_THREAD_BOUND _after_ setting cpus_allowed. So,
following race can be happen.

  CPU0                           CPU1
  ----------------------------------------------------
  do_set_cpus_allowed()
				sys_sched_setaffinity()
  p->flags |= PF_THREAD_BOUND;

The solution is to take either rq lock or pi_lock. They prevent
a race because set_cpus_allowed_ptr() take both locks. This patch
choose to use latter way.

Reported-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
---
 kernel/kthread.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 4ba7ccc..92e3083 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -196,15 +196,20 @@ EXPORT_SYMBOL(kthread_create_on_node);
  */
 void kthread_bind(struct task_struct *p, unsigned int cpu)
 {
+	unsigned long flags;
+
 	/* Must have done schedule() in kthread() before we set_task_cpu */
 	if (!wait_task_inactive(p, TASK_UNINTERRUPTIBLE)) {
 		WARN_ON(1);
 		return;
 	}

+	/* protect from a race against set_cpus_allowed_ptr() */
+	raw_spin_lock_irqsave(&p->pi_lock, flags);
 	/* It's safe because the task is inactive. */
 	do_set_cpus_allowed(p, cpumask_of(cpu));
 	p->flags |= PF_THREAD_BOUND;
+	raw_spin_unlock_irqrestore(&p->pi_lock, flags);
 }
 EXPORT_SYMBOL(kthread_bind);

-- 
1.7.3.1




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/