linux-kernel - Re: [RFC][PATCH] freezer,sched: Rewrite core freezer logic

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]

Message-ID: <YLi8Ttw3Xb3ynUW2@hirez.programming.kicks-ass.net>
Date:   Thu, 3 Jun 2021 13:26:06 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Will Deacon <will@...nel.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-arch@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Catalin Marinas <catalin.marinas@....com>,
        Marc Zyngier <maz@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Morten Rasmussen <morten.rasmussen@....com>,
        Qais Yousef <qais.yousef@....com>,
        Suren Baghdasaryan <surenb@...gle.com>,
        Quentin Perret <qperret@...gle.com>, Tejun Heo <tj@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        kernel-team@...roid.com, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [RFC][PATCH] freezer,sched: Rewrite core freezer logic

On Thu, Jun 03, 2021 at 11:58:56AM +0100, Will Deacon wrote:
> On Thu, Jun 03, 2021 at 12:35:22PM +0200, Peter Zijlstra wrote:
> > On Wed, Jun 02, 2021 at 01:54:53PM +0100, Will Deacon wrote:

> > > > @@ -116,20 +173,8 @@ bool freeze_task(struct task_struct *p)
> > > >  {
> > > >  	unsigned long flags;
> > > >  
> > > >  	spin_lock_irqsave(&freezer_lock, flags);
> > > > +	if (!freezing(p) || frozen(p) || __freeze_task(p)) {
> > > >  		spin_unlock_irqrestore(&freezer_lock, flags);
> > > >  		return false;
> > > >  	}
> > > 
> > > I've been trying to figure out how this serialises with ttwu(), given that
> > > frozen(p) will go and read p->state. I suppose it works out because only the
> > > freezer can wake up tasks from the FROZEN state, but it feels a bit brittle.
> > 
> > p->pi_lock; both ttwu() and __freeze_task() (which is essentially a
> > variant of set_special_state()) take ->pi_lock. I'll put in a comment.
> 
> The part I struggled with was freeze_task(), which doesn't take ->pi_lock
> yet calls frozen(p).

Ah, I can't read... I assumed you were asking about __freeze_task().

So frozen(p) checks for p->state == TASK_FROZEN (and complicated), which
is a stable state. Once you're frozen you stay frozen until thaw, which
is after freezing per construction.

The tricky bit is __freeze_task(), that does take pi_lock. It checks for
FREEZABLE and if set, changes to FROZEN. And this does in fact race with
ttwu() and relies on pi_lock to serialize.

A concurrent wakeup (from a non-frozen task) can try and wake the task
we're trying to freeze. If we win, ttwu will see FROZEN and ignore, if
ttwu() wins, we don't see FREEZABLE and do another round of freezing.

> > > > @@ -137,7 +182,7 @@ bool freeze_task(struct task_struct *p)
> > > >  	if (!(p->flags & PF_KTHREAD))
> > > >  		fake_signal_wake_up(p);
> > > >  	else
> > > > -		wake_up_state(p, TASK_INTERRUPTIBLE);
> > > > +		wake_up_state(p, TASK_INTERRUPTIBLE); // TASK_NORMAL ?!?
> > > >  
> > > >  	spin_unlock_irqrestore(&freezer_lock, flags);
> > > >  	return true;
> > > > @@ -148,8 +193,8 @@ void __thaw_task(struct task_struct *p)
> > > >  	unsigned long flags;
> > > >  
> > > >  	spin_lock_irqsave(&freezer_lock, flags);
> > > > -	if (frozen(p))
> > > > -		wake_up_process(p);
> > > > +	WARN_ON_ONCE(freezing(p));
> > > > +	wake_up_state(p, TASK_FROZEN | TASK_NORMAL);
> > > 
> > > Why do we need TASK_NORMAL here?
> > 
> > It's a left-over from hacking, but I left it in because anything
> > TASK_NORMAL should be able to deal with spuriuos wakeups, something
> > try_to_freeze() now also relies on.
> 
> I just worry that it might hide bugs if TASK_FROZEN is supposed to be
> sufficient, as it would imply that we have some unfrozen tasks kicking
> around. I dunno, maybe just a comment saying that everything _should_ be
> FROZEN at this point?

I'll take it out. It really shouldn't matter.