* [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" @ 2014-11-24 23:14 Pablo Neira Ayuso 2014-11-24 23:14 ` [PATCH 2/2 nf] netfilter: conntrack: fix race between confirmation and flush Pablo Neira Ayuso 2014-11-25 12:26 ` [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Jesper Dangaard Brouer 0 siblings, 2 replies; 5+ messages in thread From: Pablo Neira Ayuso @ 2014-11-24 23:14 UTC (permalink / raw) To: netfilter-devel; +Cc: dborkman, jp.pozzi, brouer, programme110 This reverts commit 5195c14c8b27cc0b18220ddbf0e5ad3328a04187. If the conntrack clashes with an existing one, it is left out of the unconfirmed, thus, crashing when dropping the packet and releasing the conntrack. Reported-by: Daniel Borkmann <dborkman@redhat.com> Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=88841 Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- I prefer to revert the original fix and replace it by the follow up to pass one single patch to -stable. net/netfilter/nf_conntrack_core.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 2c69975..5016a69 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -611,16 +611,12 @@ __nf_conntrack_confirm(struct sk_buff *skb) */ NF_CT_ASSERT(!nf_ct_is_confirmed(ct)); pr_debug("Confirming conntrack %p\n", ct); - - /* We have to check the DYING flag after unlink to prevent - * a race against nf_ct_get_next_corpse() possibly called from - * user context, else we insert an already 'dead' hash, blocking - * further use of that particular connection -JM. - */ - nf_ct_del_from_dying_or_unconfirmed_list(ct); + /* We have to check the DYING flag inside the lock to prevent + a race against nf_ct_get_next_corpse() possibly called from + user context, else we insert an already 'dead' hash, blocking + further use of that particular connection -JM */ if (unlikely(nf_ct_is_dying(ct))) { - nf_ct_add_to_dying_list(ct); nf_conntrack_double_unlock(hash, reply_hash); local_bh_enable(); return NF_ACCEPT; @@ -640,6 +636,8 @@ __nf_conntrack_confirm(struct sk_buff *skb) zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) goto out; + nf_ct_del_from_dying_or_unconfirmed_list(ct); + /* Timer relative to confirmation time, not original setting time, otherwise we'd get timer wrap in weird delay cases. */ -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2 nf] netfilter: conntrack: fix race between confirmation and flush 2014-11-24 23:14 [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Pablo Neira Ayuso @ 2014-11-24 23:14 ` Pablo Neira Ayuso 2014-11-25 20:29 ` Jesper Dangaard Brouer 2014-11-25 12:26 ` [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Jesper Dangaard Brouer 1 sibling, 1 reply; 5+ messages in thread From: Pablo Neira Ayuso @ 2014-11-24 23:14 UTC (permalink / raw) To: netfilter-devel; +Cc: dborkman, jp.pozzi, brouer, programme110 Commit 5195c14c8b27c ("netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse") aimed to resolve the race condition between the confirmation (packet path) and the flush command (from control plane). However, it introduced a crash when several packets race to add a new conntrack, which seems easier to reproduce when nf_queue is in place. Fix this race, in __nf_conntrack_confirm(), by removing the CT from unconfirmed list before checking the DYING bit. In case race occured, re-add the CT to the dying list This patch also changes the verdict from NF_ACCEPT to NF_DROP when we lose race. Basically, the confirmation happens for the first packet that we see in a flow. If you just invoked conntrack -F once (which should be the common case), then this is likely to be the first packet of the flow (unless you already called flush anytime soon in the past). This should be hard to trigger, but better drop this packet, otherwise we leave things in inconsistent state since the destination will likely reply to this packet, but it will find no conntrack, unless the origin retransmits. The change of the verdict has been discussed in: https://www.marc.info/?l=linux-netdev&m=141588039530056&w=2 Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- Note: I will check and re-test this again tomorrow morning with fresh mind, compile tested only. net/netfilter/nf_conntrack_core.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c index 5016a69..c588012 100644 --- a/net/netfilter/nf_conntrack_core.c +++ b/net/netfilter/nf_conntrack_core.c @@ -611,16 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb) */ NF_CT_ASSERT(!nf_ct_is_confirmed(ct)); pr_debug("Confirming conntrack %p\n", ct); - /* We have to check the DYING flag inside the lock to prevent - a race against nf_ct_get_next_corpse() possibly called from - user context, else we insert an already 'dead' hash, blocking - further use of that particular connection -JM */ + /* We have to check the DYING flag after unlink to prevent + * a race against nf_ct_get_next_corpse() possibly called from + * user context, else we insert an already 'dead' hash, blocking + * further use of that particular connection -JM. + */ + nf_ct_del_from_dying_or_unconfirmed_list(ct); - if (unlikely(nf_ct_is_dying(ct))) { - nf_conntrack_double_unlock(hash, reply_hash); - local_bh_enable(); - return NF_ACCEPT; - } + if (unlikely(nf_ct_is_dying(ct))) + goto out; /* See if there's one in the list already, including reverse: NAT could have grabbed it without realizing, since we're @@ -636,8 +635,6 @@ __nf_conntrack_confirm(struct sk_buff *skb) zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) goto out; - nf_ct_del_from_dying_or_unconfirmed_list(ct); - /* Timer relative to confirmation time, not original setting time, otherwise we'd get timer wrap in weird delay cases. */ @@ -673,6 +670,7 @@ __nf_conntrack_confirm(struct sk_buff *skb) return NF_ACCEPT; out: + nf_ct_add_to_dying_list(ct); nf_conntrack_double_unlock(hash, reply_hash); NF_CT_STAT_INC(net, insert_failed); local_bh_enable(); -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2 nf] netfilter: conntrack: fix race between confirmation and flush 2014-11-24 23:14 ` [PATCH 2/2 nf] netfilter: conntrack: fix race between confirmation and flush Pablo Neira Ayuso @ 2014-11-25 20:29 ` Jesper Dangaard Brouer 0 siblings, 0 replies; 5+ messages in thread From: Jesper Dangaard Brouer @ 2014-11-25 20:29 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: brouer, netfilter-devel, dborkman, jp.pozzi, programme110, Florian Westphal On Tue, 25 Nov 2014 00:14:47 +0100 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Commit 5195c14c8b27c ("netfilter: conntrack: fix race in > __nf_conntrack_confirm against get_next_corpse") aimed to resolve the > race condition between the confirmation (packet path) and the flush > command (from control plane). However, it introduced a crash when > several packets race to add a new conntrack, which seems easier to > reproduce when nf_queue is in place. > > Fix this race, in __nf_conntrack_confirm(), by removing the CT > from unconfirmed list before checking the DYING bit. In case > race occured, re-add the CT to the dying list Re-adding it (to some list) case the hash race occurs should fix the problem (the reverted patch introduced). > This patch also changes the verdict from NF_ACCEPT to NF_DROP when > we lose race. Basically, the confirmation happens for the first packet > that we see in a flow. If you just invoked conntrack -F once (which > should be the common case), then this is likely to be the first packet > of the flow (unless you already called flush anytime soon in the past). > This should be hard to trigger, but better drop this packet, otherwise > we leave things in inconsistent state since the destination will likely > reply to this packet, but it will find no conntrack, unless the origin > retransmits. > > The change of the verdict has been discussed in: > https://www.marc.info/?l=linux-netdev&m=141588039530056&w=2 > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > Note: I will check and re-test this again tomorrow morning with fresh > mind, compile tested only. > > net/netfilter/nf_conntrack_core.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c > index 5016a69..c588012 100644 > --- a/net/netfilter/nf_conntrack_core.c > +++ b/net/netfilter/nf_conntrack_core.c > @@ -611,16 +611,15 @@ __nf_conntrack_confirm(struct sk_buff *skb) > */ > NF_CT_ASSERT(!nf_ct_is_confirmed(ct)); > pr_debug("Confirming conntrack %p\n", ct); > - /* We have to check the DYING flag inside the lock to prevent > - a race against nf_ct_get_next_corpse() possibly called from > - user context, else we insert an already 'dead' hash, blocking > - further use of that particular connection -JM */ > + /* We have to check the DYING flag after unlink to prevent > + * a race against nf_ct_get_next_corpse() possibly called from > + * user context, else we insert an already 'dead' hash, blocking > + * further use of that particular connection -JM. > + */ > + nf_ct_del_from_dying_or_unconfirmed_list(ct); > > - if (unlikely(nf_ct_is_dying(ct))) { > - nf_conntrack_double_unlock(hash, reply_hash); > - local_bh_enable(); > - return NF_ACCEPT; > - } > + if (unlikely(nf_ct_is_dying(ct))) > + goto out; > > /* See if there's one in the list already, including reverse: > NAT could have grabbed it without realizing, since we're > @@ -636,8 +635,6 @@ __nf_conntrack_confirm(struct sk_buff *skb) > zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h))) > goto out; > > - nf_ct_del_from_dying_or_unconfirmed_list(ct); > - > /* Timer relative to confirmation time, not original > setting time, otherwise we'd get timer wrap in > weird delay cases. */ > @@ -673,6 +670,7 @@ __nf_conntrack_confirm(struct sk_buff *skb) > return NF_ACCEPT; > > out: > + nf_ct_add_to_dying_list(ct); > nf_conntrack_double_unlock(hash, reply_hash); > NF_CT_STAT_INC(net, insert_failed); > local_bh_enable(); The change looks good to me. And we only re-add it to dying_list in the goto "out" error cases. I know Florian had some ideas of howto get rid of the unconfirmed list all together (which would be a fairly good optimization), but I guess we want a fairly simple patch for just fixing the bug first? Thanks for taking care of this Pablo. -- Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" 2014-11-24 23:14 [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Pablo Neira Ayuso 2014-11-24 23:14 ` [PATCH 2/2 nf] netfilter: conntrack: fix race between confirmation and flush Pablo Neira Ayuso @ 2014-11-25 12:26 ` Jesper Dangaard Brouer 2014-11-25 18:59 ` Pablo Neira Ayuso 1 sibling, 1 reply; 5+ messages in thread From: Jesper Dangaard Brouer @ 2014-11-25 12:26 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: netfilter-devel, dborkman, jp.pozzi, programme110, brouer On Tue, 25 Nov 2014 00:14:46 +0100 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > This reverts commit 5195c14c8b27cc0b18220ddbf0e5ad3328a04187. > > If the conntrack clashes with an existing one, it is left out of > the unconfirmed, thus, crashing when dropping the packet and > releasing the conntrack. > > Reported-by: Daniel Borkmann <dborkman@redhat.com> > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=88841 > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > I prefer to revert the original fix and replace it by the follow up to > pass one single patch to -stable. I'm fine with the revert. -- Best regards, Jesper Dangaard Brouer MSc.CS, Sr. Network Kernel Developer at Red Hat Author of http://www.iptv-analyzer.org LinkedIn: http://www.linkedin.com/in/brouer ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" 2014-11-25 12:26 ` [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Jesper Dangaard Brouer @ 2014-11-25 18:59 ` Pablo Neira Ayuso 0 siblings, 0 replies; 5+ messages in thread From: Pablo Neira Ayuso @ 2014-11-25 18:59 UTC (permalink / raw) To: Jesper Dangaard Brouer; +Cc: netfilter-devel, dborkman, jp.pozzi, programme110 On Tue, Nov 25, 2014 at 01:26:29PM +0100, Jesper Dangaard Brouer wrote: > On Tue, 25 Nov 2014 00:14:46 +0100 > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > This reverts commit 5195c14c8b27cc0b18220ddbf0e5ad3328a04187. > > > > If the conntrack clashes with an existing one, it is left out of > > the unconfirmed, thus, crashing when dropping the packet and > > releasing the conntrack. > > > > Reported-by: Daniel Borkmann <dborkman@redhat.com> > > Fixes: https://bugzilla.kernel.org/show_bug.cgi?id=88841 > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > I prefer to revert the original fix and replace it by the follow up to > > pass one single patch to -stable. > > I'm fine with the revert. No problem. It would be great if I can get some feedback on the alternative I'm proposing to resolve this (see patch 2/2 in this series). Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-25 20:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-24 23:14 [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Pablo Neira Ayuso 2014-11-24 23:14 ` [PATCH 2/2 nf] netfilter: conntrack: fix race between confirmation and flush Pablo Neira Ayuso 2014-11-25 20:29 ` Jesper Dangaard Brouer 2014-11-25 12:26 ` [PATCH 1/2 nf] Revert "netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse" Jesper Dangaard Brouer 2014-11-25 18:59 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).