linux-kernel - Re: [ANNOUNCE] 3.12.5-rt6

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: <20131217063156.6ac3bfed@gandalf.local.home>
Date:	Tue, 17 Dec 2013 06:31:56 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Mike Galbraith <bitbucket@...ine.de>
Cc:	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	linux-rt-users <linux-rt-users@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	John Kacur <jkacur@...hat.com>
Subject: Re: [ANNOUNCE] 3.12.5-rt6

On Tue, 17 Dec 2013 08:16:31 +0100
Mike Galbraith <bitbucket@...ine.de> wrote:

> Hi Sebastian,
> 
> Looks like there's a booboo here:
> 
> On Mon, 2013-12-16 at 10:14 +0100, Sebastian Andrzej Siewior wrote:
 
> "ptrace: fix ptrace vs tasklist_lock race" added.. 
> 
> @@ -1068,8 +1082,11 @@ unsigned long wait_task_inactive(struct
>  		 * is actually now running somewhere else!
>  		 */
>  		while (task_running(rq, p)) {
> -			if (match_state && unlikely(p->state != match_state))
> +			if (match_state) {
> +				if (!unlikely(check_task_state(p, match_state)))
> +					return 0;
>  				return 0;
> +			}

Ouch! 
>  			cpu_relax();
>  		}
>  
> ..which is how it stays with the whole series applied.
> 
> The patch contains hunk 2 from
> 
>    "sched/rt: Fix wait_task_interactive() to test rt_spin_lock state",
> 
> which went away in -rt6, so it seems the busted hunk should be as below
> if the two are to be merged.
> 
> @@ -1068,8 +1082,10 @@ unsigned long wait_task_inactive(struct
>  		 * is actually now running somewhere else!
>  		 */
>  		while (task_running(rq, p)) {
> -			if (match_state && unlikely(p->state != match_state))
> +			if (match_state && unlikely(p->state != match_state)
> +			    && unlikely(p->saved_state != match_state))
>  				return 0;
> +			}

Yeah, it should just be:

		if (match_state && check_task_state(p, match_state))
			return 0;

Also, looking at check_task_state():

+static bool check_task_state(struct task_struct *p, long match_state)
+{
+       bool match = false;
+
+       raw_spin_lock_irq(&p->pi_lock);
+       if (p->state == match_state)
+               match = true;
+       else if (p->saved_state == match_state)
+               match = true;

Why the if () else if()? and not just:

	if (p->state == match_state || p->save_state == match_state)
		match = true;
?

The else if makes me think there's something missing.

-- Steve

+       raw_spin_unlock_irq(&p->pi_lock);
+
+       return match;
+}



>  			cpu_relax();
>  		}
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/