linux-kernel - Re: [PATCH v2 5/5] 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]

Date:   Thu, 21 Apr 2022 22:07:21 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     rjw@...ysocki.net, oleg@...hat.com, mingo@...nel.org,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, mgorman@...e.de, bigeasy@...utronix.de,
        Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org,
        tj@...nel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 5/5] freezer,sched: Rewrite core freezer logic

On Thu, Apr 21, 2022 at 09:55:51PM +0200, Peter Zijlstra wrote:
> On Thu, Apr 21, 2022 at 12:26:44PM -0500, Eric W. Biederman wrote:
> > Peter Zijlstra <peterz@...radead.org> writes:
> > 
> > > --- a/kernel/ptrace.c
> > > +++ b/kernel/ptrace.c
> > > @@ -288,7 +288,7 @@ static int ptrace_check_attach(struct ta
> > >  	}
> > >  	__set_current_state(TASK_RUNNING);
> > >  
> > > -	if (!wait_task_inactive(child, TASK_TRACED) ||
> > > +	if (!wait_task_inactive(child, TASK_TRACED|TASK_FREEZABLE) ||
> > >  	    !ptrace_freeze_traced(child))
> > >  		return -ESRCH;
> > 
> > Do we mind that this is going to fail if the child is frozen
> > during ptrace_check_attach?
> 
> Why should this fail? wait_task_inactive() will in fact succeed if it is
> frozen due to the added TASK_FREEZABLE and some wait_task_inactive()
> changes elsewhere in this patch.

These:

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3260,6 +3260,19 @@ int migrate_swap(struct task_struct *cur
 }
 #endif /* CONFIG_NUMA_BALANCING */
 
+static inline bool __wti_match(struct task_struct *p, unsigned int match_state)
+{
+	unsigned int state = READ_ONCE(p->__state);
+
+	if ((match_state & TASK_FREEZABLE) && state == TASK_FROZEN)
+		return true;
+
+	if (state == (match_state & ~TASK_FREEZABLE))
+		return true;
+
+	return false;
+}
+
 /*
  * wait_task_inactive - wait for a thread to unschedule.
  *
@@ -3304,7 +3317,7 @@ unsigned long wait_task_inactive(struct
 		 * is actually now running somewhere else!
 		 */
 		while (task_running(rq, p)) {
-			if (match_state && unlikely(READ_ONCE(p->__state) != match_state))
+			if (match_state && !__wti_match(p, match_state))
 				return 0;
 			cpu_relax();
 		}
@@ -3319,7 +3332,7 @@ unsigned long wait_task_inactive(struct
 		running = task_running(rq, p);
 		queued = task_on_rq_queued(p);
 		ncsw = 0;
-		if (!match_state || READ_ONCE(p->__state) == match_state)
+		if (!match_state || __wti_match(p, match_state))
 			ncsw = p->nvcsw | LONG_MIN; /* sets MSB */
 		task_rq_unlock(rq, p, &rf);
 


> And I don't see why ptrace_freeze_traced() should fail. It'll warn
> though, I should extend/remove that WARN_ON_ONCE() looking at __state,
> but it should work.

And that looks like (after removal of the one WARN):

static bool ptrace_freeze_traced(struct task_struct *task)
{
	unsigned long flags;
	bool ret = false;

	/* Lockless, nobody but us can set this flag */
	if (task->jobctl & JOBCTL_LISTENING)
		return ret;

	if (!lock_task_sighand(task, &flags))
		return ret;

	if (task_is_traced(task) &&
	    !looks_like_a_spurious_pid(task) &&
	    !__fatal_signal_pending(task)) {
		WARN_ON_ONCE(task->jobctl & JOBCTL_DELAY_WAKEKILL);
		task->jobctl |= JOBCTL_DELAY_WAKEKILL;
		ret = true;
	}
	unlock_task_sighand(task, &flags);

	return ret;
}

And nothing there cares about ->__state.