Commit 04ed90e2 authored by Jussi Laakkonen's avatar Jussi Laakkonen

[connman] Do not disable firewall context if one rule fails. JB#43924

After the changes to simply ignore invalid rules by handling iptables
errors with setjmp()/longjmp() it is no longer necessary to discard the
whole firewall context if a single rule fails. This commit adds more
information to logs and simply ignores errors while enabling firewall
contexts.

When removing all rules empty rule list should not give errors.

Updated test-firewall.c to comply with the changes made for firewall.c.
parent 62e0ea68
......@@ -443,19 +443,26 @@ static int firewall_enable_rule(struct fw_rule *rule)
rule->rule_spec);
if (err < 0) {
DBG("cannot insert managed rule %d", err);
return err;
goto err;
}
err = __connman_iptables_commit(rule->type, rule->table);
if (err < 0) {
DBG("iptables commit failed %d", err);
return err;
goto err;
}
rule->enabled = true;
return 0;
err:
connman_warn("failed to add rule to iptables: id: %d type: %d "
"table: %s chain: %s interface: %s rule: %s",
rule->id, rule->type, rule->table, rule->chain,
rule->ifname, rule->rule_spec);
return err;
}
static int firewall_disable_rule(struct fw_rule *rule)
......@@ -581,6 +588,10 @@ int __connman_firewall_remove_rule(struct firewall_context *ctx, int id)
list = prev;
}
/* An empty list of rules is not an error if all rules are removed */
if (id == FW_ALL_RULES && !g_list_length(ctx->rules))
return 0;
return err;
}
......@@ -597,6 +608,7 @@ int __connman_firewall_enable_rule(struct firewall_context *ctx, int id)
int e;
int err = -ENOENT;
int count = 0;
int invalid = 0;
for (list = g_list_first(ctx->rules); list; list = g_list_next(list)) {
rule = list->data;
......@@ -605,10 +617,12 @@ int __connman_firewall_enable_rule(struct firewall_context *ctx, int id)
e = firewall_enable_rule(rule);
/* Do not stop if enabling all rules */
if (e == 0 && err == -ENOENT)
if (e == 0 && err == -ENOENT) {
err = 0;
else if (e < 0)
} else if (e < 0) {
err = e;
invalid++;
}
if (id != FW_ALL_RULES)
break;
......@@ -617,8 +631,9 @@ int __connman_firewall_enable_rule(struct firewall_context *ctx, int id)
count++;
}
if (!err && id == FW_ALL_RULES) {
DBG("firewall enabled");
/* Invalid rules are ignored, just report errors */
if (id == FW_ALL_RULES) {
DBG("firewall enabled, invalid rules: %d", invalid);
ctx->enabled = true;
}
......@@ -650,7 +665,8 @@ int __connman_firewall_disable_rule(struct firewall_context *ctx, int id)
}
}
if (!err && id == FW_ALL_RULES) {
/* An empty list of rules is not an error */
if ((!err || !g_list_length(ctx->rules)) && id == FW_ALL_RULES) {
DBG("firewall disabled");
ctx->enabled = false;
}
......@@ -662,13 +678,11 @@ int __connman_firewall_enable(struct firewall_context *ctx)
{
int err;
/* Invalid rules are ignored, just report that there were errors */
err = __connman_firewall_enable_rule(ctx, FW_ALL_RULES);
if (err < 0) {
connman_warn("Failed to install iptables rules: %s",
strerror(-err));
__connman_firewall_disable_rule(ctx, FW_ALL_RULES);
return err;
}
if (err < 0)
connman_warn("Failed to install some of the iptables rules. "
"Last error: %s", strerror(-err));
firewall_is_up = true;
......@@ -2645,6 +2659,8 @@ static int init_all_dynamic_firewall_rules(void)
out:
err = enable_general_firewall();
if (err)
DBG("problem enabling");
return err;
}
......
......@@ -878,6 +878,9 @@ int __connman_iptables_append(int type,
if (global_iptables_type & IPTABLES_ADD_FAIL)
return -EINVAL;
if (global_iptables_type & IPTABLES_COMMIT_FAIL)
return 0;
DBG("list sizes IPv4: %d IPv6: %d", g_slist_length(rules_ipv4),
g_slist_length(rules_ipv6));
......@@ -915,6 +918,9 @@ int __connman_iptables_insert(int type,
if (global_iptables_type & IPTABLES_INS_FAIL)
return -EINVAL;
if (global_iptables_type & IPTABLES_COMMIT_FAIL)
return 0;
DBG("list sizes IPv4: %d IPv6: %d", g_slist_length(rules_ipv4),
g_slist_length(rules_ipv6));
......@@ -949,10 +955,13 @@ int __connman_iptables_delete(int type,
if (!table_exists(type, table_name))
return -EINVAL;
if (global_iptables_type & IPTABLES_DEL_FAIL)
return -EINVAL;
if (global_iptables_type & IPTABLES_COMMIT_FAIL)
return 0;
DBG("list sizes IPv4: %d IPv6: %d", g_slist_length(rules_ipv4),
g_slist_length(rules_ipv6));
......@@ -1184,8 +1193,10 @@ gboolean g_file_get_contents(const gchar *filename, gchar **contents,
return TRUE;
}
#define RULES_GEN4 48
#define RULES_GEN6 50
#define CHAINS_GEN4 3
#define RULES_GEN4 (CHAINS_GEN4 + 45)
#define CHAINS_GEN6 3
#define RULES_GEN6 (CHAINS_GEN6 + 47)
#define RULES_ETH 14
#define RULES_CEL 4
#define RULES_TETH 7
......@@ -2084,11 +2095,11 @@ static void firewall_test_basic0()
g_assert(ctx);
g_assert(__connman_firewall_enable(ctx) == -ENOENT);
g_assert_cmpint(__connman_firewall_enable(ctx), ==, 0);
g_assert(__connman_firewall_is_up());
g_assert(__connman_firewall_disable(ctx) == -ENOENT);
g_assert_cmpint(__connman_firewall_disable(ctx), ==, 0);
__connman_firewall_pre_cleanup();
__connman_firewall_cleanup();
......@@ -3663,30 +3674,35 @@ static void firewall_test_iptables_fail2()
const char **device_rules[] = { eth_input, NULL, eth_output };
setup_test_params(CONFIG_OK|CONFIG_ALL);
setup_iptables_params(IPTABLES_ADD_FAIL); // General rules are not added
/*
* General rules are not added, only the managed chains because
* they are added using __connman_iptables_insert()
*/
setup_iptables_params(IPTABLES_ADD_FAIL);
__connman_iptables_init();
__connman_firewall_init();
g_assert_cmpint(g_slist_length(rules_ipv4), ==, 0);
g_assert_cmpint(g_slist_length(rules_ipv6), ==, 0);
g_assert_cmpint(g_slist_length(rules_ipv4), ==, CHAINS_GEN4);
g_assert_cmpint(g_slist_length(rules_ipv6), ==, CHAINS_GEN6);
test_service.state = CONNMAN_SERVICE_STATE_CONFIGURATION;
service_state_change(&test_service, CONNMAN_SERVICE_STATE_READY);
g_assert_cmpint(g_slist_length(rules_ipv4), ==, RULES_ETH + RULES_ETH_ADD1 +
RULES_ETH_ADD3);
g_assert_cmpint(g_slist_length(rules_ipv6), ==, RULES_ETH + RULES_ETH_ADD1 +
RULES_ETH_ADD3);
g_assert_cmpint(g_slist_length(rules_ipv4), ==, CHAINS_GEN4 +
RULES_ETH + RULES_ETH_ADD1 + RULES_ETH_ADD3);
g_assert_cmpint(g_slist_length(rules_ipv6), ==, CHAINS_GEN4 +
RULES_ETH + RULES_ETH_ADD1 + RULES_ETH_ADD3);
ifname = connman_service_get_interface(&test_service);
check_rules(assert_rule_exists, device_rules, ifname);
service_state_change(&test_service, CONNMAN_SERVICE_STATE_DISCONNECT);
g_assert_cmpint(g_slist_length(rules_ipv4), ==, 0);
g_assert_cmpint(g_slist_length(rules_ipv6), ==, 0);
g_assert_cmpint(g_slist_length(rules_ipv4), ==, CHAINS_GEN4);
g_assert_cmpint(g_slist_length(rules_ipv6), ==, CHAINS_GEN6);
check_rules(assert_rule_not_exists, device_rules, ifname);
......@@ -3709,7 +3725,12 @@ static void firewall_test_iptables_fail3()
const char **device_rules[] = { eth_input, NULL, eth_output };
setup_test_params(CONFIG_OK|CONFIG_ALL);
setup_iptables_params(IPTABLES_INS_FAIL); // Managed chain fails
/*
* Managed chains also fail as they are added with
* __connman_iptables_insert().
*/
setup_iptables_params(IPTABLES_INS_FAIL);
__connman_iptables_init();
__connman_firewall_init();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment