iptables-restore: support acquiring the lock.

Currently, ip[6]tables-restore does not perform any locking, so it
is not safe to use concurrently with ip[6]tables.

This patch makes ip[6]tables-restore wait for the lock if -w
was specified. Arguments to -w and -W are supported in the same
was as they are in ip[6]tables.

The lock is not acquired on startup. Instead, it is acquired when
a new table handle is created (on encountering '*') and released
when the table is committed (COMMIT). This makes it possible to
keep long-running iptables-restore processes in the background
(for example, reading commands from a pipe opened by a system
management daemon) and simultaneously run iptables commands.

If -w is not specified, then the command proceeds without taking
the lock.

Tested as follows:

1. Run iptables-restore -w, and check that iptables commands work
   with or without -w.
2. Type "*filter" into the iptables-restore input. Verify that
   a) ip[6]tables commands without -w fail with "another app is
      currently holding the xtables lock...".
   b) ip[6]tables commands with "-w 2" fail after 2 seconds.
   c) ip[6]tables commands with "-w" hang until "COMMIT" is
      typed into the iptables-restore window.
3. With the lock held by an ip6tables-restore process:
     strace -e flock /tmp/iptables/sbin/iptables-restore -w 1 -W 100000
   shows 11 calls to flock and fails.
4. Run an iptables-restore with -w and one without -w, and check:
   a) Type "*filter" in the first and then the second, and the
      second exits with an error.
   b) Type "*filter" in the second and "*filter" "-S" "COMMIT"
      into the first. The rules are listed only when the first
      copy sees "COMMIT".

Signed-off-by: Narayan Kamath <narayan@google.com>
Signed-off-by: Lorenzo Colitti <lorenzo@google.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index dc0acb05a48a8df73278639f8ea656005b7a57d2..8a47f09c950394b67bc288e5368c27f4d746b1e5 100644 (file)
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -15,6 +15,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include "ip6tables.h"
+#include "xshared.h"
 #include "xtables.h"
 #include "libiptc/libip6tc.h"
 #include "ip6tables-multi.h"
@@ -25,17 +26,23 @@
 #define DEBUGP(x, args...)
 #endif
 
-static int counters = 0, verbose = 0, noflush = 0;
+static int counters = 0, verbose = 0, noflush = 0, wait = 0;
+
+static struct timeval wait_interval = {
+       .tv_sec = 1,
+};
 
 /* Keeping track of external matches and targets.  */
 static const struct option options[] = {
-       {.name = "counters", .has_arg = false, .val = 'c'},
-       {.name = "verbose",  .has_arg = false, .val = 'v'},
-       {.name = "test",     .has_arg = false, .val = 't'},
-       {.name = "help",     .has_arg = false, .val = 'h'},
-       {.name = "noflush",  .has_arg = false, .val = 'n'},
-       {.name = "modprobe", .has_arg = true,  .val = 'M'},
-       {.name = "table",    .has_arg = true,  .val = 'T'},
+       {.name = "counters",      .has_arg = 0, .val = 'c'},
+       {.name = "verbose",       .has_arg = 0, .val = 'v'},
+       {.name = "test",          .has_arg = 0, .val = 't'},
+       {.name = "help",          .has_arg = 0, .val = 'h'},
+       {.name = "noflush",       .has_arg = 0, .val = 'n'},
+       {.name = "modprobe",      .has_arg = 1, .val = 'M'},
+       {.name = "table",         .has_arg = 1, .val = 'T'},
+       {.name = "wait",          .has_arg = 2, .val = 'w'},
+       {.name = "wait-interval", .has_arg = 2, .val = 'W'},
        {NULL},
 };
 
@@ -43,14 +50,16 @@ static void print_usage(const char *name, const char *version) __attribute__((no
 
 static void print_usage(const char *name, const char *version)
 {
-       fprintf(stderr, "Usage: %s [-c] [-v] [-t] [-h] [-n] [-T table] [-M command]\n"
+       fprintf(stderr, "Usage: %s [-c] [-v] [-t] [-h] [-n] [-w secs] [-W usecs] [-T table] [-M command]\n"
                        "          [ --counters ]\n"
                        "          [ --verbose ]\n"
                        "          [ --test ]\n"
                        "          [ --help ]\n"
                        "          [ --noflush ]\n"
+                       "          [ --wait=<seconds>\n"
+                       "          [ --wait-interval=<usecs>\n"
                        "          [ --table=<TABLE> ]\n"
-                       "          [ --modprobe=<command> ]\n", name);
+                       "          [ --modprobe=<command> ]\n", name);
 
        exit(1);
 }
@@ -181,7 +190,7 @@ int ip6tables_restore_main(int argc, char *argv[])
 {
        struct xtc_handle *handle = NULL;
        char buffer[10240];
-       int c;
+       int c, lock;
        char curtable[XT_TABLE_MAXNAMELEN + 1];
        FILE *in;
        int in_table = 0, testing = 0;
@@ -189,6 +198,7 @@ int ip6tables_restore_main(int argc, char *argv[])
        const struct xtc_ops *ops = &ip6tc_ops;
 
        line = 0;
+       lock = XT_LOCK_NOT_ACQUIRED;
 
        ip6tables_globals.program_name = "ip6tables-restore";
        c = xtables_init_all(&ip6tables_globals, NFPROTO_IPV6);
@@ -203,7 +213,7 @@ int ip6tables_restore_main(int argc, char *argv[])
        init_extensions6();
 #endif
 
-       while ((c = getopt_long(argc, argv, "bcvthnM:T:", options, NULL)) != -1) {
+       while ((c = getopt_long(argc, argv, "bcvthnwWM:T:", options, NULL)) != -1) {
                switch (c) {
                        case 'b':
                                fprintf(stderr, "-b/--binary option is not implemented\n");
@@ -224,6 +234,12 @@ int ip6tables_restore_main(int argc, char *argv[])
                        case 'n':
                                noflush = 1;
                                break;
+                       case 'w':
+                               wait = parse_wait_time(argc, argv);
+                               break;
+                       case 'W':
+                               parse_wait_interval(argc, argv, &wait_interval);
+                               break;
                        case 'M':
                                xtables_modprobe_program = optarg;
                                break;
@@ -268,8 +284,23 @@ int ip6tables_restore_main(int argc, char *argv[])
                                DEBUGP("Not calling commit, testing\n");
                                ret = 1;
                        }
+
+                       /* Done with the current table, release the lock. */
+                       if (lock >= 0) {
+                               xtables_unlock(lock);
+                               lock = XT_LOCK_NOT_ACQUIRED;
+                       }
+
                        in_table = 0;
                } else if ((buffer[0] == '*') && (!in_table)) {
+                       /* Acquire a lock before we create a new table handle */
+                       lock = xtables_lock(wait, &wait_interval);
+                       if (lock == XT_LOCK_BUSY) {
+                               fprintf(stderr, "Another app is currently holding the xtables lock. "
+                                       "Perhaps you want to use the -w option?\n");
+                               exit(RESOURCE_PROBLEM);
+                       }
+
                        /* New table */
                        char *table;
 

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 4d77721b0473b0d3c8815a0df7fceb52faee7697..579d347b090fa15811dc25ac06d1365277757036 100644 (file)
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -1779,7 +1779,7 @@ int do_command6(int argc, char *argv[], char **table,
        generic_opt_check(command, cs.options);
 
        /* Attempt to acquire the xtables lock */
-       if (!restore && !xtables_lock(wait, &wait_interval)) {
+       if (!restore && xtables_lock(wait, &wait_interval) == XT_LOCK_BUSY) {
                fprintf(stderr, "Another app is currently holding the xtables lock. ");
                if (wait == 0)
                        fprintf(stderr, "Perhaps you want to use the -w option?\n");

diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 2f1522db523ec2f524ba0ceb35948b3fdcf3336e..7bb06d84b1bfbb54ed2017dd1ebb0b0b4a2abdc9 100644 (file)
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -12,6 +12,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include "iptables.h"
+#include "xshared.h"
 #include "xtables.h"
 #include "libiptc/libiptc.h"
 #include "iptables-multi.h"
@@ -22,17 +23,23 @@
 #define DEBUGP(x, args...)
 #endif
 
-static int counters = 0, verbose = 0, noflush = 0;
+static int counters = 0, verbose = 0, noflush = 0, wait = 0;
+
+static struct timeval wait_interval = {
+       .tv_sec = 1,
+};
 
 /* Keeping track of external matches and targets.  */
 static const struct option options[] = {
-       {.name = "counters", .has_arg = false, .val = 'c'},
-       {.name = "verbose",  .has_arg = false, .val = 'v'},
-       {.name = "test",     .has_arg = false, .val = 't'},
-       {.name = "help",     .has_arg = false, .val = 'h'},
-       {.name = "noflush",  .has_arg = false, .val = 'n'},
-       {.name = "modprobe", .has_arg = true,  .val = 'M'},
-       {.name = "table",    .has_arg = true,  .val = 'T'},
+       {.name = "counters",      .has_arg = 0, .val = 'c'},
+       {.name = "verbose",       .has_arg = 0, .val = 'v'},
+       {.name = "test",          .has_arg = 0, .val = 't'},
+       {.name = "help",          .has_arg = 0, .val = 'h'},
+       {.name = "noflush",       .has_arg = 0, .val = 'n'},
+       {.name = "modprobe",      .has_arg = 1, .val = 'M'},
+       {.name = "table",         .has_arg = 1, .val = 'T'},
+       {.name = "wait",          .has_arg = 2, .val = 'w'},
+       {.name = "wait-interval", .has_arg = 2, .val = 'W'},
        {NULL},
 };
 
@@ -42,14 +49,16 @@ static void print_usage(const char *name, const char *version) __attribute__((no
 
 static void print_usage(const char *name, const char *version)
 {
-       fprintf(stderr, "Usage: %s [-c] [-v] [-t] [-h] [-n] [-T table] [-M command]\n"
+       fprintf(stderr, "Usage: %s [-c] [-v] [-t] [-h] [-n] [-w secs] [-W usecs] [-T table] [-M command]\n"
                        "          [ --counters ]\n"
                        "          [ --verbose ]\n"
                        "          [ --test ]\n"
                        "          [ --help ]\n"
                        "          [ --noflush ]\n"
+                       "          [ --wait=<seconds>\n"
+                       "          [ --wait-interval=<usecs>\n"
                        "          [ --table=<TABLE> ]\n"
-                       "          [ --modprobe=<command> ]\n", name);
+                       "          [ --modprobe=<command> ]\n", name);
 
        exit(1);
 }
@@ -180,7 +189,7 @@ iptables_restore_main(int argc, char *argv[])
 {
        struct xtc_handle *handle = NULL;
        char buffer[10240];
-       int c;
+       int c, lock;
        char curtable[XT_TABLE_MAXNAMELEN + 1];
        FILE *in;
        int in_table = 0, testing = 0;
@@ -188,6 +197,7 @@ iptables_restore_main(int argc, char *argv[])
        const struct xtc_ops *ops = &iptc_ops;
 
        line = 0;
+       lock = XT_LOCK_NOT_ACQUIRED;
 
        iptables_globals.program_name = "iptables-restore";
        c = xtables_init_all(&iptables_globals, NFPROTO_IPV4);
@@ -202,7 +212,7 @@ iptables_restore_main(int argc, char *argv[])
        init_extensions4();
 #endif
 
-       while ((c = getopt_long(argc, argv, "bcvthnM:T:", options, NULL)) != -1) {
+       while ((c = getopt_long(argc, argv, "bcvthnwWM:T:", options, NULL)) != -1) {
                switch (c) {
                        case 'b':
                                fprintf(stderr, "-b/--binary option is not implemented\n");
@@ -223,6 +233,12 @@ iptables_restore_main(int argc, char *argv[])
                        case 'n':
                                noflush = 1;
                                break;
+                       case 'w':
+                               wait = parse_wait_time(argc, argv);
+                               break;
+                       case 'W':
+                               parse_wait_interval(argc, argv, &wait_interval);
+                               break;
                        case 'M':
                                xtables_modprobe_program = optarg;
                                break;
@@ -267,8 +283,23 @@ iptables_restore_main(int argc, char *argv[])
                                DEBUGP("Not calling commit, testing\n");
                                ret = 1;
                        }
+
+                       /* Done with the current table, release the lock. */
+                       if (lock >= 0) {
+                               xtables_unlock(lock);
+                               lock = XT_LOCK_NOT_ACQUIRED;
+                       }
+
                        in_table = 0;
                } else if ((buffer[0] == '*') && (!in_table)) {
+                       /* Acquire a lock before we create a new table handle */
+                       lock = xtables_lock(wait, &wait_interval);
+                       if (lock == XT_LOCK_BUSY) {
+                               fprintf(stderr, "Another app is currently holding the xtables lock. "
+                                       "Perhaps you want to use the -w option?\n");
+                               exit(RESOURCE_PROBLEM);
+                       }
+
                        /* New table */
                        char *table;
 

diff --git a/iptables/iptables.c b/iptables/iptables.c
index 04be5abb10c500d9944d85d34e27616be1e7b434..62731c5ebb5f1a021f8bc1ab79b64f1f59290550 100644 (file)
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -1766,7 +1766,7 @@ int do_command4(int argc, char *argv[], char **table,
        generic_opt_check(command, cs.options);
 
        /* Attempt to acquire the xtables lock */
-       if (!restore && !xtables_lock(wait, &wait_interval)) {
+       if (!restore && xtables_lock(wait, &wait_interval) == XT_LOCK_BUSY) {
                fprintf(stderr, "Another app is currently holding the xtables lock. ");
                if (wait == 0)
                        fprintf(stderr, "Perhaps you want to use the -w option?\n");

diff --git a/iptables/xshared.c b/iptables/xshared.c
index 06147d180a6487818bd21ec3d7064ee7f5a0e7c5..3fbe3b1a99b77d01c9c4c50d9a1a827ff802a772 100644 (file)
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -246,7 +246,7 @@ void xs_init_match(struct xtables_match *match)
                match->init(match->m);
 }
 
-bool xtables_lock(int wait, struct timeval *wait_interval)
+int xtables_lock(int wait, struct timeval *wait_interval)
 {
        struct timeval time_left, wait_time;
        int fd, i = 0;
@@ -256,22 +256,22 @@ bool xtables_lock(int wait, struct timeval *wait_interval)
 
        fd = open(XT_LOCK_NAME, O_CREAT, 0600);
        if (fd < 0)
-               return true;
+               return XT_LOCK_UNSUPPORTED;
 
        if (wait == -1) {
                if (flock(fd, LOCK_EX) == 0)
-                       return true;
+                       return fd;
 
                fprintf(stderr, "Can't lock %s: %s\n", XT_LOCK_NAME,
                        strerror(errno));
-               return false;
+               return XT_LOCK_BUSY;
        }
 
        while (1) {
                if (flock(fd, LOCK_EX | LOCK_NB) == 0)
-                       return true;
+                       return fd;
                else if (timercmp(&time_left, wait_interval, <))
-                       return false;
+                       return XT_LOCK_BUSY;
 
                if (++i % 10 == 0) {
                        fprintf(stderr, "Another app is currently holding the xtables lock; "
@@ -285,6 +285,12 @@ bool xtables_lock(int wait, struct timeval *wait_interval)
        }
 }
 
+void xtables_unlock(int lock)
+{
+       if (lock >= 0)
+               close(lock);
+}
+
 int parse_wait_time(int argc, char *argv[])
 {
        int wait = -1;

diff --git a/iptables/xshared.h b/iptables/xshared.h
index d8a697ae665dd91e1a7bf14dcacfa5a5e097a222..539e6c243b519c3c3886b9c49035c841b85402ed 100644 (file)
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -86,7 +86,28 @@ extern struct xtables_match *load_proto(struct iptables_command_state *);
 extern int subcmd_main(int, char **, const struct subcommand *);
 extern void xs_init_target(struct xtables_target *);
 extern void xs_init_match(struct xtables_match *);
-bool xtables_lock(int wait, struct timeval *wait_interval);
+
+/**
+ * Values for the iptables lock.
+ *
+ * A value >= 0 indicates the lock filedescriptor. Other values are:
+ *
+ * XT_LOCK_UNSUPPORTED : The system does not support locking, execution will
+ * proceed lockless.
+ *
+ * XT_LOCK_BUSY : The lock was held by another process. xtables_lock only
+ * returns this value when |wait| == false. If |wait| == true, xtables_lock
+ * will not return unless the lock has been acquired.
+ *
+ * XT_LOCK_NOT_ACQUIRED : We have not yet attempted to acquire the lock.
+ */
+enum {
+       XT_LOCK_BUSY = -1,
+       XT_LOCK_UNSUPPORTED  = -2,
+       XT_LOCK_NOT_ACQUIRED  = -3,
+};
+extern int xtables_lock(int wait, struct timeval *tv);
+extern void xtables_unlock(int lock);
 
 int parse_wait_time(int argc, char *argv[]);
 void parse_wait_interval(int argc, char *argv[], struct timeval *wait_interval);