linux-kernel - Re: [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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210614165422.GC13677@redhat.com>
Date:   Mon, 14 Jun 2021 18:54:23 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     rjw@...ysocki.net, mingo@...nel.org, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, mgorman@...e.de,
        Will Deacon <will@...nel.org>, Tejun Heo <tj@...nel.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH] freezer,sched: Rewrite core freezer logic

On 06/14, Peter Zijlstra wrote:
>
> On Mon, Jun 14, 2021 at 05:42:47PM +0200, Oleg Nesterov wrote:
> >
> > > +	/*
> > > +	 * If stuck in TRACED, and the ptracer is FROZEN, we're frozen too.
> > > +	 */
> > > +	if (task_is_traced(p))
> > > +		return frozen(rcu_dereference(p->parent));
> >
> > This looks racy, p->parent can resume this task and then enter
> > __refrigerator().
>
> But this is about the child, we won't report it frozen, unless the
> parent is also frozen. If the parent is frozen, it cannot resume the
> task.

Yes, but...

> The other way around, if the parent resumes the task and then gets
> frozen,

Yes ...

> then we'll wait until the task gets frozen.

how/where will we wait until the tracee gets frozen ?

Again, suppose that p->parent resumes p and gets frozen after the
task_is_traced(p) check and before the frozen(p->parent) check.

Then try_to_freeze_tasks() can succeed with todo == 0 and miss the
running "p" ?

> > > +	 * If stuck in STOPPED and the parent is FROZEN, we're frozen too.
> > > +	 */
> > > +	if (task_is_stopped(p))
> > > +		return frozen(rcu_dereference(p->real_parent));
> >
> > (you could use ->parent in this case too and unify this check with the
> > "traced" case above)
>
> Are you sure? The way I read the code ptrace_attach() will change
> ->parent, but STOPPED is controlled by the jobctl.

Yes, sorry I was not clear. let me add more details.

task_is_stopped() is only possible if task is not ptraced, see the
"if (!current->ptrace)" check before set_special_state(TASK_STOPPED)
in do_signal_stop(). And if the task is not traced, then
task->parent == task->real_parent.

> > I don't understand. How this connects to ->parent or ->real_parent?
> > SIGCONT can come from anywhere and wake this stopped task up?
>
> Could be me who's not understanding, I thought only the real parent
> could do that.

No, any task can do this, as long as check_kill_permission() succeeds.
Even the kernel can send SIGCONT, say, you can use F_SETSIG(SIGCONT).

> > I guess you do this to avoid freezable_schedule() in ptrace/signal_stop,
> > and we can't use TASK_STOPPED|TASK_FREEZABLE, it should not run after
> > thaw()... But see above, we can't rely on __frozen(parent).
>
> I do this because freezing puts a task in TASK_FROZEN, and that cannot
> preserve TAKS_STOPPED or TASK_TRACED without being subject to wakups

Yes, yes, this is what I tried to say.

Oleg.