From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932898AbcESRy4 (ORCPT ); Thu, 19 May 2016 13:54:56 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:38458 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932216AbcESRyz (ORCPT ); Thu, 19 May 2016 13:54:55 -0400 Date: Thu, 19 May 2016 19:54:31 +0200 From: Peter Zijlstra To: Chris Metcalf Cc: Gilad Ben Yossef , Steven Rostedt , Ingo Molnar , Andrew Morton , Rik van Riel , Tejun Heo , Frederic Weisbecker , Thomas Gleixner , "Paul E. McKenney" , Christoph Lameter , Viresh Kumar , Catalin Marinas , Will Deacon , Andy Lutomirski , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v12 07/13] task_isolation: add debug boot flag Message-ID: <20160519175431.GY3193@twins.programming.kicks-ass.net> References: <1459877922-15512-1-git-send-email-cmetcalf@mellanox.com> <1459877922-15512-8-git-send-email-cmetcalf@mellanox.com> <20160518135645.GJ3193@twins.programming.kicks-ass.net> <684587d7-3653-7570-215f-37d3e9e786bc@mellanox.com> <20160518170647.GL3193@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, May 19, 2016 at 10:42:39AM -0400, Chris Metcalf wrote: > >>>>+ rcu_read_lock(); > >>>>+ p = cpu_curr(cpu); Here @cpu can schedule, hit TASK_DEAD and do put_task_struct() and kfree() the task. > >>>>+ get_task_struct(p); And here we then do a use-after-free. > >>>>+ rcu_read_unlock(); > >>>>+ task_isolation_debug_task(cpu, p); > >>>>+ put_task_struct(p); > 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? Yep, as per the above. > 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. Nope... the only way to avoid this from happening is taking @cpu's rq->lock to prevent the remote CPU from scheduling. > 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(). The whole pid data structure is RCU managed, rq->curr is not. > Anyway, whatever more clarity you can offer me, or suggestions for > APIs to use are welcome. The API proposed in the discussion below.. > >See also the discussion around: > > > >lkml.kernel.org/r/20160518170218.GY3192@twins.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. Nope, that's just as broken. So the 'simple' thing is: struct rq *rq = cpu_rq(cpu); struct task_struct *task; raw_spin_lock_irq(&rq->lock); task = rq->curr; get_task_struct(task); raw_spin_unlock_irq(&rq->lock); Because by holding rq->lock, the remote CPU cannot schedule and the current task _must_ still be valid. And note; the above can result in a task which already has PF_EXITING set. The complex thing is described in the linked thread and will likely make your head hurt.