]> git.ipfire.org Git - thirdparty/iptables.git/commitdiff
xshared: Implement xtables lock timeout using signals
authorJethro Beekman <jethro@fortanix.com>
Mon, 14 Feb 2022 09:35:56 +0000 (10:35 +0100)
committerFlorian Westphal <fw@strlen.de>
Tue, 15 Feb 2022 22:42:05 +0000 (23:42 +0100)
Previously, if a lock timeout is specified using `-wN `, flock() is
called using LOCK_NB in a loop with a sleep. This results in two issues.

The first issue is that the process may wait longer than necessary when
the lock becomes available. For this the `-W` option was added, but this
requires fine-tuning.

The second issue is that if lock contention is high, invocations using
`-w` (without a timeout) will always win lock acquisition from
invocations that use `-w N`. This is because invocations using `-w` are
actively waiting on the lock whereas those using `-w N` only check from
time to time whether the lock is free, which will never be the case.

This patch removes the sleep loop and deprecates the `-W` option (making
it non-functional). Instead, flock() is always called in a blocking
fashion, but the alarm() function is used with a non-SA_RESTART signal
handler to cancel the system call.

Signed-off-by: Jethro Beekman <jethro@fortanix.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
iptables/ip6tables.c
iptables/iptables-restore.8.in
iptables/iptables-restore.c
iptables/iptables.8.in
iptables/iptables.c
iptables/tests/shell/testcases/ipt-restore/0002-parameters_0
iptables/xshared.c
iptables/xshared.h

index 560b6ed0d74b67a28b1d11674cfb60748d4acf20..f4796b897d74c7c602659776c05fa835e5c90329 100644 (file)
@@ -712,7 +712,6 @@ int do_command6(int argc, char *argv[], char **table,
        };
        struct xtables_args args = {
                .family = AF_INET6,
-               .wait_interval.tv_sec = 1,
        };
        struct ip6t_entry *e = NULL;
        unsigned int nsaddrs = 0, ndaddrs = 0;
@@ -721,9 +720,6 @@ int do_command6(int argc, char *argv[], char **table,
 
        int verbose = 0;
        int wait = 0;
-       struct timeval wait_interval = {
-               .tv_sec = 1,
-       };
        const char *chain = NULL;
        const char *policy = NULL, *newname = NULL;
        unsigned int rulenum = 0, command = 0;
@@ -739,7 +735,6 @@ int do_command6(int argc, char *argv[], char **table,
        newname         = p.newname;
        verbose         = p.verbose;
        wait            = args.wait;
-       wait_interval   = args.wait_interval;
        nsaddrs         = args.s.naddrs;
        ndaddrs         = args.d.naddrs;
        saddrs          = args.s.addr.v6;
@@ -749,7 +744,7 @@ int do_command6(int argc, char *argv[], char **table,
 
        /* Attempt to acquire the xtables lock */
        if (!restore)
-               xtables_lock_or_exit(wait, &wait_interval);
+               xtables_lock_or_exit(wait);
 
        /* only allocate handle if we weren't called with a handle */
        if (!*handle)
index 883da998b0f7eebacdf46cbd730b585114f9850b..20216842d8358c562e23951ab1cdbf70003ff468 100644 (file)
@@ -67,13 +67,6 @@ the program will exit if the lock cannot be obtained.  This option will
 make the program wait (indefinitely or for optional \fIseconds\fP) until
 the exclusive lock can be obtained.
 .TP
-\fB\-W\fP, \fB\-\-wait-interval\fP \fImicroseconds\fP
-Interval to wait per each iteration.
-When running latency sensitive applications, waiting for the xtables lock
-for extended durations may not be acceptable. This option will make each
-iteration take the amount of time specified. The default interval is
-1 second. This option only works with \fB\-w\fP.
-.TP
 \fB\-M\fP, \fB\-\-modprobe\fP \fImodprobe_program\fP
 Specify the path to the modprobe program. By default, iptables-restore will
 inspect /proc/sys/kernel/modprobe to determine the executable's path.
index 3c0a238917ecdac706aa971de9eb20715f496e2a..1917fb2315665a4385dee6a5b51ba06c70fbd641 100644 (file)
 
 static int counters, verbose, noflush, wait;
 
-static struct timeval wait_interval = {
-       .tv_sec = 1,
-};
-
 /* Keeping track of external matches and targets.  */
 static const struct option options[] = {
        {.name = "counters",      .has_arg = 0, .val = 'c'},
@@ -51,7 +47,6 @@ static void print_usage(const char *name, const char *version)
                        "          [ --help ]\n"
                        "          [ --noflush ]\n"
                        "          [ --wait=<seconds>\n"
-                       "          [ --wait-interval=<usecs>\n"
                        "          [ --table=<TABLE> ]\n"
                        "          [ --modprobe=<command> ]\n", name);
 }
@@ -101,6 +96,7 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
        FILE *in;
        int in_table = 0, testing = 0;
        const char *tablename = NULL;
+       bool wait_interval_set = false;
 
        line = 0;
        lock = XT_LOCK_NOT_ACQUIRED;
@@ -135,7 +131,8 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
                                wait = parse_wait_time(argc, argv);
                                break;
                        case 'W':
-                               parse_wait_interval(argc, argv, &wait_interval);
+                               parse_wait_interval(argc, argv);
+                               wait_interval_set = true;
                                break;
                        case 'M':
                                xtables_modprobe_program = optarg;
@@ -165,7 +162,7 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
        }
        else in = stdin;
 
-       if (!wait_interval.tv_sec && !wait) {
+       if (wait_interval_set && !wait) {
                fprintf(stderr, "Option --wait-interval requires option --wait\n");
                exit(1);
        }
@@ -203,7 +200,7 @@ ip46tables_restore_main(const struct iptables_restore_cb *cb,
                        in_table = 0;
                } else if ((buffer[0] == '*') && (!in_table)) {
                        /* Acquire a lock before we create a new table handle */
-                       lock = xtables_lock_or_exit(wait, &wait_interval);
+                       lock = xtables_lock_or_exit(wait);
 
                        /* New table */
                        char *table;
index ccc498f56def18c7e612c7347bb3ab6badc9b3ce..627ff0e4da7a4a324250cef3e922b91ed12e25c3 100644 (file)
@@ -377,13 +377,6 @@ the program will exit if the lock cannot be obtained.  This option will
 make the program wait (indefinitely or for optional \fIseconds\fP) until
 the exclusive lock can be obtained.
 .TP
-\fB\-W\fP, \fB\-\-wait-interval\fP \fImicroseconds\fP
-Interval to wait per each iteration.
-When running latency sensitive applications, waiting for the xtables lock
-for extended durations may not be acceptable. This option will make each
-iteration take the amount of time specified. The default interval is
-1 second. This option only works with \fB\-w\fP.
-.TP
 \fB\-n\fP, \fB\-\-numeric\fP
 Numeric output.
 IP addresses and port numbers will be printed in numeric format.
index f5fe868c777fc3dc8c7bc6719f33e73cecfa9004..ccebb1a6c7eac213f289c72ef852d9ed8b15582f 100644 (file)
@@ -706,15 +706,11 @@ int do_command4(int argc, char *argv[], char **table,
        };
        struct xtables_args args = {
                .family = AF_INET,
-               .wait_interval.tv_sec = 1,
        };
        struct ipt_entry *e = NULL;
        unsigned int nsaddrs = 0, ndaddrs = 0;
        struct in_addr *saddrs = NULL, *smasks = NULL;
        struct in_addr *daddrs = NULL, *dmasks = NULL;
-       struct timeval wait_interval = {
-               .tv_sec = 1,
-       };
        int verbose = 0;
        int wait = 0;
        const char *chain = NULL;
@@ -732,7 +728,6 @@ int do_command4(int argc, char *argv[], char **table,
        newname         = p.newname;
        verbose         = p.verbose;
        wait            = args.wait;
-       wait_interval   = args.wait_interval;
        nsaddrs         = args.s.naddrs;
        ndaddrs         = args.d.naddrs;
        saddrs          = args.s.addr.v4;
@@ -742,7 +737,7 @@ int do_command4(int argc, char *argv[], char **table,
 
        /* Attempt to acquire the xtables lock */
        if (!restore)
-               xtables_lock_or_exit(wait, &wait_interval);
+               xtables_lock_or_exit(wait);
 
        /* only allocate handle if we weren't called with a handle */
        if (!*handle)
index 5c8748ec765b2703e260772871a0eb440f39a2df..d632cbc0d6b9a169b7062f4c18cb47e09d134621 100755 (executable)
@@ -2,7 +2,7 @@
 
 set -e
 
-# make sure wait and wait-interval options are accepted
+# make sure wait options are accepted
 
 clean_tempfile()
 {
@@ -18,4 +18,3 @@ tmpfile=$(mktemp) || exit 1
 $XT_MULTI iptables-save -f $tmpfile
 $XT_MULTI iptables-restore $tmpfile
 $XT_MULTI iptables-restore -w 5 $tmpfile
-$XT_MULTI iptables-restore -w 5 -W 1 $tmpfile
index 1fd7acc953ed757e0126a0083f0b4475b826a7b8..50a1d48a55ebe777de95a907d63461d2aa518390 100644 (file)
 #include <sys/file.h>
 #include <sys/socket.h>
 #include <sys/un.h>
-#include <sys/time.h>
 #include <unistd.h>
 #include <fcntl.h>
 #include <xtables.h>
 #include <math.h>
+#include <signal.h>
 #include "xshared.h"
 
 /*
@@ -243,14 +243,14 @@ void xs_init_match(struct xtables_match *match)
                match->init(match->m);
 }
 
-static int xtables_lock(int wait, struct timeval *wait_interval)
+static void alarm_ignore(int i) {
+}
+
+static int xtables_lock(int wait)
 {
-       struct timeval time_left, wait_time;
+       struct sigaction sigact_alarm;
        const char *lock_file;
-       int fd, i = 0;
-
-       time_left.tv_sec = wait;
-       time_left.tv_usec = 0;
+       int fd;
 
        lock_file = getenv("XTABLES_LOCKFILE");
        if (lock_file == NULL || lock_file[0] == '\0')
@@ -263,31 +263,24 @@ static int xtables_lock(int wait, struct timeval *wait_interval)
                return XT_LOCK_FAILED;
        }
 
-       if (wait == -1) {
-               if (flock(fd, LOCK_EX) == 0)
-                       return fd;
-
-               fprintf(stderr, "Can't lock %s: %s\n", lock_file,
-                       strerror(errno));
-               return XT_LOCK_BUSY;
+       if (wait != -1) {
+               sigact_alarm.sa_handler = alarm_ignore;
+               sigact_alarm.sa_flags = SA_RESETHAND;
+               sigemptyset(&sigact_alarm.sa_mask);
+               sigaction(SIGALRM, &sigact_alarm, NULL);
+               alarm(wait);
        }
 
-       while (1) {
-               if (flock(fd, LOCK_EX | LOCK_NB) == 0)
-                       return fd;
-               else if (timercmp(&time_left, wait_interval, <))
-                       return XT_LOCK_BUSY;
+       if (flock(fd, LOCK_EX) == 0)
+               return fd;
 
-               if (++i % 10 == 0) {
-                       fprintf(stderr, "Another app is currently holding the xtables lock; "
-                               "still %lds %ldus time ahead to have a chance to grab the lock...\n",
-                               time_left.tv_sec, time_left.tv_usec);
-               }
-
-               wait_time = *wait_interval;
-               select(0, NULL, NULL, NULL, &wait_time);
-               timersub(&time_left, wait_interval, &time_left);
+       if (errno == EINTR) {
+               errno = EWOULDBLOCK;
        }
+
+       fprintf(stderr, "Can't lock %s: %s\n", lock_file,
+               strerror(errno));
+       return XT_LOCK_BUSY;
 }
 
 void xtables_unlock(int lock)
@@ -296,9 +289,9 @@ void xtables_unlock(int lock)
                close(lock);
 }
 
-int xtables_lock_or_exit(int wait, struct timeval *wait_interval)
+int xtables_lock_or_exit(int wait)
 {
-       int lock = xtables_lock(wait, wait_interval);
+       int lock = xtables_lock(wait);
 
        if (lock == XT_LOCK_FAILED) {
                xtables_free_opts(1);
@@ -334,7 +327,7 @@ int parse_wait_time(int argc, char *argv[])
        return wait;
 }
 
-void parse_wait_interval(int argc, char *argv[], struct timeval *wait_interval)
+void parse_wait_interval(int argc, char *argv[])
 {
        const char *arg;
        unsigned int usec;
@@ -354,8 +347,7 @@ void parse_wait_interval(int argc, char *argv[], struct timeval *wait_interval)
                                      "too long usec wait %u > 999999 usec",
                                      usec);
 
-               wait_interval->tv_sec = 0;
-               wait_interval->tv_usec = usec;
+               fprintf(stderr, "Ignoring deprecated --wait-interval option.\n");
                return;
        }
        xtables_error(PARAMETER_PROBLEM, "wait interval not numeric");
@@ -1235,9 +1227,6 @@ xtables_printhelp(const struct xtables_rule_match *matches)
 "  --table     -t table        table to manipulate (default: `filter')\n"
 "  --verbose   -v              verbose mode\n"
 "  --wait      -w [seconds]    maximum wait to acquire xtables lock before give up\n"
-"  --wait-interval -W [usecs]  wait time to try to acquire xtables lock\n"
-"                              interval to wait for xtables lock\n"
-"                              default is 1 second\n"
 "  --line-numbers              print line numbers when listing\n"
 "  --exact     -x              expand numbers (display exact values)\n");
 
@@ -1665,7 +1654,7 @@ void do_parse(int argc, char *argv[],
                                              "iptables-restore");
                        }
 
-                       parse_wait_interval(argc, argv, &args->wait_interval);
+                       parse_wait_interval(argc, argv);
                        wait_interval_set = true;
                        break;
 
index d13de95e7dff611c870e979545321b8406903f12..0de0e12e5a922b5eb1703844c41b56fa76d88558 100644 (file)
@@ -6,7 +6,6 @@
 #include <stdint.h>
 #include <netinet/in.h>
 #include <net/if.h>
-#include <sys/time.h>
 #include <linux/netfilter_arp/arp_tables.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv6/ip6_tables.h>
@@ -189,10 +188,10 @@ enum {
        XT_LOCK_NOT_ACQUIRED  = -3,
 };
 extern void xtables_unlock(int lock);
-extern int xtables_lock_or_exit(int wait, struct timeval *tv);
+extern int xtables_lock_or_exit(int wait);
 
 int parse_wait_time(int argc, char *argv[]);
-void parse_wait_interval(int argc, char *argv[], struct timeval *wait_interval);
+void parse_wait_interval(int argc, char *argv[]);
 int parse_counters(const char *string, struct xt_counters *ctr);
 bool tokenize_rule_counters(char **bufferp, char **pcnt, char **bcnt, int line);
 bool xs_has_arg(int argc, char *argv[]);
@@ -294,7 +293,6 @@ struct xtables_args {
        const char      *arp_htype, *arp_ptype;
        unsigned long long pcnt_cnt, bcnt_cnt;
        int             wait;
-       struct timeval  wait_interval;
 };
 
 struct xt_cmd_parse_ops {