| Message ID | 1434567805-2996-1-git-send-email-chris.j.arges@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 17, 2015 at 02:03:25PM -0500, Chris J Arges wrote: > From: Pablo Neira Ayuso <pablo@netfilter.org> > > BugLink: http://bugs.launchpad.net/bugs/1466135 > > 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:[<ffffffffa03d99ac>] [<ffffffffa03d99ac>] destroy_conntrack+0x15c/0x190 [nf_conntrack] > <4>[67096.760255] Call Trace: > <4>[67096.760255] [<ffffffff814844a7>] nf_conntrack_destroy+0x17/0x30 > <4>[67096.760255] [<ffffffffa03d9bb5>] nf_conntrack_find_get+0x85/0x130 [nf_conntrack] > <4>[67096.760255] [<ffffffffa03d9fb2>] nf_conntrack_in+0x352/0xb60 [nf_conntrack] > <4>[67096.760255] [<ffffffffa048c771>] ipv4_conntrack_local+0x51/0x60 [nf_conntrack_ipv4] > <4>[67096.760255] [<ffffffff81484419>] nf_iterate+0x69/0xb0 > <4>[67096.760255] [<ffffffff814b5b00>] ? dst_output+0x0/0x20 > <4>[67096.760255] [<ffffffff814845d4>] nf_hook_slow+0x74/0x110 > <4>[67096.760255] [<ffffffff814b5b00>] ? dst_output+0x0/0x20 > <4>[67096.760255] [<ffffffff814b66d5>] raw_sendmsg+0x775/0x910 > <4>[67096.760255] [<ffffffff8104c5a8>] ? flush_tlb_others_ipi+0x128/0x130 > <4>[67096.760255] [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [<ffffffff814c136a>] inet_sendmsg+0x4a/0xb0 > <4>[67096.760255] [<ffffffff81444e93>] ? sock_sendmsg+0x13/0x140 > <4>[67096.760255] [<ffffffff81444f97>] sock_sendmsg+0x117/0x140 > <4>[67096.760255] [<ffffffff8102e299>] ? native_smp_send_reschedule+0x49/0x60 > <4>[67096.760255] [<ffffffff81519beb>] ? _spin_unlock_bh+0x1b/0x20 > <4>[67096.760255] [<ffffffff8109d930>] ? autoremove_wake_function+0x0/0x40 > <4>[67096.760255] [<ffffffff814960f0>] ? do_ip_setsockopt+0x90/0xd80 > <4>[67096.760255] [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [<ffffffff8100bc4e>] ? apic_timer_interrupt+0xe/0x20 > <4>[67096.760255] [<ffffffff814457c9>] sys_sendto+0x139/0x190 > <4>[67096.760255] [<ffffffff810efa77>] ? audit_syscall_entry+0x1d7/0x200 > <4>[67096.760255] [<ffffffff810ef7c5>] ? __audit_syscall_exit+0x265/0x290 > <4>[67096.760255] [<ffffffff81474daf>] compat_sys_socketcall+0x13f/0x210 > <4>[67096.760255] [<ffffffff8104dea3>] 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 <edumazet@google.com> > Cc: Andrew Vagin <avagin@parallels.com> > Cc: Florian Westphal <fw@strlen.de> > Reported-by: Andrew Vagin <avagin@parallels.com> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > Reviewed-by: Eric Dumazet <edumazet@google.com> > Acked-by: Andrew Vagin <avagin@parallels.com> > (cherry picked from commit e53376bef2cd97d3e3f61fdc677fb8da7d03d0da) > Signed-off-by: Chris J Arges <chris.j.arges@canonical.com> > --- > include/net/netfilter/nf_conntrack.h | 2 ++ > net/netfilter/nf_conntrack_core.c | 34 +++++++++++++++++++++++++++++----- > net/netfilter/nf_synproxy_core.c | 5 ++--- > net/netfilter/xt_CT.c | 7 +------ > 4 files changed, 34 insertions(+), 14 deletions(-) > > diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h > index 01ea6ee..b2ac624 100644 > --- a/include/net/netfilter/nf_conntrack.h > +++ b/include/net/netfilter/nf_conntrack.h > @@ -284,6 +284,8 @@ extern unsigned int nf_conntrack_max; > extern unsigned int nf_conntrack_hash_rnd; > void init_nf_conntrack_hash_rnd(void); > > +void nf_conntrack_tmpl_insert(struct net *net, struct nf_conn *tmpl); > + > #define NF_CT_STAT_INC(net, count) __this_cpu_inc((net)->ct.stat->count) > #define NF_CT_STAT_INC_ATOMIC(net, count) this_cpu_inc((net)->ct.stat->count) > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index 43549eb..5ede63c 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -450,7 +450,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); > @@ -464,6 +466,21 @@ out: > } > EXPORT_SYMBOL_GPL(nf_conntrack_hash_check_insert); > > +/* deletion from this larval template list happens via nf_ct_put() */ > +void nf_conntrack_tmpl_insert(struct net *net, struct nf_conn *tmpl) > +{ > + __set_bit(IPS_TEMPLATE_BIT, &tmpl->status); > + __set_bit(IPS_CONFIRMED_BIT, &tmpl->status); > + nf_conntrack_get(&tmpl->ct_general); > + > + spin_lock_bh(&nf_conntrack_lock); > + /* Overload tuple linked list to put us in template list. */ > + hlist_nulls_add_head_rcu(&tmpl->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, > + &net->ct.tmpl); > + spin_unlock_bh(&nf_conntrack_lock); > +} > +EXPORT_SYMBOL_GPL(nf_conntrack_tmpl_insert); > + > /* Confirm a connection given skb; places it in hash table */ > int > __nf_conntrack_confirm(struct sk_buff *skb) > @@ -735,11 +752,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 > @@ -763,6 +779,11 @@ 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); > nf_ct_ext_free(ct); > kmem_cache_free(net->ct.nf_conntrack_cachep, ct); > @@ -858,6 +879,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); > diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c > index 9858e3e..52e20c9 100644 > --- a/net/netfilter/nf_synproxy_core.c > +++ b/net/netfilter/nf_synproxy_core.c > @@ -363,9 +363,8 @@ static int __net_init synproxy_net_init(struct net *net) > goto err2; > if (!nfct_synproxy_ext_add(ct)) > goto err2; > - __set_bit(IPS_TEMPLATE_BIT, &ct->status); > - __set_bit(IPS_CONFIRMED_BIT, &ct->status); > > + nf_conntrack_tmpl_insert(net, ct); > snet->tmpl = ct; > > snet->stats = alloc_percpu(struct synproxy_stats); > @@ -390,7 +389,7 @@ static void __net_exit synproxy_net_exit(struct net *net) > { > struct synproxy_net *snet = synproxy_pernet(net); > > - nf_conntrack_free(snet->tmpl); > + nf_ct_put(snet->tmpl); > synproxy_proc_exit(net); > free_percpu(snet->stats); > } > diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c > index da35ac0..8899601 100644 > --- a/net/netfilter/xt_CT.c > +++ b/net/netfilter/xt_CT.c > @@ -226,12 +226,7 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par, > goto err3; > } > > - __set_bit(IPS_TEMPLATE_BIT, &ct->status); > - __set_bit(IPS_CONFIRMED_BIT, &ct->status); > - > - /* Overload tuple linked list to put us in template list. */ > - hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, > - &par->net->ct.tmpl); > + nf_conntrack_tmpl_insert(par->net, ct); > out: > info->ct = ct; > return 0; > -- > 1.9.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Clean cherry-pick.
Applied to the Trusty master-next branch.
diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h index 01ea6ee..b2ac624 100644 --- a/include/net/netfilter/nf_conntrack.h +++ b/include/net/netfilter/nf_conntrack.h @@ -284,6 +284,8 @@ extern unsigned int nf_conntrack_max; extern unsigned int nf_conntrack_hash_rnd; void init_nf_conntrack_hash_rnd(void); +void nf_conntrack_tmpl_insert(struct net *net, struct nf_conn *tmpl); + #define NF_CT_STAT_INC(net, count) __this_cpu_inc((net)->ct.stat->count) #define NF_CT_STAT_INC_ATOMIC(net, count) this_cpu_inc((net)->ct.stat->count) diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 43549eb..5ede63c 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -450,7 +450,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); @@ -464,6 +466,21 @@ out: } EXPORT_SYMBOL_GPL(nf_conntrack_hash_check_insert); +/* deletion from this larval template list happens via nf_ct_put() */ +void nf_conntrack_tmpl_insert(struct net *net, struct nf_conn *tmpl) +{ + __set_bit(IPS_TEMPLATE_BIT, &tmpl->status); + __set_bit(IPS_CONFIRMED_BIT, &tmpl->status); + nf_conntrack_get(&tmpl->ct_general); + + spin_lock_bh(&nf_conntrack_lock); + /* Overload tuple linked list to put us in template list. */ + hlist_nulls_add_head_rcu(&tmpl->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, + &net->ct.tmpl); + spin_unlock_bh(&nf_conntrack_lock); +} +EXPORT_SYMBOL_GPL(nf_conntrack_tmpl_insert); + /* Confirm a connection given skb; places it in hash table */ int __nf_conntrack_confirm(struct sk_buff *skb) @@ -735,11 +752,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 @@ -763,6 +779,11 @@ 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); nf_ct_ext_free(ct); kmem_cache_free(net->ct.nf_conntrack_cachep, ct); @@ -858,6 +879,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); diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c index 9858e3e..52e20c9 100644 --- a/net/netfilter/nf_synproxy_core.c +++ b/net/netfilter/nf_synproxy_core.c @@ -363,9 +363,8 @@ static int __net_init synproxy_net_init(struct net *net) goto err2; if (!nfct_synproxy_ext_add(ct)) goto err2; - __set_bit(IPS_TEMPLATE_BIT, &ct->status); - __set_bit(IPS_CONFIRMED_BIT, &ct->status); + nf_conntrack_tmpl_insert(net, ct); snet->tmpl = ct; snet->stats = alloc_percpu(struct synproxy_stats); @@ -390,7 +389,7 @@ static void __net_exit synproxy_net_exit(struct net *net) { struct synproxy_net *snet = synproxy_pernet(net); - nf_conntrack_free(snet->tmpl); + nf_ct_put(snet->tmpl); synproxy_proc_exit(net); free_percpu(snet->stats); } diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c index da35ac0..8899601 100644 --- a/net/netfilter/xt_CT.c +++ b/net/netfilter/xt_CT.c @@ -226,12 +226,7 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par, goto err3; } - __set_bit(IPS_TEMPLATE_BIT, &ct->status); - __set_bit(IPS_CONFIRMED_BIT, &ct->status); - - /* Overload tuple linked list to put us in template list. */ - hlist_nulls_add_head_rcu(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode, - &par->net->ct.tmpl); + nf_conntrack_tmpl_insert(par->net, ct); out: info->ct = ct; return 0;