linux-kernel - Re: [PATCH v12 07/13] task_isolation: add debug boot flag

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: <ff42f417-655f-199a-4200-d4a95f166cf8@mellanox.com>
Date:	Thu, 19 May 2016 11:12:24 -0400
From:	Chris Metcalf <cmetcalf@...lanox.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Gilad Ben Yossef <giladb@...hip.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>, Tejun Heo <tj@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Christoph Lameter <cl@...ux.com>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Andy Lutomirski <luto@...capital.net>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v12 07/13] task_isolation: add debug boot flag

(Resending in text/plain.  I just screwed around with my Thunderbird
config some more in hopes of getting it to pay attention to all the
settings that say "use plain text for LKML", but, we'll see.)

On 5/18/2016 1:06 PM, Peter Zijlstra wrote:
> On Wed, May 18, 2016 at 12:35:19PM -0400, Chris Metcalf wrote:
>> On 5/18/2016 9:56 AM, Peter Zijlstra wrote:
>>> On Tue, Apr 05, 2016 at 01:38:36PM -0400, Chris Metcalf wrote:
>>>> +#ifdef CONFIG_TASK_ISOLATION
>>>> +void task_isolation_debug(int cpu)
>>>> +{
>>>> +	struct task_struct *p;
>>>> +
>>>> +	if (!task_isolation_possible(cpu))
>>>> +		return;
>>>> +
>>>> +	rcu_read_lock();
>>>> +	p = cpu_curr(cpu);
>>>> +	get_task_struct(p);
>>>> +	rcu_read_unlock();
>>>> +	task_isolation_debug_task(cpu, p);
>>>> +	put_task_struct(p);
>>> This is still broken...
>> I don't know how or why, though. :-)  Can you give me a better idiom?
>> This looks to my eye just like how it's done for something like
>> sched_setaffinity() by one task on another task, and I would have
>> assumed the risks there of the other task evaporating part way
>> through would be the same as the risks here.
> Because rcu_read_lock() does not stop the task pointed to by
> cpu_curr(cpu) from disappearing on you entirely.

So clearly once we have a task struct with an incremented usage count,
we are golden: the task isolation code only touches immediate fields
of task_struct, which are guaranteed to stick around until we
put_task_struct(), and the other path is into send_sig_info(), which
is already robust to the task struct being exited (the ->sighand
becomes NULL and we bail out in __lock_task_sighand, otherwise we're
holding sighand->siglock until we deliver the signal).

So, I think what you're saying is that there is a race between when we
read per_cpu(runqueues, cpu).curr, and when we increment the
p->usage value in the task, and that the RCU read lock doesn't help
with that?  My impression was that by being the ".curr" task, we are
guaranteed that it hasn't gone through do_exit() yet, and thus we
benefit from an RCU guarantee around being able to validly dereference
the pointer, i.e. it hasn't yet been freed and so dereferencing is safe.

I don't see how grabbing the ->curr from the runqueue is any more
fragile from an RCU perspective than grabbing the task from the pid in
kill_pid_info().  And in fact, that code doesn't even bump
task->usage, as far as I can see, just relying on getting the
sighand->siglock.

Anyway, whatever more clarity you can offer me, or suggestions for
APIs to use are welcome.

> See also the discussion around:
>
> lkml.kernel.org/r/20160518170218.GY3192@...ns.programming.kicks-ass.net

This makes me wonder if I should use rcu_dereference(&cpu_curr(p))
just for clarity, though I think it's just as correct either way.

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com