linux-kernel - Re: NULL pointer dereference in pick_next_task

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] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Nov 2019 16:09:17 +0000
From:   Qais Yousef <qais.yousef@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Kirill Tkhai <ktkhai@...tuozzo.com>,
        Quentin Perret <qperret@...gle.com>,
        linux-kernel@...r.kernel.org, aaron.lwe@...il.com,
        valentin.schneider@....com, mingo@...nel.org, pauld@...hat.com,
        jdesfossez@...italocean.com, naravamudan@...italocean.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        juri.lelli@...hat.com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, kernel-team@...roid.com, john.stultz@...aro.org
Subject: Re: NULL pointer dereference in pick_next_task_fair

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