linux-kernel - Re: Preventing IPI sending races in arch code

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: <1385409103.9218.15.camel@pasglop>
Date:	Tue, 26 Nov 2013 06:51:43 +1100
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Vineet Gupta <Vineet.Gupta1@...opsys.com>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	Gilad Ben-Yossef <gilad@...yossef.com>,
	Noam Camus <noamc@...hip.com>,
	David Daney <david.daney@...ium.com>,
	James Hogan <james.hogan@...tec.com>,
	thomas Gleixner <tglx@...utronix.de>,
	lkml <linux-kernel@...r.kernel.org>,
	Richard Kuo <rkuo@...eaurora.org>
Subject: Re: Preventing IPI sending races in arch code

On Mon, 2013-11-25 at 13:35 +0000, Vineet Gupta wrote:

> Before reading ur email I was coding something like below:
> 
> void arch_send_ipi(int cpu, int type)
> {
>   u32 *pending_ptr = per_cpu_ptr(ipi_bits, cpu);
> 
>   while (cmpxchg(pending_ptr, 0, 1 << type) != 0)
> 	cpu_relax();
> 
>   raise_ipi(cpu);
> }

So you would have blocked the sender while there was already
a pending IPI on the target ? Why ?

The optimization proposed by Peter is actually the only interesting
change here, without it the existing set_bit was perfectly fine.
Remember that set_bit is atomic.

Ben.

> But obviously your version is nicer due to optimization, unless I'm over-analyzing it.
> 
> 
> > Also, the above can be thought of as a memory ordering issue:
> >
> >   STORE pending
> >   MB /* implied by cmpxchg */
> >   STORE ipi /* raise the actual thing */
> >
> > In that case the other end must be:
> >
> >   LOAD ipi
> >   MB /* implied by xchg */
> >   LOAD pending
> >
> > Which is what your code seems to do.
> 
> ...
> 
> >
> >> IMO the while loop is
> >> completely useless specially if IPIs are not coalesced in h/w. 
> > Agreed, the while loops seems superfluous.
> 
> Not with your version of sender, since we need it as described above.
> 
> >> And we need to move
> >> the xchg ahead of ACK'ing the IPI
> >>
> >> do_IPI
> >>    pending = xchg(&ipi_data->bits, 0);
> >>    plat_smp_ops.ipi_clear(irq);
> >>    while (ffs....)
> >>       switch(next-msg)
> >>       ...
> >>
> >> Does that look sane to you.
> > This I'm not at all certain of; continuing with the memory order analogy
> > this would allow for the case where we see 0 pending, set a bit, try and
> > raise the interrupt but then do not because its already assert.
> >
> > And since you just removed the while() loop, we'll be left with a !0
> > pending vector and nobody processing it.
> 
> Right we need it with ur version of sender. Bit don't with my simplistic one.
> 
> -Vineet
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arch" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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