From 999eaa241212d3952ddff39a99d0d55a74e3639e Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Thu, 16 Mar 2017 16:55:02 +0900 Subject: 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 Signed-off-by: Lorenzo Colitti Signed-off-by: Pablo Neira Ayuso --- iptables/iptables-restore.c | 55 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 12 deletions(-) (limited to 'iptables/iptables-restore.c') diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c index 2f1522db..7bb06d84 100644 --- a/iptables/iptables-restore.c +++ b/iptables/iptables-restore.c @@ -12,6 +12,7 @@ #include #include #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=\n" + " [ --wait-interval=\n" " [ --table= ]\n" - " [ --modprobe= ]\n", name); + " [ --modprobe= ]\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; -- cgit v1.2.3