firewall3: ipset: Handle reload_set properly

The reload_set option was added in commit 509e673dab01 ("firewall3:
Improve ipset support"), and the purpose of the option is to control if
a set should be flushed or not on a firewall reload.

In some cases, the option unfortunately does not work properly. I had
fixed the errors locally, but failed to submit a v2 of "Improve ipset
support". This patch contains my local fixes, and after the following
changes are applied then the option (as well as ipset support) works as
at least I expect.

The following errors have been fixed:

* "family" was not written to the state file, causing all sets read from
this file was considered as ipv4. Save family to ensure that sets are
handled correctly on firewall reload.

* The default value of "reload_set" is false, meaning that the
reload-check in "fw3_create_ipsets()" is always true (on reload). A
consequence of this is that new sets are never created on firewall
reload. In order to ensure that new sets are created, only consider
"reload_set" if the set exists. If a set (from configuration) does not
exist, we always want to create it.

* On reload and before "fw3_destroy_ipsets()" are called, we need to
update run_state to ensure that sets are updated correctly. We need to
check if the sets in run_state is found in cfg_state, if not the set
should be destroyed (done by forcing reload_set to true). If the set is
found, then we copy the value of reload_set to the set in run_state so
that the elements are updated as the user expects.

Since we now always copy the value of reload_set from cfg_state, there
is no need to write reload_set to run_state.

Signed-off-by: Kristian Evensen <kristian.evensen@gmail.com>

diff --git a/ipsets.c b/ipsets.c
index 93bde0d30d998278a00b9c0d5cce57513c4810f6..280845b9b7105c7aff6ec7b320cc647f01a7e775 100644 (file)
--- a/ipsets.c
+++ b/ipsets.c
@@ -427,13 +427,16 @@ fw3_create_ipsets(struct fw3_state *state, enum fw3_family family,
        /* spawn ipsets */
        list_for_each_entry(ipset, &state->ipsets, list)
        {
-               if (ipset->family != family ||
-                   (reload_set && !ipset->reload_set))
+               if (ipset->family != family)
                        continue;
 
                if (ipset->external)
                        continue;
 
+               if (fw3_check_ipset(ipset) &&
+                   (reload_set && !ipset->reload_set))
+                       continue;
+
                if (!exec)
                {
                        exec = fw3_command_pipe(false, "ipset", "-exist", "-");
@@ -568,3 +571,43 @@ out:
 
        return rv;
 }
+
+void
+fw3_ipsets_update_run_state(enum fw3_family family, struct fw3_state *run_state,
+                           struct fw3_state *cfg_state)
+{
+       struct fw3_ipset *ipset_run, *ipset_cfg;
+       bool in_cfg;
+
+       list_for_each_entry(ipset_run, &run_state->ipsets, list) {
+               if (ipset_run->family != family)
+                       continue;
+
+               in_cfg = false;
+
+               list_for_each_entry(ipset_cfg, &cfg_state->ipsets, list) {
+                       if (ipset_cfg->family != family)
+                               continue;
+
+                       if (strlen(ipset_run->name) ==
+                           strlen(ipset_cfg->name) &&
+                           !strcmp(ipset_run->name, ipset_cfg->name)) {
+                               in_cfg = true;
+                               break;
+                       }
+               }
+
+               /* If a set is found in run_state, but not in cfg_state then the
+                * set has been deleted/renamed. Set reload_set to true to force
+                * the old set to be destroyed in the "stop" fase of the reload.
+                * If the set is found, then copy the reload_set value from the
+                * configuration state. This ensures that the elements are
+                * always updated according to the configuration, and not the
+                * runtime state (which the user might have forgotten).
+                */
+               if (!in_cfg)
+                       ipset_run->reload_set = true;
+               else
+                       ipset_run->reload_set = ipset_cfg->reload_set;
+       }
+}

diff --git a/ipsets.h b/ipsets.h
index fec79f8754ad88eb43089d28752a9ac733589b34..76078d4dfcbe017d61e82e3e50b169f668e946ab 100644 (file)
--- a/ipsets.h
+++ b/ipsets.h
@@ -37,6 +37,10 @@ struct fw3_ipset * fw3_lookup_ipset(struct fw3_state *state, const char *name);
 
 bool fw3_check_ipset(struct fw3_ipset *set);
 
+void
+fw3_ipsets_update_run_state(enum fw3_family family, struct fw3_state *run_state,
+                           struct fw3_state *cfg_state);
+
 static inline void fw3_free_ipset(struct fw3_ipset *ipset)
 {
        list_del(&ipset->list);

diff --git a/main.c b/main.c
index 8d9a2e8d432debc61c19d4b13f72a4556e3a9276..7ad00b424a7ce3355593ac35f8227ba4b47d2003 100644 (file)
--- a/main.c
+++ b/main.c
@@ -354,6 +354,7 @@ reload(void)
                        fw3_ipt_close(handle);
                }
 
+               fw3_ipsets_update_run_state(family, run_state, cfg_state);
                fw3_destroy_ipsets(run_state, family, true);
 
                family_set(run_state, family, false);

diff --git a/utils.c b/utils.c
index 147acf8b3707b5b06488d360ad7543b4723c6934..b465878a71f2bfc4cb3f872fbf23b5d28aec4766 100644 (file)
--- a/utils.c
+++ b/utils.c
@@ -586,8 +586,11 @@ write_ipset_uci(struct uci_context *ctx, struct fw3_ipset *s,
        uci_set(ctx, &ptr);
 
        ptr.o      = NULL;
-       ptr.option = "reload_set";
-       ptr.value  = s->reload_set ? "true" : "false";
+       ptr.option = "family";
+       if (s->family == FW3_FAMILY_V4)
+               ptr.value = "ipv4";
+       else
+               ptr.value = "ipv6";
        uci_set(ctx, &ptr);
 
        ptr.o      = NULL;