Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 7 Nov 2019 16:09:17 +0000
From:   Qais Yousef <qais.yousef@arm.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Kirill Tkhai <ktkhai@virtuozzo.com>,
        Quentin Perret <qperret@google.com>,
        linux-kernel@vger.kernel.org, aaron.lwe@gmail.com,
        valentin.schneider@arm.com, mingo@kernel.org, pauld@redhat.com,
        jdesfossez@digitalocean.com, naravamudan@digitalocean.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        juri.lelli@redhat.com, rostedt@goodmis.org, bsegall@google.com,
        mgorman@suse.de, kernel-team@android.com, john.stultz@linaro.org
Subject: Re: NULL pointer dereference in pick_next_task_fair
Message-ID: <20191107160916.tf2hnho57ob6bm5n@e107158-lin.cambridge.arm.com>
References: <20191028174603.GA246917@google.com>
 <20191106120525.GX4131@hirez.programming.kicks-ass.net>
 <33643a5b-1b83-8605-2347-acd1aea04f93@virtuozzo.com>
 <20191106165437.GX4114@hirez.programming.kicks-ass.net>
 <20191106172737.GM5671@hirez.programming.kicks-ass.net>
 <831c2cd4-40a4-31b2-c0aa-b5f579e770d6@virtuozzo.com>
 <20191107132628.GZ4114@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20191107132628.GZ4114@hirez.programming.kicks-ass.net>
User-Agent: NeoMutt/20171215
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 11/07/19 14:26, Peter Zijlstra wrote:
> On Thu, Nov 07, 2019 at 11:36:50AM +0300, Kirill Tkhai wrote:
> > On 06.11.2019 20:27, Peter Zijlstra wrote:
> > > On Wed, Nov 06, 2019 at 05:54:37PM +0100, Peter Zijlstra wrote:
> > >> On Wed, Nov 06, 2019 at 06:51:40PM +0300, Kirill Tkhai wrote:
> > >>>> +#ifdef CONFIG_SMP
> > >>>> +	if (!rq->nr_running) {
> > >>>> +		/*
> > >>>> +		 * Make sure task_on_rq_curr() fails, such that we don't do
> > >>>> +		 * put_prev_task() + set_next_task() on this task again.
> > >>>> +		 */
> > >>>> +		prev->on_cpu = 2;
> > >>>>  		newidle_balance(rq, rf);
> > >>>
> > >>> Shouldn't we restore prev->on_cpu = 1 after newidle_balance()? Can't prev
> > >>> become pickable again after newidle_balance() releases rq->lock, and we
> > >>> take it again, so this on_cpu == 2 never will be cleared?
> > >>
> > >> Indeed so.
> > > 
> > > Oh wait, the way it was written this is not possible. Because
> > > rq->nr_running == 0 and prev->on_cpu > 0 it means the current task is
> > > going to sleep and cannot be woken back up.
> > 
> > I mostly mean throttling. AFAIR, tasks of throttled rt_rq are not accounted
> > in rq->nr_running. But it seems rt_rq may become unthrottled again after
> > newidle_balance() unlocks rq lock, and prev will become pickable again.
> 
> Urgh... throttling.
> 
> Bah.. I had just named the "->on_cpu = 2" thing leave_task(), to match
> prepare_task() and finish_task(), but when we have to flip it back to 1
> that doesn't really work.
> 
> Also, I'm still flip-flopping on where to place it. Yesterday I
> suggested placing it before put_prev_task(), but then I went to write a
> comment, and either way around put_prev_task() needs to be very careful.
> 
> So I went back to placing it after and putting lots of comments on.
> 
> How does the below look?

This looks good to me. But it makes me wonder, if there's no penalty to adding
the leave_task() before put_prev_task(), and if it results on relaxing the
requirement of 'no permanent state change before releasing the rq lock',
shouldn't we just do it?

Anyways. I'll pick this version and spin it through my reproducer.

Thanks for fixing this!

--
Qais Yousef