[PATCH nf] netfilter: conntrack: restore IPS_CONFIRMED out of nf_conntrack_hash_check_insert()

[PATCH nf] netfilter: conntrack: restore IPS_CONFIRMED out of nf_conntrack_hash_check_insert()

[Pablo Neira Ayuso]

e6d57e9ff0ae ("netfilter: conntrack: fix rmmod double-free race")
consolidates IPS_CONFIRMED bit set in nf_conntrack_hash_check_insert().
However, this breaks ctnetlink:

 # conntrack -I -p tcp --timeout 123 --src 1.2.3.4 --dst 5.6.7.8 --state ESTABLISHED --sport 1 --dport 4 -u SEEN_REPLY
 conntrack v1.4.6 (conntrack-tools): Operation failed: Device or resource busy

This is a partial revert of the aforementioned commit.

Fixes: e6d57e9ff0ae ("netfilter: conntrack: fix rmmod double-free race")
Reported-by: Stéphane Graber <stgraber@stgraber.org>
Tested-by: Stéphane Graber <stgraber@stgraber.org>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_conntrack_bpf.c     | 1 +
 net/netfilter/nf_conntrack_core.c    | 1 -
 net/netfilter/nf_conntrack_netlink.c | 3 +++
 3 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index cd99e6dc1f35..34913521c385 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -381,6 +381,7 @@ __bpf_kfunc struct nf_conn *bpf_ct_insert_entry(struct nf_conn___init *nfct_i)
 	struct nf_conn *nfct = (struct nf_conn *)nfct_i;
 	int err;
 
+	nfct->status |= IPS_CONFIRMED;
 	err = nf_conntrack_hash_check_insert(nfct);
 	if (err < 0) {
 		nf_conntrack_free(nfct);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index c6a6a6099b4e..7ba6ab9b54b5 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -932,7 +932,6 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct)
 		goto out;
 	}
 
-	ct->status |= IPS_CONFIRMED;
 	smp_wmb();
 	/* The caller holds a reference to this object */
 	refcount_set(&ct->ct_general.use, 2);
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index bfc3aaa2c872..d3ee18854698 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2316,6 +2316,9 @@ ctnetlink_create_conntrack(struct net *net,
 	nfct_seqadj_ext_add(ct);
 	nfct_synproxy_ext_add(ct);
 
+	/* we must add conntrack extensions before confirmation. */
+	ct->status |= IPS_CONFIRMED;
+
 	if (cda[CTA_STATUS]) {
 		err = ctnetlink_change_status(ct, cda);
 		if (err < 0)
-- 
2.30.2

Re: [PATCH nf] netfilter: conntrack: restore IPS_CONFIRMED out of nf_conntrack_hash_check_insert()

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> e6d57e9ff0ae ("netfilter: conntrack: fix rmmod double-free race")
> consolidates IPS_CONFIRMED bit set in nf_conntrack_hash_check_insert().
> However, this breaks ctnetlink:
> 
>  # conntrack -I -p tcp --timeout 123 --src 1.2.3.4 --dst 5.6.7.8 --state ESTABLISHED --sport 1 --dport 4 -u SEEN_REPLY
>  conntrack v1.4.6 (conntrack-tools): Operation failed: Device or resource busy
> 
> This is a partial revert of the aforementioned commit.
> 
> Fixes: e6d57e9ff0ae ("netfilter: conntrack: fix rmmod double-free race")
> Reported-by: Stéphane Graber <stgraber@stgraber.org>
> Tested-by: Stéphane Graber <stgraber@stgraber.org>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_conntrack_bpf.c     | 1 +
>  net/netfilter/nf_conntrack_core.c    | 1 -
>  net/netfilter/nf_conntrack_netlink.c | 3 +++
>  3 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index bfc3aaa2c872..d3ee18854698 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -2316,6 +2316,9 @@ ctnetlink_create_conntrack(struct net *net,
>  	nfct_seqadj_ext_add(ct);
>  	nfct_synproxy_ext_add(ct);
>  
> +	/* we must add conntrack extensions before confirmation. */
> +	ct->status |= IPS_CONFIRMED;
> +

I'd guess that these 2 lines are the only part that is needed, but up
to you.

Re: [PATCH nf] netfilter: conntrack: restore IPS_CONFIRMED out of nf_conntrack_hash_check_insert()

[Pablo Neira Ayuso]

On Wed, Apr 19, 2023 at 08:17:23AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > e6d57e9ff0ae ("netfilter: conntrack: fix rmmod double-free race")
> > consolidates IPS_CONFIRMED bit set in nf_conntrack_hash_check_insert().
> > However, this breaks ctnetlink:
> > 
> >  # conntrack -I -p tcp --timeout 123 --src 1.2.3.4 --dst 5.6.7.8 --state ESTABLISHED --sport 1 --dport 4 -u SEEN_REPLY
> >  conntrack v1.4.6 (conntrack-tools): Operation failed: Device or resource busy
> > 
> > This is a partial revert of the aforementioned commit.
> > 
> > Fixes: e6d57e9ff0ae ("netfilter: conntrack: fix rmmod double-free race")
> > Reported-by: Stéphane Graber <stgraber@stgraber.org>
> > Tested-by: Stéphane Graber <stgraber@stgraber.org>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_conntrack_bpf.c     | 1 +
> >  net/netfilter/nf_conntrack_core.c    | 1 -
> >  net/netfilter/nf_conntrack_netlink.c | 3 +++
> >  3 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > index bfc3aaa2c872..d3ee18854698 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -2316,6 +2316,9 @@ ctnetlink_create_conntrack(struct net *net,
> >  	nfct_seqadj_ext_add(ct);
> >  	nfct_synproxy_ext_add(ct);
> >  
> > +	/* we must add conntrack extensions before confirmation. */
> > +	ct->status |= IPS_CONFIRMED;
> > +
> 
> I'd guess that these 2 lines are the only part that is needed, but up
> to you.

OK, I have drropped the bfp chunk.