On Wed, Jan 25, 2017 at 09:43:14PM +0100, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@xxxxxxx>
> Subject: [PATCH] netfilter: nf_ct_helper: warn when not applying default helper assignment
>
> Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
> assignment") is causing behavior regressions in firewalls, as traffic
> handled by conntrack helpers is now by default not passed through even
> though it was before due to missing CT targets (which were not necessary
> before this commit).
>
> The default had to be switched off due to security reasons [1] [2] and
> therefore should stay the way it is, but let's be friendly to firewall
> admins and issue a warning the first time we're in situation where packet
> would be likely passed through with the old default but we're likely going
> to drop it on the floor now.
Links to [1] [2] are now gone in the patch description.
> Rewrite the code a little bit as suggested by Linus, so that we avoid
> spaghettiing the code even more -- namely the whole decision making
> process regarding helper selection (either automatic or not) is being
> separated, so that the whole logic can be simplified and code (condition)
> duplication reduced.
>
> Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
> ---
> net/netfilter/nf_conntrack_helper.c | 58 +++++++++++++++++++++++++++----------
> 1 file changed, 42 insertions(+), 16 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
> index 7341adf..c93a331 100644
> --- a/net/netfilter/nf_conntrack_helper.c
> +++ b/net/netfilter/nf_conntrack_helper.c
> @@ -188,6 +188,39 @@ struct nf_conn_help *
> }
> EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
>
> +static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
> +{
> + return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> +}
> +
> +static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn *ct, struct net *net)
Please use the prefixes that we already have in place:
nf_ct_lookup_helper() probably?
> +{
> + struct nf_conntrack_helper *ret;
> +
> + if (!net->ct.sysctl_auto_assign_helper) {
> + if (net->ct.auto_assign_helper_warned)
> + return NULL;
> + if (!find_auto_helper(ct))
This fits in one line, so I think no need for find_auto_helper(), look:
if (!__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))
> + return NULL;
> + pr_info("nf_conntrack: default automatic helper assignment "
> + "has been turned off for security reasons and CT-based "
> + " firewall rule not found. Use the iptables CT target "
> + "to attach helpers instead.\n");
> + net->ct.auto_assign_helper_warned = 1;
> + return NULL;
> + }
> +
> + ret = find_auto_helper(ct);
> + if (!ret || net->ct.auto_assign_helper_warned)
> + return ret;
> + pr_info("nf_conntrack: automatic helper assignment is deprecated and it will "
> + "be removed soon. Use the iptables CT target to attach helpers "
> + " instead.\n");
You can probably remove this older message now if you prefer the new
one you wrote, actually it's a bit redundant IMO.
> + net->ct.auto_assign_helper_warned = 1;
> + return ret;
> +}
> +
> +
> int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
> gfp_t flags)
> {
> @@ -213,26 +246,19 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
> }
>
> help = nfct_help(ct);
> - if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
> - helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> - if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
> - pr_info("nf_conntrack: automatic helper "
> - "assignment is deprecated and it will "
> - "be removed soon. Use the iptables CT target "
> - "to attach helpers instead.\n");
> - net->ct.auto_assign_helper_warned = true;
> - }
> - }
>
> - if (helper == NULL) {
> - if (help)
> - RCU_INIT_POINTER(help->helper, NULL);
> - return 0;
> + if (!helper) {
> + helper = ct_lookup_helper(ct, net);
> + if (!helper) {
> + if (help)
> + RCU_INIT_POINTER(help->helper, NULL);
> + return 0;
> + }
> }
>
> - if (help == NULL) {
> + if (!help) {
I don't think these cleanups belong this patch. If you aim to net
tree, then please let's keep this smaller.
I know general coding style prefer different notation, but we have
quite many spots in netfilter that are using this style already.
Thank you.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
