[libvirt] [PATCH v2 3/4] Add a mutex to serialize updates to firewall

[Daniel P. Berrange]

On Mon, Jan 27, 2014 at 04:50:56PM -0500, Stefan Berger wrote:
> On 01/27/2014 12:18 PM, Daniel P. Berrange wrote:
> >The nwfilter conf update mutex previously serialized
> >updates to the internal data structures for firewall
> >rules, and updates to the firewall itself. Since the
> 
> Hm, wasn't aware (anymore) of this double-purpose.

It wasn't entirely clear to me either, but I am right in
believing that it isn't safe to have multiple threads all
creating iptables rules for different VMs at the same
time, aren't I ? 

> I also hadn't looked at this patch in the first round...
> 
> >former is going to be turned into a read/write lock
> >instead of a mutex, a new lock is required to serialize
> >access to the firewall itself.
> >
> >With this new lock, the lock ordering rules will be
> >for virNWFilter{Define,Undefine}
> >
> >       1. nwfilter driver lock
> >       2. nwfilter update lock
> 
> 
> Insert: 3. nwfilter callback drivers lock
> 
> This is then used in this order also by nwfilterStateReload
> 
> 
> >       3. virt driver lock
> >       4. domain object lock
> >       5. gentech driver lock
> >
> >and VM start
> >
> >       1. nwfilter update lock
> >       2. virt driver lock
> >       3. domain object lock
> >       4. gentech driver lock
> >
> >Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> >---
> >  src/nwfilter/nwfilter_driver.c         |  4 +++-
> >  src/nwfilter/nwfilter_gentech_driver.c | 19 +++++++++++++++++--
> >  src/nwfilter/nwfilter_gentech_driver.h |  2 +-
> >  3 files changed, 21 insertions(+), 4 deletions(-)
> >
> >diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
> >index 89913cf8..d500963 100644
> >--- a/src/nwfilter/nwfilter_gentech_driver.c
> >+++ b/src/nwfilter/nwfilter_gentech_driver.c
> >@@ -936,6 +943,7 @@ _virNWFilterInstantiateFilter(virNWFilterDriverStatePtr driver,
> >      int rc;
> >
> >      virNWFilterLockFilterUpdates();
> >+    virMutexLock(&updateMutex);
> 
> 
> Since the filter updates lock had the two purposes before, you are
> now introducing a separate lock to assign a purpose to each lock.
> Further below you are preventing concurrent teardowns to this here.
> 
> I am wondering how much further down this lock here could actually
> be pushed. This and the other function
> (virNWFilterInstantiateFilterLate) where you  place this lock are
> calling __virNWFilterInstantiateFilter and nothing else calls that
> function [and the filter read protection above the lock call will
> remain]. So I think this lock could be placed inside
> __virNWFilterInstantiateFilter(). Also looking at that function I am
> not sure whether there is anything worth protecting using this newly
> introduced lock then. It ends up calling virNWFilterInstantiate().
> Here I would be a bit careful with the threads being started to
> learn the IP addresses. So maybe this function could be the place
> where to serialize access. What's your take?

The callgraph showed the three entry points into this area of
code look like:

virNWFilterInstantiateFilterLate    virNWFilterInstantiateFilter virNWFilterTeardownFilter
                 |                         |                       |
                 +-------------------------|-----+                 |
                 |            +------------+     |                 |
                 |            |                  |                 |
                 V            V                  V                 V
          _virNWFilterInstantiateFilter       _virNWFilterTeardownFilter

Looking at the code I don't see how it is safe to allow teardown to
happen in parallel with instantiate. The teardown code could confuse
and conflict with the instantiate code causing it to fail I believe.
In particular a VM could exit causing QEMU to request teardown, while
the DHCP snoop thread is in the middle of re-creating the iptables
ruleset for a VM, so we surely require serialization of modifications
to iptables.

I could, push the locking down one level, but it wouldn't change the
level of serialization, and the lock calls are clearer at the level
of the code I have them I believe.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|