linux-kernel - Re: [KVM PATCH v3 1/3] KVM: fix race in irq

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: <20091027140237.GM29477@redhat.com>
Date:	Tue, 27 Oct 2009 16:02:37 +0200
From:	Gleb Natapov <gleb@...hat.com>
To:	Gregory Haskins <gregory.haskins@...il.com>
Cc:	Gregory Haskins <ghaskins@...ell.com>, kvm@...r.kernel.org,
	"alacrityvm-devel@...ts.sourceforge.net" 
	<alacrityvm-devel@...ts.sourceforge.net>,
	linux-kernel@...r.kernel.org, paulmck@...ux.vnet.ibm.com
Subject: Re: [KVM PATCH v3 1/3] KVM: fix race in irq_routing logic

On Tue, Oct 27, 2009 at 09:39:03AM -0400, Gregory Haskins wrote:
> Gleb Natapov wrote:
> > On Mon, Oct 26, 2009 at 12:21:57PM -0400, Gregory Haskins wrote:
> >> The current code suffers from the following race condition:
> >>
> >> thread-1                                    thread-2
> >> -----------------------------------------------------------
> >>
> >> kvm_set_irq() {
> >>    rcu_read_lock()
> >>    irq_rt = rcu_dereference(table);
> >>    rcu_read_unlock();
> >>
> >>                                        kvm_set_irq_routing() {
> >>                                           mutex_lock();
> >>                                           irq_rt = table;
> >>                                           rcu_assign_pointer();
> >>                                           mutex_unlock();
> >>                                           synchronize_rcu();
> >>
> >>                                           kfree(irq_rt);
> >>
> >>    irq_rt->entry->set(); /* bad */
> >>
> > This is not what happens. irq_rt is never accessed outside read-side
> > critical section.
> 
> Sorry, I was generalizing to keep the comments short.  I figured it
> would be clear what I was actually saying, but realize in retrospect
> that I was a little ambiguous.
> 
A little is underestimation :) There is not /* bad */ line in the code!

> Yes, irq_rt is not accessed outside the RSCS.  However, the entry
> pointers stored in the irq_rt->map are, and this is equally problematic
> afaict.
The pointer is in text and can't disappear without kvm_set_irq()
disappearing too.

> 
> In this particular case we seem to never delete entries at run-time once
> they are established.  Therefore, while perhaps sloppy, its technically
> safe to leave them unprotected from this perspective.  The issue is more
> related to shutdown since a kvm_set_irq() caller could be within the
> aforementioned race-region and call entry->set() after the guest is
> gone.  Or did I miss something?
> 
The caller of kvm_set_irq() should hold reference to kvm instance, so it
can't disappear while you are inside kvm_set_irq(). RCU protects only
kvm->irq_routing not kvm structure itself.

> > Data is copied from irq_rt onto the stack and this copy is accessed
> > outside critical section.
> 
> As mentioned above, I do not believe this really protect us.  And even
I don't see the prove it doesn't, so I assume it does.

> if it did, the copy is just a work-around to avoid sleeping within the
It is not a work-around. There was two solutions to the problem one is
to call ->set() outside rcu critical section another is to use SRCU. I
decided to use the first one. This way the code is much simpler and I
remember asking Paul what are the disadvantages of using SRCU and there
was something.

> standard RCU RSCS, which is what SRCU is designed for.  So rather than
> inventing an awkward two-phased stack based solution, it's better to
> reuse the provided tools, IMO.
> 
> To flip it around:  Is there any reason why an SRCU would not work here,
> and thus we were forced to use something like the stack-copy approach?
> 
If SRCU has no disadvantage comparing to RCU why not use it always? :)

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