On Wed, Jul 18, 2012 at 09:27:42AM +0300, Gleb Natapov wrote: > On Tue, Jul 17, 2012 at 07:14:52PM +0300, Michael S. Tsirkin wrote: > > > _Seems_ racy, or _is_ racy? Please identify the race. > > > > Look at this: > > > > static inline int kvm_irq_line_state(unsigned long *irq_state, > > int irq_source_id, int level) > > { > > /* Logical OR for level trig interrupt */ > > if (level) > > set_bit(irq_source_id, irq_state); > > else > > clear_bit(irq_source_id, irq_state); > > > > return !!(*irq_state); > > } > > > > > > Now: > > If other CPU changes some other bit after the atomic change, > > it looks like !!(*irq_state) might return a stale value. > > > > CPU 0 clears bit 0. CPU 1 sets bit 1. CPU 1 sets level to 1. > > If CPU 0 sees a stale value now it will return 0 here > > and interrupt will get cleared. > > > This will hardly happen on x86 especially since bit is set with > serialized instruction. But there is actually a race here. > CPU 0 clears bit 0. CPU 0 read irq_state as 0. CPU 1 sets level to 1. > CPU 1 calls kvm_ioapic_set_irq(1). CPU 0 calls kvm_ioapic_set_irq(0). > No ioapic thinks the level is 0 but irq_state is not 0. > > This untested and un-compiled patch should fix it. Getting rid of atomics completely makes me more comfortable, and by moving all bitmap handling to under pic/ioapic lock we can do just that. I just tested and posted a patch that fixes the race in this way. Could you take a look pls? -- MST -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html