[nf-next PATCH] netfilter: conntrack: Relax helper auto-assignment warning for nftables

[nf-next PATCH] netfilter: conntrack: Relax helper auto-assignment warning for nftables

[Phil Sutter]

With nftables, no template is being used and instead helper assignment
happens after conntrack initialization. With helper auto assignment
being disabled by default, this leads to this spurious kernel log
suggesting to use iptables CT target.

To avoid the bogus and confusing message, check helper's refcount: It is
initialized to 1 by nf_conntrack_helper_register() and incremented by
nf_conntrack_helper_try_module_get() during nft_ct_helper_obj_init(). So
if its value is larger than 1, it must be in use *somewhere*.

This approach is not perfect since there is no guarantee the helper will
actually be assigned to the packet's flow. Yet it should still cover the
intended use-case of merely loading the module.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/netfilter/nf_conntrack_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
index ae4488a13c70c..828957f1802d8 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -213,11 +213,13 @@ static struct nf_conntrack_helper *
 nf_ct_lookup_helper(struct nf_conn *ct, struct net *net)
 {
 	struct nf_conntrack_net *cnet = nf_ct_pernet(net);
+	struct nf_conntrack_helper *helper =
+		__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
 
 	if (!cnet->sysctl_auto_assign_helper) {
 		if (cnet->auto_assign_helper_warned)
 			return NULL;
-		if (!__nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))
+		if (!helper || refcount_read(&helper->refcnt) > 1)
 			return NULL;
 		pr_info("nf_conntrack: default automatic helper assignment "
 			"has been turned off for security reasons and CT-based "
@@ -227,7 +229,7 @@ nf_ct_lookup_helper(struct nf_conn *ct, struct net *net)
 		return NULL;
 	}
 
-	return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
+	return helper;
 }
 
 int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
-- 
2.34.1

Re: [nf-next PATCH] netfilter: conntrack: Relax helper auto-assignment warning for nftables

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> With nftables, no template is being used and instead helper assignment
> happens after conntrack initialization. With helper auto assignment
> being disabled by default, this leads to this spurious kernel log
> suggesting to use iptables CT target.
>
> To avoid the bogus and confusing message, check helper's refcount: It is
> initialized to 1 by nf_conntrack_helper_register() and incremented by
> nf_conntrack_helper_try_module_get() during nft_ct_helper_obj_init(). So
> if its value is larger than 1, it must be in use *somewhere*.

Why not set cnet->auto_assign_helper_warned = true; from nft_ct.c?

Re: [nf-next PATCH] netfilter: conntrack: Relax helper auto-assignment warning for nftables

[Phil Sutter]

Hi,

On Sat, Feb 19, 2022 at 02:25:47PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > With nftables, no template is being used and instead helper assignment
> > happens after conntrack initialization. With helper auto assignment
> > being disabled by default, this leads to this spurious kernel log
> > suggesting to use iptables CT target.
> >
> > To avoid the bogus and confusing message, check helper's refcount: It is
> > initialized to 1 by nf_conntrack_helper_register() and incremented by
> > nf_conntrack_helper_try_module_get() during nft_ct_helper_obj_init(). So
> > if its value is larger than 1, it must be in use *somewhere*.
> 
> Why not set cnet->auto_assign_helper_warned = true; from nft_ct.c?

I tried, but nf_ct_pernet() is not usable from module context, or
actually symbol nf_conntrack_net_id. So I had to introduce a routine to
set the value. On the other hand I didn't like the fact that it would
permanently disable the warning even after 'nft flush ruleset'
(nit-picking).

I can recover that approach and send a v2 if you think (re-)using refcnt
is a bad idea here.

Thanks, Phil

Re: [nf-next PATCH] netfilter: conntrack: Relax helper auto-assignment warning for nftables

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> I tried, but nf_ct_pernet() is not usable from module context, or
> actually symbol nf_conntrack_net_id. So I had to introduce a routine to
> set the value. On the other hand I didn't like the fact that it would
> permanently disable the warning even after 'nft flush ruleset'
> (nit-picking).
> I can recover that approach and send a v2 if you think (re-)using refcnt
> is a bad idea here.

I think its a good idea to add a helper function disable the warning.

We can also call it from xt_CT.c.

Re: [nf-next PATCH] netfilter: conntrack: Relax helper auto-assignment warning for nftables

[Phil Sutter]

On Sat, Feb 19, 2022 at 02:41:59PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@nwl.cc> wrote:
> > I tried, but nf_ct_pernet() is not usable from module context, or
> > actually symbol nf_conntrack_net_id. So I had to introduce a routine to
> > set the value. On the other hand I didn't like the fact that it would
> > permanently disable the warning even after 'nft flush ruleset'
> > (nit-picking).
> > I can recover that approach and send a v2 if you think (re-)using refcnt
> > is a bad idea here.
> 
> I think its a good idea to add a helper function disable the warning.
> 
> We can also call it from xt_CT.c.

OK, I'll prepare a v2. Thanks!