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: <529334CA.1000401@synopsys.com>
Date:	Mon, 25 Nov 2013 17:00:18 +0530
From:	Vineet Gupta <Vineet.Gupta1@...opsys.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	"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 11/25/2013 04:30 PM, Peter Zijlstra wrote:
> On Mon, Nov 25, 2013 at 04:22:18PM +0530, Vineet Gupta wrote:
>> Hi,
>>
>> I've been looking into cleaning up bitrot in ARC SMP support. Unlike some other
>> arches/platforms, we don't have per-msg-type IRQ, so the actual msg (say cross
>> function call) corresponding to IPI needs to be encoded in a per-cpu word (1 bit
>> per msg type) before kicking the IPI.
>>
>> The current code (indicative below) is completely bonkers as it calls set_bit w/o
>> any protection whatsoever, clearly racy in case of multiple senders, where
>> receiver could end up NOT seeing one of the writes.
>>
>> ipi_send_msg(cpu, msg-type)
>> {
>>    struct ipi_data *ipi_data = &per_cpu(ipi_data, cpu);
>>
>>    local_irq_save();
>>    set_bit(msg-type, &ipi_data->bits)
>>    plat_smp_ops.ipi_send(cpumask)
>>    local_irq_restore();
>> }
>>
>> Adding a spinlock here would serialize the sending part, but I still see issue in
>> receiver. Upon receipt of First IPI, the msg holding word will be atomically
>> exchanged with 0, so 2nd IPI will not see any msg in the word. Augmenting with an
>> atomic counter would only help detect the issue - but I don't see how it will help
>> elide the issue.
>>
>> Does that mean w/o proper hardware assist (i.e. IRQ providing the msg id
>> indication), the race, however small, remains ?
> 
> You can use cmpxchg to set the bit, and in case the previous value
> wasn't 0 not send a second IPI.
> 

Thx Peter, that'll do it.

While we are at it, I wanted to confirm another potential race (ARC/blackfin..)
The IPI handler clears the interrupt before atomically-read-n-clear the msg word.

do_IPI
   plat_smp_ops.ipi_clear(irq);
   while ((pending = xchg(&ipi_data->bits, 0) != 0)
      find_next_bit(....)
      switch(next-msg)

Depending on arch this could lead to an immediate IPI interrupt, and again
ipi_data->bits could get out of syn with IPI senders. IMO the while loop is
completely useless specially if IPIs are not coalesced in h/w. 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.

Thx,
-Vineet
--
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/