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.
(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).
>
> 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
