From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ani Sinha Subject: Re: [PATCH 1/1] commit e53376bef2cd97d3e3f61fdc677fb8da7d03d0da upstream. Date: Sat, 24 Oct 2015 11:31:40 -0700 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 To: Pablo Neira Ayuso , Patrick McHardy , Eric Dumazet , "David S. Miller" , netfilter-devel@vger.kernel.org, Andrew Vagin , coreteam@netfilter.org, Florian Westphal , "netdev@vger.kernel.org" , Zefan Li , Ani Sinha Return-path: In-Reply-To: Sender: netdev-owner@vger.kernel.org List-Id: netfilter-devel.vger.kernel.org Please refer to the thread "linux 3.4.43 : kernel crash at __nf_conntrack_confirm" on netdev for context. thanks On Sat, Oct 24, 2015 at 10:27 AM, Ani Sinha wrote: > netfilter: nf_conntrack: don't release a conntrack with non-zero > refcnt > > With this patch, the conntrack refcount is initially set to zero and > it is bumped once it is added to any of the list, so we fulfill > Eric's golden rule which is that all released objects always have a > refcount that equals zero. > > Andrey Vagin reports that nf_conntrack_free can't be called for a > conntrack with non-zero ref-counter, because it can race with > nf_conntrack_find_get(). > > A conntrack slab is created with SLAB_DESTROY_BY_RCU. Non-zero > ref-counter says that this conntrack is used. So when we release > a conntrack with non-zero counter, we break this assumption. > > CPU1 CPU2 > ____nf_conntrack_find() > nf_ct_put() > destroy_conntrack() > ... > init_conntrack > __nf_conntrack_alloc (set use = 1) > atomic_inc_not_zero(&ct->use) (use = 2) > if (!l4proto->new(ct, skb, dataoff, timeouts)) > nf_conntrack_free(ct); (use = 2 !!!) > ... > __nf_conntrack_alloc (set use = 1) > if (!nf_ct_key_equal(h, tuple, zone)) > nf_ct_put(ct); (use = 0) > destroy_conntrack() > /* continue to work with CT */ > > After applying the path "[PATCH] netfilter: nf_conntrack: fix RCU > race in nf_conntrack_find_get" another bug was triggered in > destroy_conntrack(): > > <4>[67096.759334] ------------[ cut here ]------------ > <2>[67096.759353] kernel BUG at net/netfilter/nf_conntrack_core.c:211! > ... > <4>[67096.759837] Pid: 498649, comm: atdd veid: 666 Tainted: G C --------------- 2.6.32-042stab084.18 #1 042stab084_18 /DQ45CB > <4>[67096.759932] RIP: 0010:[] [] destroy_conntrack+0x15c/0x190 [nf_conntrack] > <4>[67096.760255] Call Trace: > <4>[67096.760255] [] nf_conntrack_destroy+0x17/0x30 > <4>[67096.760255] [] nf_conntrack_find_get+0x85/0x130 [nf_conntrack] > <4>[67096.760255] [] nf_conntrack_in+0x352/0xb60 [nf_conntrack] > <4>[67096.760255] [] ipv4_conntrack_local+0x51/0x60 [nf_conntrack_ipv4] > <4>[67096.760255] [] nf_iterate+0x69/0xb0 > <4>[67096.760255] [] ? dst_output+0x0/0x20 > <4>[67096.760255] [] nf_hook_slow+0x74/0x110 > <4>[67096.760255] [] ? dst_output+0x0/0x20 > <4>[67096.760255] [] raw_sendmsg+0x775/0x910 > <4>[67096.760255] [] ? flush_tlb_others_ipi+0x128/0x130 > <4>[67096.760255] [] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [] inet_sendmsg+0x4a/0xb0 > <4>[67096.760255] [] ? sock_sendmsg+0x13/0x140 > <4>[67096.760255] [] sock_sendmsg+0x117/0x140 > <4>[67096.760255] [] ? native_smp_send_reschedule+0x49/0x60 > <4>[67096.760255] [] ? _spin_unlock_bh+0x1b/0x20 > <4>[67096.760255] [] ? autoremove_wake_function+0x0/0x40 > <4>[67096.760255] [] ? do_ip_setsockopt+0x90/0xd80 > <4>[67096.760255] [] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [] sys_sendto+0x139/0x190 > <4>[67096.760255] [] ? audit_syscall_entry+0x1d7/0x200 > <4>[67096.760255] [] ? __audit_syscall_exit+0x265/0x290 > <4>[67096.760255] [] compat_sys_socketcall+0x13f/0x210 > <4>[67096.760255] [] ia32_sysret+0x0/0x5 > > I have reused the original title for the RFC patch that Andrey posted and > most of the original patch description. > > Cc: Eric Dumazet > Cc: Andrew Vagin > Cc: Florian Westphal > Cc: Zefan Li > Signed-off-by: Ani Sinha > Reported-by: Andrew Vagin > Signed-off-by: Pablo Neira Ayuso > Reviewed-by: Eric Dumazet > Acked-by: Andrew Vagin > --- > net/netfilter/nf_conntrack_core.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index 9a171b2..9a46908 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -441,7 +441,9 @@ nf_conntrack_hash_check_insert(struct nf_conn *ct) > goto out; > > add_timer(&ct->timeout); > - nf_conntrack_get(&ct->ct_general); > + smp_wmb(); > + /* The caller holds a reference to this object */ > + atomic_set(&ct->ct_general.use, 2); > __nf_conntrack_hash_insert(ct, hash, repl_hash); > NF_CT_STAT_INC(net, insert); > spin_unlock_bh(&nf_conntrack_lock); > @@ -732,11 +734,10 @@ __nf_conntrack_alloc(struct net *net, u16 zone, > nf_ct_zone->id = zone; > } > #endif > - /* > - * changes to lookup keys must be done before setting refcnt to 1 > + /* Because we use RCU lookups, we set ct_general.use to zero before > + * this is inserted in any list. > */ > - smp_wmb(); > - atomic_set(&ct->ct_general.use, 1); > + atomic_set(&ct->ct_general.use, 0); > return ct; > > #ifdef CONFIG_NF_CONNTRACK_ZONES > @@ -759,6 +760,10 @@ EXPORT_SYMBOL_GPL(nf_conntrack_alloc); > void nf_conntrack_free(struct nf_conn *ct) > { > struct net *net = nf_ct_net(ct); > + /* A freed object has refcnt == 0, that's > + * the golden rule for SLAB_DESTROY_BY_RCU > + */ > + NF_CT_ASSERT(atomic_read(&ct->ct_general.use) == 0); > > nf_ct_ext_destroy(ct); > atomic_dec(&net->ct.count); > @@ -846,6 +851,9 @@ init_conntrack(struct net *net, struct nf_conn *tmpl, > NF_CT_STAT_INC(net, new); > } > > + /* Now it is inserted into the unconfirmed list, bump refcount */ > + nf_conntrack_get(&ct->ct_general); > + > /* Overload tuple linked list to put us in unconfirmed list. */ > hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, > &net->ct.unconfirmed); > -- > 1.8.1.4 >