linux-kernel - Re: [PATCH 2/2] irq_work: Fix racy IRQ_WORK

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:	Wed, 31 Oct 2012 08:59:50 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	anish kumar <anish198519851985@...il.com>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Paul McKenney <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Paul Gortmaker <paul.gortmaker@...driver.com>
Subject: Re: [PATCH 2/2] irq_work: Fix racy IRQ_WORK_BUSY flag setting

On Wed, 2012-10-31 at 20:04 +0900, anish kumar wrote:

> > /*
> >  * Claim the entry so that no one else will poke at it.
> >  */
> > static bool irq_work_claim(struct irq_work *work)
> > {
> > 	unsigned long flags, nflags;
> > 
> > 	for (;;) {
> > 		flags = work->flags;
> > 		if (flags & IRQ_WORK_PENDING)
> > 			return false;
> > 		nflags = flags | IRQ_WORK_FLAGS;
> nflags = 1 | 3
> nflags = 2 | 3
> In both cases the result would be same.If I am right then wouldn't this
> operation be redundant?

Right. Actually we could change the new loop to:

	for (;;) {
		oflags = cmpxchg(&work->flags, flags, IRQ_WORK_FLAGS);
		if (oflags == flags)
			break;
		if (oflags & IRQ_WORK_PENDING)
			return false;
		flags = oflags;
		cpu_relax();
	}


> > 		if (cmpxchg(&work->flags, flags, nflags) == flags)
> > 			break;
> > 		cpu_relax();
> > 	}
> > 
> > 	return true;
> > }
> > 


> > 
> > This now does:
> > 
> > 	CPU 1					CPU 2
> > 	-----					-----
> > 	(flags = 0)
> > 	cmpxchg(flags, 0, IRQ_WORK_FLAGS)
> > 	(flags = 3)
> > 	[...]
> > 	xchg(&flags, IRQ_WORK_BUSY)
> > 	(flags = 2)
> > 	func()
> > 						oflags = cmpxchg(&flags, flags, nflags);
> > 						(sees flags = 2)
> > 						if (flags & IRQ_WORK_PENDING)
> > 							(not true)
> > 						(loop)						
> > 	cmpxchg(flags, 2, 0);
> > 	(flags = 2)
> > 						flags = 3
> > 
> > 
> > 
> > With both patch 1 and 2 you fixed the bug.
> This is the best explanation anyone can put forward for this problem.

Frederic,

Would you like to add my explanation to your change log? You can add the
entire thing, which I think would explain a lot to people.

Thanks,

-- Steve


--
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/