diff mbox series

iptables-restore: free the table lock when skipping a table

Message ID 43c70bd9e925dc4f6e1a45f05c718625463ba660.camel@jgoguen.ca
State Accepted
Delegated to: Pablo Neira
Headers show
Series iptables-restore: free the table lock when skipping a table | expand

Commit Message

Joel Goguen July 11, 2018, 11:32 p.m. UTC 
Currently, when running `iptables-restore --table=X`, where `X` is not the first
table in the rules dump, the restore will fail when parsing the second table:

- a lock is acquird when parsing the first table name
- the table name does not match the parameter to `--table` so processing
continues until the next table
- when processing the next table a lock is acquired, which fails because a lock
is already held

This will release the lock as soon as it's decided the current table won't be
used.

With existing code:

    # iptables -L
    Chain INPUT (policy ACCEPT)
    target     prot opt source               destination

    Chain FORWARD (policy ACCEPT)
    target     prot opt source               destination

    Chain OUTPUT (policy ACCEPT)
    target     prot opt source               destination

    # iptables-restore <iptables.dump
    Another app is currently holding the xtables lock. Perhaps you want to use
the -w option?

    # iptables -L
    Chain INPUT (policy ACCEPT)
    target     prot opt source               destination

    Chain FORWARD (policy ACCEPT)
    target     prot opt source               destination

    Chain OUTPUT (policy ACCEPT)
    target     prot opt source               destination

With this change:
    # iptables -L
    Chain INPUT (policy ACCEPT)
    target     prot opt source               destination

    Chain FORWARD (policy ACCEPT)
    target     prot opt source               destination

    Chain OUTPUT (policy ACCEPT)
    target     prot opt source               destination

    # iptables-restore <iptables.dump

    # iptables -L
    Chain INPUT (policy DROP)
    target     prot opt source               destination
    ACCEPT     all  --  anywhere             anywhere
    ACCEPT     all  --  anywhere             anywhere             state
RELATED,ESTABLISHED
    ACCEPT     icmp --  anywhere             anywhere

    Chain FORWARD (policy DROP)
    target     prot opt source               destination

    Chain OUTPUT (policy ACCEPT)
    target     prot opt source               destination
    REJECT     tcp  --  anywhere             anywhere             tcp
dpt:netbios-ns reject-with icmp-port-unreachable
    REJECT     udp  --  anywhere             anywhere             udp
dpt:netbios-ns reject-with icmp-port-unreachable

And the test suite:
    # ./iptables/tests/shell/run-tests.sh

    I: [OK]          ././iptables/tests/shell/testcases/chain/0001duplicate_1
    I: [OK]          ././iptables/tests/shell/testcases/chain/0002newchain_0
    I: [OK]          ././iptables/tests/shell/testcases/chain/0003rename_1
    I: [OK]          ././iptables/tests/shell/testcases/ebtables/0001-ebtables-
basic_0
    I: [OK]          ././iptables/tests/shell/testcases/firewalld-restore/0001-
firewalld_0
    I: [OK]          ././iptables/tests/shell/testcases/firewalld-restore/0002-
firewalld-restart_0
    I: [OK]          ././iptables/tests/shell/testcases/ipt-restore/0001load-
specific-table_0
    I: [OK]          ././iptables/tests/shell/testcases/ipt-save/0001load-
dumps_0
    I: [OK]          ././iptables/tests/shell/testcases/ipt-save/0002load-
fedora27-firewalld_0
    I: legacy results: [OK] 9 [FAILED] 0 [TOTAL] 9
    I: [OK]          ././iptables/tests/shell/testcases/chain/0001duplicate_1
    I: [OK]          ././iptables/tests/shell/testcases/chain/0002newchain_0
    I: [OK]          ././iptables/tests/shell/testcases/chain/0003rename_1
    I: [OK]          ././iptables/tests/shell/testcases/ebtables/0001-ebtables-
basic_0
    I: [OK]          ././iptables/tests/shell/testcases/firewalld-restore/0001-
firewalld_0
    I: [OK]          ././iptables/tests/shell/testcases/firewalld-restore/0002-
firewalld-restart_0
    I: [OK]          ././iptables/tests/shell/testcases/ipt-restore/0001load-
specific-table_0
    I: [OK]          ././iptables/tests/shell/testcases/ipt-save/0001load-
dumps_0
    I: [OK]          ././iptables/tests/shell/testcases/ipt-save/0002load-
fedora27-firewalld_0
    I: nft results: [OK] 9 [FAILED] 0 [TOTAL] 9
    I: combined results: [OK] 18 [FAILED] 0 [TOTAL] 18

Signed-off-by: Joel Goguen <contact+netfilter@jgoguen.ca>
---
 iptables/ip6tables-restore.c                       |  7 +++-
 iptables/iptables-restore.c                        |  9 +++--
 .../ipt-restore/0001load-specific-table_0          | 42 ++++++++++++++++++++++
 .../testcases/ipt-restore/dumps/ip6tables.dump     | 30 ++++++++++++++++
 .../testcases/ipt-restore/dumps/iptables.dump      | 30 ++++++++++++++++
 5 files changed, 115 insertions(+), 3 deletions(-)

unreachable
+COMMIT
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Florian Westphal July 26, 2018, 1:08 p.m. UTC | #1 
Joel Goguen <contact+netfilter@jgoguen.ca> wrote:
> Currently, when running `iptables-restore --table=X`, where `X` is not the first
> table in the rules dump, the restore will fail when parsing the second table:

Applied, thanks for your patience.

> And the test suite:
>     # ./iptables/tests/shell/run-tests.sh

Thanks for including a test case!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joel Goguen July 26, 2018, 2:19 p.m. UTC | #2 
On Thu, 2018-07-26 at 15:08 +0200, Florian Westphal wrote:
> Applied, thanks for your patience.

I also opened https://bugzilla.netfilter.org/show_bug.cgi?id=1271 because I
wasn't sure if it was needed in addition to sending the patch here. The bug
report has the same patch and tests.
diff mbox series

Patch

diff --git iptables/ip6tables-restore.c iptables/ip6tables-restore.c
index ceffa616..a880fae5 100644
--- iptables/ip6tables-restore.c
+++ iptables/ip6tables-restore.c
@@ -325,8 +325,13 @@  int ip6tables_restore_main(int argc, char *argv[])
 			strncpy(curtable, table, XT_TABLE_MAXNAMELEN);
 			curtable[XT_TABLE_MAXNAMELEN] = '\0';
 
-			if (tablename != NULL && strcmp(tablename, table) != 0)
+			if (tablename != NULL && strcmp(tablename, table) != 0)
{
+				if (lock >= 0) {
+					xtables_unlock(lock);
+					lock = XT_LOCK_NOT_ACQUIRED;
+				}
 				continue;
+			}
 			if (handle)
 				ops->free(handle);
 
diff --git iptables/iptables-restore.c iptables/iptables-restore.c
index 39198752..190a990f 100644
--- iptables/iptables-restore.c
+++ iptables/iptables-restore.c
@@ -97,7 +97,7 @@  static int parse_counters(char *string, struct xt_counters
*ctr)
 static char *newargv[255];
 static int newargc;
 
-/* function adding one argument to newargv, updating newargc 
+/* function adding one argument to newargv, updating newargc
  * returns true if argument added, false otherwise */
 static int add_argv(char *what) {
 	DEBUGP("add_argv: %s\n", what);
@@ -323,8 +323,13 @@  iptables_restore_main(int argc, char *argv[])
 			strncpy(curtable, table, XT_TABLE_MAXNAMELEN);
 			curtable[XT_TABLE_MAXNAMELEN] = '\0';
 
-			if (tablename && (strcmp(tablename, table) != 0))
+			if (tablename && (strcmp(tablename, table) != 0)) {
+				if (lock >= 0) {
+					xtables_unlock(lock);
+					lock = XT_LOCK_NOT_ACQUIRED;
+				}
 				continue;
+			}
 			if (handle)
 				ops->free(handle);
 
diff --git iptables/tests/shell/testcases/ipt-restore/0001load-specific-table_0
iptables/tests/shell/testcases/ipt-restore/0001load-specific-table_0
new file mode 100755
index 00000000..56d2d966
--- /dev/null
+++ iptables/tests/shell/testcases/ipt-restore/0001load-specific-table_0
@@ -0,0 +1,42 @@ 
+#!/bin/bash
+# vim: syntax=sh:noexpandtab:sw=4:ts=4:sts=4
+
+RET=0
+tmpfile=""
+
+set -x
+
+clean_tempfile()
+{
+	if [ -n "${tmpfile}" ]; then
+		rm -f "${tmpfile}"
+	fi
+}
+
+trap clean_tempfile EXIT
+
+tmpfile=$(mktemp) || exit 1
+
+do_simple()
+{
+	iptables="${1}"
+	table="${2}"
+	dumpfile="$(dirname "${0}")/dumps/${iptables}.dump"
+
+	"$XT_MULTI" "${iptables}-restore" --table="${table}" <"${dumpfile}";
rv=$?
+
+	if [ "${rv}" -ne 0 ]; then
+		RET=1
+	fi
+}
+
+do_simple "iptables" "filter"
+do_simple "iptables" "mangle"
+do_simple "iptables" "raw"
+do_simple "iptables" "nat"
+do_simple "ip6tables" "filter"
+do_simple "ip6tables" "mangle"
+do_simple "ip6tables" "raw"
+do_simple "ip6tables" "nat"
+
+exit "${RET}"
diff --git iptables/tests/shell/testcases/ipt-restore/dumps/ip6tables.dump
iptables/tests/shell/testcases/ipt-restore/dumps/ip6tables.dump
new file mode 100644
index 00000000..4ac4f882
--- /dev/null
+++ iptables/tests/shell/testcases/ipt-restore/dumps/ip6tables.dump
@@ -0,0 +1,30 @@ 
+*nat
+:PREROUTING ACCEPT [0:0]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [8:656]
+:POSTROUTING ACCEPT [8:656]
+COMMIT
+
+*mangle
+:PREROUTING ACCEPT [794:190738]
+:INPUT ACCEPT [794:190738]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [991:170303]
+:POSTROUTING ACCEPT [991:170303]
+COMMIT
+
+*raw
+:PREROUTING ACCEPT [794:190738]
+:OUTPUT ACCEPT [991:170303]
+COMMIT
+
+*filter
+:INPUT DROP [0:0]
+:FORWARD DROP [0:0]
+:OUTPUT ACCEPT [991:170303]
+-A INPUT -i lo -j ACCEPT
+-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT
+-A INPUT -p ipv6-icmp -j ACCEPT
+-A OUTPUT -p tcp -m tcp --dport 137 -j REJECT --reject-with icmp6-port-
unreachable
+-A OUTPUT -p udp -m udp --dport 137 -j REJECT --reject-with icmp6-port-
unreachable
+COMMIT
diff --git iptables/tests/shell/testcases/ipt-restore/dumps/iptables.dump
iptables/tests/shell/testcases/ipt-restore/dumps/iptables.dump
new file mode 100644
index 00000000..6e4e42d3
--- /dev/null
+++ iptables/tests/shell/testcases/ipt-restore/dumps/iptables.dump
@@ -0,0 +1,30 @@ 
+*nat
+:PREROUTING ACCEPT [1:89]
+:INPUT ACCEPT [0:0]
+:OUTPUT ACCEPT [351:24945]
+:POSTROUTING ACCEPT [351:24945]
+COMMIT
+
+*mangle
+:PREROUTING ACCEPT [3270:1513114]
+:INPUT ACCEPT [3270:1513114]
+:FORWARD ACCEPT [0:0]
+:OUTPUT ACCEPT [3528:1087907]
+:POSTROUTING ACCEPT [3546:1090751]
+COMMIT
+
+*raw
+:PREROUTING ACCEPT [3270:1513114]
+:OUTPUT ACCEPT [3528:1087907]
+COMMIT
+
+*filter
+:INPUT DROP [37:4057]
+:FORWARD DROP [0:0]
+:OUTPUT ACCEPT [3528:1087907]
+-A INPUT -i lo -j ACCEPT
+-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT
+-A INPUT -p icmp -j ACCEPT
+-A OUTPUT -p tcp -m tcp --dport 137 -j REJECT --reject-with icmp-port-
unreachable
+-A OUTPUT -p udp -m udp --dport 137 -j REJECT --reject-with icmp-port-