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

Stefan Berger stefanb at linux.vnet.ibm.com
Tue Jan 28 23:56:47 UTC 2014


On 01/28/2014 07:38 AM, Daniel P. Berrange wrote:
> On Tue, Jan 28, 2014 at 07:31:04AM -0500, Stefan Berger wrote:
>> On 01/28/2014 06:15 AM, Daniel P. Berrange wrote:
>>> 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 ?
>> At least one thread shouldn't try to instantiate filters for one
>> (VM) interface while another one tears them down. So that would be
>> serialization per interface. I think instantiation of filters could
>> be done concurrently for different interfaces, but not the execution
>> of the iptables commands themselves, though. The latter is locking
>> that needs to be done by the ebtables/iptables driver and that
>> driver does serialize the execution of all scripts using the
>> execCLIMutex. The ebtables and iptables rules are created on a
>> per-interface basis, all hooking into some form of 'root' chains.
>> The critical part here is that the 'root' chains can be accessed
>> concurrently by different interfaces -- from what I can tell is that
>> all the scripts that are run by the eb/iptables driver only need to
>> be serialized and that is done with that execCLIMutex. So we should
>> be fine from that perspective.
>>
>> At least locking on a per-interface basis is already happening in
>> the 'gentech' driver:
>>
>>
>> nwfilter_gentech_driver.c::virNWFilterInstantiate
>>
>> [...]
>>          if (virNWFilterLockIface(ifname) < 0)
>>              goto err_exit;
>>
>>          rc = techdriver->applyNewRules(ifname, nptrs, ptrs);
>>
>>          if (teardownOld && rc == 0)
>>              techdriver->tearOldRules(ifname);
>>
>>          if (rc == 0 && (virNetDevValidateConfig(ifname, NULL,
>> ifindex) <= 0)) {
>>              virResetLastError();
>>              /* interface changed/disppeared */
>>              techdriver->allTeardown(ifname);
>>              rc = -1;
>>          }
>>
>>          virNWFilterUnlockIface(ifname);
>> [...]
>>
>>
>>
>> nwfilter_gentech_driver.c::_virNWFilterTeardownFilter
>>
>> [...]
>>      if (virNWFilterLockIface(ifname) < 0)
>>         return -1;
>>
>>      techdriver->allTeardown(ifname);
>>
>>      virNWFilterIPAddrMapDelIPAddr(ifname, NULL);
>>
>>      virNWFilterUnlockIface(ifname);
>> [...]
>>
>>
>> (Besides the above calls to the 'techdriver', there are others that
>> call into the techdriver during the test phases of a filter updated.
>> They hold the writer lock to the filter updates and with this block
>> every concurrent thread then.)
>>
>> I may be missing something subtle, but I think there is already
>> enough serialization happening per interface.
> Ok, I think you might be right then, and we can just skip this
> patch entirely and rely in the ifname locks.

I definitely hope so.

Regards,
     Stefan
>
> Daniel




More information about the libvir-list mailing list