On 19.10.2018 18:33, Laine Stump wrote:
> On 10/18/18 2:49 AM, Nikolay Shirokovskiy wrote:
>> For filter without references to other filters hash is just sha-256 of filter's
>> xml. For filters with references hash is sha-256 of string consisting of
>> filter's self hash and hashes of directly referenced filters.
>
>
> I didn't read the entire patch, but if you're just comparing hashes of
> the filters' XML, then you aren't actually checking to see if *someone
> else* outside libvirt has modified the actual rules in ebtables/iptables
> (and someday not *too* far away nbtables, or maybe firewalld "rich
> rules"). That is the really important thing we need to check for. One of
> the reasons for a libvirtd restart to delete and reload all the rules is
> to clean up after some third party has "moved our cheese" and our filter
> rules no longer work as expected, and just checking that our own
> internal data is unchanged doesn't help that problem.
Agree. I mentioned the same issue in the cover letter. Need to rework
series)
>
>
> (BTW, on a slightly related topic - since I've lately been thinking
> about making libvirt work properly on systems with nbtables and in
> particular firewalld with an nbtables backend, I've been wondering if
> our whole model of "deleting old rules" based on our current idea of the
> libvirt xml ==> eb/iptables backend isn't improper. Maybe we should be
> saving the actual rules that we sent out in the status, and undoing
> those. The rules we need to delete could be *very* different from what
> the new libvirtd is generating (e.g. we decided we didn't need one of
> the rules anymore so it's no longer generated, or we're now using
> nbtables/ebpf/sub-etha-senso-tables or whatever).
The approach of not keeping firewall rules status is that libvirt keeps
all rules in 4 chains: libvirt-O-$IFACE, libvirt-I-$IFACE, libvirt-J-$IFACE,
libvirt-O-$IFACE (the last two are temporary) and their subchains. This
names are fixed so we can always cleanup all rules installed by libvirt.
Not sure of nbtables though - can we clean chains installed thru iptables/ebtables
thru nbtables.
Nikolay
>
>
>>
>> If filter is not complete that is some of filters it's referencing directly or
>> indirectly are missing the hash is set to NULL as well as if there was OOM on
>> hash caculation. So having NULL hash is regular situation.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
>> ---
>> src/conf/virnwfilterobj.c | 145 +++++++++++++++++++++++++++++++++
>> src/conf/virnwfilterobj.h | 9 ++
>> src/libvirt_private.syms | 3 +
>> src/nwfilter/nwfilter_driver.c | 3 +
>> src/nwfilter/nwfilter_gentech_driver.c | 2 +
>> 5 files changed, 162 insertions(+)
>>
>> diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c
>> index 0136a0d..4cc81df 100644
>> --- a/src/conf/virnwfilterobj.c
>> +++ b/src/conf/virnwfilterobj.c
>> @@ -28,6 +28,7 @@
>> #include "virlog.h"
>> #include "virnwfilterobj.h"
>> #include "virstring.h"
>> +#include "vircrypto.h"
>>
>> #define VIR_FROM_THIS VIR_FROM_NWFILTER
>>
>> @@ -40,6 +41,8 @@ struct _virNWFilterObj {
>>
>> virNWFilterDefPtr def;
>> virNWFilterDefPtr newDef;
>> +
>> + char *hash;
>> };
>>
>> struct _virNWFilterObjList {
>> @@ -82,6 +85,13 @@ virNWFilterObjGetNewDef(virNWFilterObjPtr obj)
>> }
>>
>>
>> +char*
>> +virNWFilterObjGetHash(virNWFilterObjPtr obj)
>> +{
>> + return obj->hash;
>> +}
>> +
>> +
>> bool
>> virNWFilterObjWantRemoved(virNWFilterObjPtr obj)
>> {
>> @@ -307,6 +317,139 @@ virNWFilterDefEqual(const virNWFilterDef *def1,
>> }
>>
>>
>> +static void
>> +virNWFilterObjInvalidateHash(virNWFilterObjListPtr nwfilters,
>> + virNWFilterObjPtr obj)
>> +{
>> + virNWFilterDefPtr def = obj->def;
>> + size_t i;
>> +
>> + if (!obj->hash)
>> + return;
>> +
>> + if (obj->newDef) {
>> + VIR_FREE(obj->hash);
>> + return;
>> + }
>> +
>> + for (i = 0; i < def->nentries; i++) {
>> + virNWFilterObjPtr child;
>> + char *filterref;
>> +
>> + if (def->filterEntries[i]->rule ||
>> + !def->filterEntries[i]->include)
>> + continue;
>> +
>> + filterref = def->filterEntries[i]->include->filterref;
>> +
>> + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref))) {
>> + VIR_FREE(obj->hash);
>> + return;
>> + }
>> +
>> + virNWFilterObjInvalidateHash(nwfilters, child);
>> +
>> + if (!child->hash) {
>> + VIR_FREE(obj->hash);
>> + virNWFilterObjUnlock(child);
>> + return;
>> + }
>> +
>> + virNWFilterObjUnlock(child);
>> + }
>> +}
>> +
>> +
>> +void
>> +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters)
>> +{
>> + size_t i;
>> + virNWFilterObjPtr obj;
>> +
>> + for (i = 0; i < nwfilters->count; i++) {
>> + obj = nwfilters->objs[i];
>> + virNWFilterObjLock(obj);
>> + virNWFilterObjInvalidateHash(nwfilters, obj);
>> + virNWFilterObjUnlock(obj);
>> + }
>> +}
>> +
>> +
>> +static void
>> +virNWFilterObjUpdateHash(virNWFilterObjListPtr nwfilters,
>> + virNWFilterObjPtr obj)
>> +{
>> + virNWFilterDefPtr def = obj->newDef ? obj->newDef : obj->def;
>> + size_t i;
>> + char *hash;
>> + char *xml;
>> + virBuffer buf = VIR_BUFFER_INITIALIZER;
>> +
>> + if (obj->hash)
>> + return;
>> +
>> + if (!(xml = virNWFilterDefFormat(def)))
>> + return;
>> +
>> + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, xml, &hash))
>> + goto cleanup;
>> +
>> + virBufferAsprintf(&buf, "%s\n", hash);
>> + VIR_FREE(hash);
>> +
>> + for (i = 0; i < def->nentries; i++) {
>> + char *filterref;
>> + virNWFilterObjPtr child;
>> +
>> + if (def->filterEntries[i]->rule ||
>> + !def->filterEntries[i]->include)
>> + continue;
>> +
>> + filterref = def->filterEntries[i]->include->filterref;
>> +
>> + if (!(child = virNWFilterObjListFindByName(nwfilters, filterref)))
>> + goto cleanup;
>> +
>> + virNWFilterObjUpdateHash(nwfilters, child);
>> +
>> + if (!child->hash) {
>> + virNWFilterObjUnlock(child);
>> + goto cleanup;
>> + }
>> +
>> + virBufferAsprintf(&buf, "%s\n", child->hash);
>> +
>> + virNWFilterObjUnlock(child);
>> + }
>> +
>> + if (virBufferCheckError(&buf) < 0)
>> + goto cleanup;
>> +
>> + hash = virBufferContentAndReset(&buf);
>> + ignore_value(virCryptoHashString(VIR_CRYPTO_HASH_SHA256, hash, &obj->hash));
>> + VIR_FREE(hash);
>> +
>> + cleanup:
>> + virBufferFreeAndReset(&buf);
>> + VIR_FREE(xml);
>> +}
>> +
>> +
>> +void
>> +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters)
>> +{
>> + size_t i;
>> + virNWFilterObjPtr obj;
>> +
>> + for (i = 0; i < nwfilters->count; i++) {
>> + obj = nwfilters->objs[i];
>> + virNWFilterObjLock(obj);
>> + virNWFilterObjUpdateHash(nwfilters, obj);
>> + virNWFilterObjUnlock(obj);
>> + }
>> +}
>> +
>> +
>> virNWFilterObjPtr
>> virNWFilterObjListAssignDef(virNWFilterObjListPtr nwfilters,
>> virNWFilterDefPtr def)
>> @@ -555,6 +698,8 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters,
>> virNWFilterObjUnlock(obj);
>> }
>>
>> + virNWFilterObjListUpdateHash(nwfilters);
>> +
>> VIR_DIR_CLOSE(dir);
>> return ret;
>> }
>> diff --git a/src/conf/virnwfilterobj.h b/src/conf/virnwfilterobj.h
>> index 4a54dd5..1c41a3e 100644
>> --- a/src/conf/virnwfilterobj.h
>> +++ b/src/conf/virnwfilterobj.h
>> @@ -50,6 +50,9 @@ virNWFilterObjGetDef(virNWFilterObjPtr obj);
>> virNWFilterDefPtr
>> virNWFilterObjGetNewDef(virNWFilterObjPtr obj);
>>
>> +char*
>> +virNWFilterObjGetHash(virNWFilterObjPtr obj);
>> +
>> bool
>> virNWFilterObjWantRemoved(virNWFilterObjPtr obj);
>>
>> @@ -114,4 +117,10 @@ virNWFilterObjLock(virNWFilterObjPtr obj);
>> void
>> virNWFilterObjUnlock(virNWFilterObjPtr obj);
>>
>> +void
>> +virNWFilterObjListInvalidateHash(virNWFilterObjListPtr nwfilters);
>> +
>> +void
>> +virNWFilterObjListUpdateHash(virNWFilterObjListPtr nwfilters);
>> +
>> #endif /* VIRNWFILTEROBJ_H */
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 335210c..a7cfe80 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -1077,6 +1077,7 @@ virNWFilterBindingObjListRemove;
>>
>> # conf/virnwfilterobj.h
>> virNWFilterObjGetDef;
>> +virNWFilterObjGetHash;
>> virNWFilterObjGetNewDef;
>> virNWFilterObjListAssignDef;
>> virNWFilterObjListExport;
>> @@ -1085,10 +1086,12 @@ virNWFilterObjListFindByUUID;
>> virNWFilterObjListFindInstantiateFilter;
>> virNWFilterObjListFree;
>> virNWFilterObjListGetNames;
>> +virNWFilterObjListInvalidateHash;
>> virNWFilterObjListLoadAllConfigs;
>> virNWFilterObjListNew;
>> virNWFilterObjListNumOfNWFilters;
>> virNWFilterObjListRemove;
>> +virNWFilterObjListUpdateHash;
>> virNWFilterObjLock;
>> virNWFilterObjTestUnassignDef;
>> virNWFilterObjUnlock;
>> diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c
>> index 1ab906f..5591c0b 100644
>> --- a/src/nwfilter/nwfilter_driver.c
>> +++ b/src/nwfilter/nwfilter_driver.c
>> @@ -570,6 +570,8 @@ nwfilterDefineXML(virConnectPtr conn,
>> def = NULL;
>> objdef = virNWFilterObjGetDef(obj);
>>
>> + virNWFilterObjListUpdateHash(driver->nwfilters);
>> +
>> if (virNWFilterSaveConfig(driver->configDir, objdef) < 0) {
>> virNWFilterObjListRemove(driver->nwfilters, obj);
>> goto cleanup;
>> @@ -616,6 +618,7 @@ nwfilterUndefine(virNWFilterPtr nwfilter)
>> goto cleanup;
>>
>> virNWFilterObjListRemove(driver->nwfilters, obj);
>> + virNWFilterObjListInvalidateHash(driver->nwfilters);
>> obj = NULL;
>> ret = 0;
>>
>> diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c
>> index e5dea91..d64621b 100644
>> --- a/src/nwfilter/nwfilter_gentech_driver.c
>> +++ b/src/nwfilter/nwfilter_gentech_driver.c
>> @@ -1066,6 +1066,8 @@ virNWFilterBuildAll(virNWFilterDriverStatePtr driver,
>> virNWFilterBuildIter,
>> &data);
>> } else {
>> + virNWFilterObjListInvalidateHash(driver->nwfilters);
>> + virNWFilterObjListUpdateHash(driver->nwfilters);
>> data.step = STEP_SWITCH;
>> virNWFilterBindingObjListForEach(driver->bindings,
>> virNWFilterBuildIter,
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
