Hi, in general I do consider this idea but I do not agree with the implementation in your patch.
In particular I think that the firewall init script is the wrong place
to unload kmods, actually I think a random service should never unload
kmods, at most only load them when needed for the first time.
We can certainly change the netfilter kmod packages to not autoload on
boot anymore, then insmod the required ones from fw3 (similar to how
iptables did it).
Furthermore the module dependencies should be calculated in the C code
directly, I do not like parsing the firewall config for a 2nd time in
shell, just to determine needed modules.
A couple of additional comments are inline below.
On 01.11.2013 06:35, Takayuki Kaiso wrote:
> Now all the firewall modules are loaded at boot time regardless of the user's
> firewall configuration.
> Then, performance overhead is so high for low cpu power boards (Ex. soekris
> net4826 with 266MHz)
> by this approach and should be better to load modules on demand base.
>
> This patch allow us to unload unnecessary modules especially for conntrack
> related ones.
> It can decrease the overhead for the config which does not enable NAT, for
> instance.
>
> Signed-off-by: Takayuki Kaiso <tka...@thinktube.com>
>
> ---
> package/firewall/files/firewall.init | 44 +++++++++
> .../patches/010-remove-unnecessary-chain.patch | 101 +++++++++++++++++++++
> 2 files changed, 145 insertions(+)
>
> diff --git a/package/firewall/files/firewall.init
> b/package/firewall/files/firewall.init
> index 64e3a8c..6eda74f 100755
> --- a/package/firewall/files/firewall.init
> +++ b/package/firewall/files/firewall.init
> @@ -2,24 +2,68 @@
>
> START=19
>
> +CONNTRACK_ZONES=
> +CONNTRACK_MODS="nf_nat_irc nf_conntrack_irc nf_nat_ftp nf_conntrack_ftp
> ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4
> nf_defrag_ipv4
> +xt_conntrack"
There are more than just those, e.g. in the conntrack-extra packages, a
more generic solution is needed here.
> +CONNTRACK_MODS_REVERSE=
> +
> +fw_zone_check_conntrack() {
> + config_get name $1 name
> + config_get_bool masq $1 masq "0"
> + config_get_bool conntrack $1 conntrack "0"
> + [ "$conntrack" = "1" -o "$masq" = "1" ] && append CONNTRACK_ZONES "$name"
Port forwards imply conntracking on a zone as well, this is not covered
here.
> +}
> +
> +load_conntrack_modules() {
> + for i in $CONNTRACK_MODS; do
> + CONNTRACK_MODS_REVERSE="$i $CONNTRACK_MODS_REVERSE"
> + done
> + for i in $CONNTRACK_MODS_REVERSE; do
> + insmod $i 2>/dev/null;
> + done
> +}
> +
> +unload_conntrack_modules() {
> + for i in $CONNTRACK_MODS; do
> + rmmod $i 2>/dev/null;
> + done
> +}
> +
> +update_firewall_modules() {
> + config_load firewall
> + config_foreach fw_zone_check_conntrack zone
> + if [ -n "$CONNTRACK_ZONES" ]; then
> + echo "loading conntrack modules"
> + load_conntrack_modules
> + else
> + echo "Unloading conntrack modules"
> + unload_conntrack_modules
> + fi
> +}
> +
> boot() {
> # Be silent on boot, firewall might be started by hotplug already,
> # so don't complain in syslog.
> fw3 -q start
> + update_firewall_modules
Shouldn't that be done before starting the firewall?
> }
>
> start() {
> fw3 start
> + update_firewall_modules
Dito.
> }
>
> stop() {
> fw3 flush
> + update_firewall_modules
After a flush, the complete firewall is empty, one could argue that the
principle of least surprise is violated here by *loading* kmods under
certain circumstances.
> }
>
> restart() {
> fw3 restart
> + update_firewall_modules
> }
>
> reload() {
> fw3 reload
> + update_firewall_modules
Reload does not clear the entire ruleset, it simply rebuilds internal
chains. There is a high probability of live rules still relying on
conntrack modules.
> }
> diff --git a/package/firewall/patches/010-remove-unnecessary-chain.patch
> b/package/firewall/patches/010-remove-unnecessary-chain.patch
> new file mode 100644
> index 0000000..a2991e9
> --- /dev/null
> +++ b/package/firewall/patches/010-remove-unnecessary-chain.patch
> @@ -0,0 +1,101 @@
> +--- a/defaults.c
> ++++ b/defaults.c
> +@@ -138,6 +138,9 @@ fw3_print_default_chains(struct fw3_ipt_
> + if (handle->family == FW3_FAMILY_V6 && defs->disable_ipv6)
> + return;
> +
> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
> ++ return;
> ++
> + if (handle->table == FW3_TABLE_FILTER)
> + {
> + fw3_ipt_set_policy(handle, "INPUT", policy(defs->policy_input));
> +@@ -213,6 +216,9 @@ fw3_print_default_head_rules(struct fw3_
> + { 0, NULL },
> + };
> +
> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
> ++ return;
> ++
> + for (tr = rules; tr->chain; tr++)
> + {
> + if (tr->table != handle->table)
> +@@ -250,10 +256,13 @@ fw3_print_default_head_rules(struct fw3_
> +
> + for (i = 0; i < ARRAY_SIZE(chains); i += 2)
> + {
> +- r = fw3_ipt_rule_new(handle);
> +- fw3_ipt_rule_extra(r, "-m conntrack --ctstate RELATED,ESTABLISHED");
> +- fw3_ipt_rule_target(r, "ACCEPT");
> +- fw3_ipt_rule_append(r, chains[i]);
> ++ if (state->enable_conntrack)
> ++ {
> ++ r = fw3_ipt_rule_new(handle);
> ++ fw3_ipt_rule_extra(r, "-m conntrack --ctstate RELATED,ESTABLISHED");
> ++ fw3_ipt_rule_target(r, "ACCEPT");
> ++ fw3_ipt_rule_append(r, chains[i]);
> ++ }
> +
> + if (defs->drop_invalid)
> + {
> +--- a/options.h
> ++++ b/options.h
> +@@ -461,6 +461,8 @@ struct fw3_state
> +
> + bool disable_ipsets;
> + bool statefile;
> ++ bool enable_nat_table;
> ++ bool enable_conntrack;
> + };
> +
> + struct fw3_chain_spec {
> +--- a/zones.c
> ++++ b/zones.c
> +@@ -213,6 +213,13 @@ fw3_load_zones(struct fw3_state *state,
> + {
> + setbit(zone->flags[0], FW3_FLAG_SNAT);
> + zone->conntrack = true;
> ++ state->enable_nat_table = true;
> ++ state->enable_conntrack = true;
This...
> ++ }
> ++
> ++ if (zone->conntrack)
> ++ {
> ++ state->enable_conntrack = true;
... and this is redundant, only the 2nd enable_conntrack assignment
seems to be needed.
> + }
> +
> + if (zone->custom_chains)
> +@@ -256,6 +263,9 @@ print_zone_chain(struct fw3_ipt_handle *
> + if (!fw3_is_family(zone, handle->family))
> + return;
> +
> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
> ++ return;
> ++
> + info(" * Zone '%s'", zone->name);
> +
> + set(zone->flags, handle->family, handle->table);
> +@@ -332,6 +342,9 @@ print_interface_rule(struct fw3_ipt_hand
> + "forward",
> + };
> +
> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
> ++ return;
> ++
> + #define jump_target(t) \
> + ((t == FW3_FLAG_REJECT) ? "reject" : fw3_flag_names[t])
> +
> +--- a/main.c
> ++++ b/main.c
> +@@ -50,6 +50,11 @@ build_state(bool runtime)
> + error("Out of memory");
> +
> + memset(state, 0, sizeof(*state));
> ++
> ++ /* set default status */
> ++ state->enable_nat_table = false;
> ++ state->enable_conntrack = false;
This is redundant and defaults to false anyway due to the memset before.
> ++
> + state->uci = uci_alloc_context();
> +
> + if (!state->uci)
You also need to enable support for the nat table if at least one
redirect is referencing the zone.
Regards,
Jow
signature.asc
Description: OpenPGP digital signature
_______________________________________________ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
