Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Jan 25, 2017 at 09:43:14PM +0100, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
> 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@suse.cz>
> ---
>  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.