From patchwork Wed Mar 15 13:45:53 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lorenzo Colitti X-Patchwork-Id: 739234 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3vjtDx14qXz9ryr for ; Thu, 16 Mar 2017 00:46:41 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="iQF46oGI"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752315AbdCONqk (ORCPT ); Wed, 15 Mar 2017 09:46:40 -0400 Received: from mail-pg0-f50.google.com ([74.125.83.50]:33478 "EHLO mail-pg0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751642AbdCONqO (ORCPT ); Wed, 15 Mar 2017 09:46:14 -0400 Received: by mail-pg0-f50.google.com with SMTP id n190so9494422pga.0 for ; Wed, 15 Mar 2017 06:46:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=vMUEXyOsQ3phkg5ojJMHZeWSVY6vL+OOEuxgRnMuEtc=; b=iQF46oGIJoet6IhXR6QowHVq2qH0x/j2DfD++jfAZX8kc0yD4qgATlpYLAZEfl/Mjc LiyPqEfAnthoo9rIcVBjv6ipMoxsQxuPmW+HV/xAF6W6vU5yXWIQpNjLh/5lx9Yovayn Bb5AjsF5MGpOjplt0JFTFnIGygvu1Fu/qzctz6fzBL5fuBKq6L+pqIJGemN8nWdK9+US hxgmqhq7R6P9rZJbYWO/REzt+1+hUV82VpRG3B+3hGoyxWGPkPkSwzcMCI5+xr/AWJdU Nk9hXwN1uHDKX+IZPynoWGz36xIKJM3JO+rypWn1vFeH7SkHEJZT9G5X3S3eTWuk8IKz slBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=vMUEXyOsQ3phkg5ojJMHZeWSVY6vL+OOEuxgRnMuEtc=; b=rbN4l48caeaHQSCsM2m07UTnj4mUzXSFpbiaWJg37qurXTyuZgWklRy+cdlD/5XXur uaKq0/qC1W0jTES/PmZNi8B2cX9+o5NB2dVJsPtNRYCBn2V1GpEvznumShvmnKyAC+11 VHTF3J8NDKs0vMBar7LZA5S7/LP4GsAjxOB74HPoaxvHyjrtLHJH9P73mCd9dO5/O9mP mrghvLE8MOBGTHhNz+/Zru8wQK9SDgI/iygPlAdTpOMAYTrU2QeCCo0xwU8Qxa2DMOFY 0SOVDHJKy1dEq06ax/qQvc9fosPJQkI0vtKwp0DyCqNynU4d5myRD0IbUeDlhwNT+z7l /syA== X-Gm-Message-State: AFeK/H3PjkLSsNWgkcdR2kiVyDYZFA5ks4RJuWoV6tC1QF3XKel+buazKlTw4IO7o2m6fK78 X-Received: by 10.99.117.11 with SMTP id q11mr3884723pgc.9.1489585572697; Wed, 15 Mar 2017 06:46:12 -0700 (PDT) Received: from lorenzo.tok.corp.google.com ([100.103.3.232]) by smtp.gmail.com with ESMTPSA id 129sm4652967pgj.59.2017.03.15.06.46.10 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 15 Mar 2017 06:46:11 -0700 (PDT) From: Lorenzo Colitti To: netfilter-devel@vger.kernel.org Cc: pablo@netfilter.org, jscherpelz@google.com, subashab@codeaurora.org, zlpnobody@gmail.com, Lorenzo Colitti , Narayan Kamath Subject: [PATCH iptables 2/2] iptables-restore: support acquiring the lock. Date: Wed, 15 Mar 2017 22:45:53 +0900 Message-Id: <20170315134553.35772-3-lorenzo@google.com> X-Mailer: git-send-email 2.12.0.367.g23dc2f6d3c-goog In-Reply-To: <20170315134553.35772-1-lorenzo@google.com> References: <20170315134553.35772-1-lorenzo@google.com> Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org 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. Signed-off-by: Narayan Kamath Signed-off-by: Lorenzo Colitti --- iptables/ip6tables-restore.c | 55 ++++++++++++++++++++++++++++++++++---------- iptables/ip6tables.c | 2 +- iptables/iptables-restore.c | 55 ++++++++++++++++++++++++++++++++++---------- iptables/iptables.c | 2 +- iptables/xshared.c | 18 ++++++++++----- iptables/xshared.h | 23 +++++++++++++++++- 6 files changed, 122 insertions(+), 33 deletions(-) diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c index dc0acb05a4..8a47f09c95 100644 --- a/iptables/ip6tables-restore.c +++ b/iptables/ip6tables-restore.c @@ -15,6 +15,7 @@ #include #include #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=\n" + " [ --wait-interval=\n" " [ --table= ]\n" - " [ --modprobe= ]\n", name); + " [ --modprobe= ]\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 4d77721b04..579d347b09 100644 --- 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 2f1522db52..7bb06d84b1 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; diff --git a/iptables/iptables.c b/iptables/iptables.c index 04be5abb10..62731c5ebb 100644 --- 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 dd5f8be028..5a7fcc0046 100644 --- a/iptables/xshared.c +++ b/iptables/xshared.c @@ -245,7 +245,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; @@ -255,22 +255,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; " @@ -284,6 +284,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 d8a697ae66..539e6c243b 100644 --- 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);