[PATCH iptables] xshared: Implement xtables lock timeout using signals

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Previously, if a lock timeout is specified using `-w`, 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` *with* a timeout. This is because invocations
using `-w` are actively waiting on the lock whereas the others only
check from time to time whether the lock is free, which will never be
the case.

This patch removes the `-W` option and the sleep loop. 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@xxxxxxxxxxxx>
---
 iptables/ip6tables.c                          |  7 +--
 iptables/iptables-restore.8.in                |  7 ---
 iptables/iptables-restore.c                   | 13 ++--
 iptables/iptables.8.in                        |  7 ---
 iptables/iptables.c                           |  7 +--
 .../testcases/ipt-restore/0002-parameters_0   |  3 +-
 iptables/xshared.c                            | 61 ++++++++-----------
 iptables/xshared.h                            |  5 +-
 8 files changed, 37 insertions(+), 73 deletions(-)

diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index b4604f83..46059785 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -725,9 +725,6 @@ int do_command6(int argc, char *argv[], char **table,
 
 	int verbose = 0;
 	int wait = 0;
-	struct timeval wait_interval = {
-		.tv_sec	= 1,
-	};
 	bool wait_interval_set = false;
 	const char *chain = NULL;
 	const char *shostnetworkmask = NULL, *dhostnetworkmask = NULL;
@@ -994,7 +991,7 @@ int do_command6(int argc, char *argv[], char **table,
 					      "You cannot use `-W' from "
 					      "ip6tables-restore");
 			}
-			parse_wait_interval(argc, argv, &wait_interval);
+			parse_wait_interval(argc, argv);
 			wait_interval_set = true;
 			break;
 
@@ -1162,7 +1159,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)
diff --git a/iptables/iptables-restore.8.in b/iptables/iptables-restore.8.in
index b4b62f92..e6144c75 100644
--- a/iptables/iptables-restore.8.in
+++ b/iptables/iptables-restore.8.in
@@ -66,13 +66,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.
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index a3efb067..5b238d3e 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -22,10 +22,6 @@
 
 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;
diff --git a/iptables/iptables.8.in b/iptables/iptables.8.in
index 759ec54f..99252884 100644
--- a/iptables/iptables.8.in
+++ b/iptables/iptables.8.in
@@ -373,13 +373,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.
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 7dc4cbc1..ab0a417a 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -707,9 +707,6 @@ int do_command4(int argc, char *argv[], char **table,
 	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,
-	};
 	bool wait_interval_set = false;
 	int verbose = 0;
 	int wait = 0;
@@ -975,7 +972,7 @@ int do_command4(int argc, char *argv[], char **table,
 					      "You cannot use `-W' from "
 					      "iptables-restore");
 			}
-			parse_wait_interval(argc, argv, &wait_interval);
+			parse_wait_interval(argc, argv);
 			wait_interval_set = true;
 			break;
 
@@ -1140,7 +1137,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)
diff --git a/iptables/tests/shell/testcases/ipt-restore/0002-parameters_0 b/iptables/tests/shell/testcases/ipt-restore/0002-parameters_0
index 5c8748ec..d632cbc0 100755
--- a/iptables/tests/shell/testcases/ipt-restore/0002-parameters_0
+++ b/iptables/tests/shell/testcases/ipt-restore/0002-parameters_0
@@ -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
diff --git a/iptables/xshared.c b/iptables/xshared.c
index efee7a30..c727a301 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -13,11 +13,11 @@
 #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");
 
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 2c05b0d7..a524622e 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -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[]);
-- 
2.25.1




[Index of Archives]     [Netfitler Users]     [Berkeley Packet Filter]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux