Re: netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse

Re: netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse

[Jesper Dangaard Brouer]

On Tue, 4 Nov 2014 09:48:32 +0800
"billbonaparte" <programme110@gmail.com> wrote:

> (sorry to send this e-mail again, last mail is rejected by server due to
> non-acceptable content)

There is several issues with your submission.  I'll take care of
resubmitting a patch in your name (so you will get credit in the git
log).

If you care to know, issues are:
 1. you are not sending to the appropriate mailing lists,
 2. patch is as an attachment (should be inlined),
 3. the patch have style and white-space issues.


> Florian Westphal [mailto:fw@strlen.de] wrote:
> >Correct.  This is broken since the central spin lock removal, since 
> >nf_conntrack_lock no longer protects both get_next_corpse and 
> >conntrack_confirm.
> > 
> >Please send a patch, moving dying check after removal of conntrack from 
> >the percpu list,
>
> Since unconfirmed conntrack is stored in unconfirmed-list which is per-cpu
> list and protected by per-cpu spin-lock, we can remove it from
> uncomfirmed-list and insert it into ct-hash-table separately. that is to
> say, we can remove it from uncomfirmed-list without holding corresponding
> hash-lock, then check if it is dying.
> if it is dying, we add it to the dying-list, then quit
> __nf_conntrack_confirm. we do this to follow the rules that the conntrack
> must alternatively at unconfirmed-list or dying-list when it is abort to be
> destroyed.

In the resubmit. I'll take a slightly more conservative approach, by
keeping the DYING check under the hash-lock, as it is currently.  I
guess we could do it without holding the hash-lock, but I want to keep
the fix as simple as possible.


> >> 	2. operation on ct->status should be atomic, because it race aginst 
> >> get_next_corpse.
[...]
> if there is a race at operating ct->status, there will be in alternative
> case:
> 1) IPS_DYING bit which set in get_next_corpse override other bits (e.g.
> IPS_SRC_NAT_DONE_BIT), or
> 2) other bits (e.g. IPS_SRC_NAT_DONE_BIT) which set in nf_nat_setup_info
> override IPS_DYING bit.

Notice the set_bit() is atomic, so we don't have these issues (of bits
getting overridden).

-- 
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

[PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Jesper Dangaard Brouer]

From: bill bonaparte <programme110@gmail.com>

After removal of the central spinlock nf_conntrack_lock, in
commit 93bb0ceb75be2 ("netfilter: conntrack: remove central
spinlock nf_conntrack_lock"), it is possible to race against
get_next_corpse().

The race is against the get_next_corpse() cleanup on
the "unconfirmed" list (a per-cpu list with seperate locking),
which set the DYING bit.

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.

Fixes: 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
Reported-by: bill bonaparte <programme110@gmail.com>
Signed-off-by: bill bonaparte <programme110@gmail.com>
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---

 net/netfilter/nf_conntrack_core.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 5016a69..1072650 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -611,12 +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
+
+	/* 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_ct_add_to_dying_list(ct);
 		nf_conntrack_double_unlock(hash, reply_hash);
 		local_bh_enable();
 		return NF_ACCEPT;
@@ -636,8 +639,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. */

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Pablo Neira Ayuso]

On Thu, Nov 06, 2014 at 02:36:48PM +0100, Jesper Dangaard Brouer wrote:
> From: bill bonaparte <programme110@gmail.com>
> 
> After removal of the central spinlock nf_conntrack_lock, in
> commit 93bb0ceb75be2 ("netfilter: conntrack: remove central
> spinlock nf_conntrack_lock"), it is possible to race against
> get_next_corpse().
> 
> The race is against the get_next_corpse() cleanup on
> the "unconfirmed" list (a per-cpu list with seperate locking),
> which set the DYING bit.
> 
> 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 seems correct to me, some side comments.

> Fixes: 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
> Reported-by: bill bonaparte <programme110@gmail.com>
> Signed-off-by: bill bonaparte <programme110@gmail.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
> 
>  net/netfilter/nf_conntrack_core.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 5016a69..1072650 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -611,12 +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
> +
> +	/* 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 */

While at this, I think it would be good to fix comment style to:

        /* We have ...
         * ...
         */

I can fix this here, no need to resend, just let me know.

> +	nf_ct_del_from_dying_or_unconfirmed_list(ct);
>  
>  	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;

Not directly related to your patch, but I don't find a good reason why
we're accepting this packet.

If the conntrack from the unconfirmed list is dying, then the object
will be released by when the packet leaves the stack to its
destination. With stateful filtering depending in place, the follow up
packet in the reply direction will likely be considered invalid (if
tcp tracking is on). Fortunately for us, the origin will likely
retransmit the syn again, so the ct will be setup accordingly.

So, why should we allow this to go through?

This return verdict was introduced in: fc35077 ("netfilter:
nf_conntrack: fix a race in __nf_conntrack_confirm against
nf_ct_get_next_corpse()") btw.

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Jesper Dangaard Brouer]

On Mon, 10 Nov 2014 17:54:39 +0100
Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> On Thu, Nov 06, 2014 at 02:36:48PM +0100, Jesper Dangaard Brouer wrote:
> > From: bill bonaparte <programme110@gmail.com>
> > 
> > After removal of the central spinlock nf_conntrack_lock, in
> > commit 93bb0ceb75be2 ("netfilter: conntrack: remove central
> > spinlock nf_conntrack_lock"), it is possible to race against
> > get_next_corpse().
> > 
> > The race is against the get_next_corpse() cleanup on
> > the "unconfirmed" list (a per-cpu list with seperate locking),
> > which set the DYING bit.
> > 
> > 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 seems correct to me, some side comments.
> 
> > Fixes: 93bb0ceb75be2 ("netfilter: conntrack: remove central spinlock nf_conntrack_lock")
> > Reported-by: bill bonaparte <programme110@gmail.com>
> > Signed-off-by: bill bonaparte <programme110@gmail.com>
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> > 
> >  net/netfilter/nf_conntrack_core.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index 5016a69..1072650 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -611,12 +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
> > +
> > +	/* 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 */
> 
> While at this, I think it would be good to fix comment style to:
> 
>         /* We have ...
>          * ...
>          */
> 
> I can fix this here, no need to resend, just let me know.

Okay, I was just trying to keep the changes as minimal as possible, if
this should go into a stable-kernel.  Your choice.


> > +	nf_ct_del_from_dying_or_unconfirmed_list(ct);
> >  
> >  	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;
> 
> Not directly related to your patch, but I don't find a good reason why
> we're accepting this packet.
> 
> If the conntrack from the unconfirmed list is dying, then the object
> will be released by when the packet leaves the stack to its
> destination. With stateful filtering depending in place, the follow up
> packet in the reply direction will likely be considered invalid (if
> tcp tracking is on). Fortunately for us, the origin will likely
> retransmit the syn again, so the ct will be setup accordingly.
> 
> So, why should we allow this to go through?

True, it also seems strange to me that we accept this packet.

> This return verdict was introduced in: fc35077 ("netfilter:
> nf_conntrack: fix a race in __nf_conntrack_confirm against
> nf_ct_get_next_corpse()") btw.

And the commit does not argue why NF_ACCEPT was chosen...

-- 
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

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Jörg Marx]

On 12.11.2014 08:35, Jesper Dangaard Brouer wrote:

Hi,

I wrote the patch in 2010, so find some arguments below:

>>> > > +	nf_ct_del_from_dying_or_unconfirmed_list(ct);
>>> > >  
>>> > >  	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;
>> > 
>> > Not directly related to your patch, but I don't find a good reason why
>> > we're accepting this packet.
>> > 
>> > If the conntrack from the unconfirmed list is dying, then the object
>> > will be released by when the packet leaves the stack to its
>> > destination. With stateful filtering depending in place, the follow up
>> > packet in the reply direction will likely be considered invalid (if
>> > tcp tracking is on). Fortunately for us, the origin will likely
>> > retransmit the syn again, so the ct will be setup accordingly.
>> > 
>> > So, why should we allow this to go through?
> True, it also seems strange to me that we accept this packet.

The raise was triggered in a scenario when we tested high-load IPsec
tunnels and flushed the conntrack hashs from userspace.

For me there is little difference in choosing DROP or ACCEPT as verdict.
The packet/skb belongs to a formerly allowed connection, most likely
this connection is still allowed (but the conntrack hash entry is about
to be removed due to userspace is flushing the conntrack table).

To minimize the impact (lost packets -> retransmit) I decided to allow
the skb in flight, so were is no lost packet at this place.

When the connection is not allowed anymore (but was allowed up to now,
because the hash entry exists), the impact is one last packet 'slipping
through'.

Today I would still decide the way I did in 2010.

> 
>> > This return verdict was introduced in: fc35077 ("netfilter:
>> > nf_conntrack: fix a race in __nf_conntrack_confirm against
>> > nf_ct_get_next_corpse()") btw.
> And the commit does not argue why NF_ACCEPT was chosen...
> 
> -- 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

best regards
Jörg Marx

-- 

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Pablo Neira Ayuso]

On Wed, Nov 12, 2014 at 11:57:31AM +0100, Jörg Marx wrote:
> On 12.11.2014 08:35, Jesper Dangaard Brouer wrote:
> 
> Hi,
> 
> I wrote the patch in 2010, so find some arguments below:
> 
> >>> > > +	nf_ct_del_from_dying_or_unconfirmed_list(ct);
> >>> > >  
> >>> > >  	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;
> >> > 
> >> > Not directly related to your patch, but I don't find a good reason why
> >> > we're accepting this packet.
> >> > 
> >> > If the conntrack from the unconfirmed list is dying, then the object
> >> > will be released by when the packet leaves the stack to its
> >> > destination. With stateful filtering depending in place, the follow up
> >> > packet in the reply direction will likely be considered invalid (if
> >> > tcp tracking is on). Fortunately for us, the origin will likely
> >> > retransmit the syn again, so the ct will be setup accordingly.
> >> > 
> >> > So, why should we allow this to go through?
> > True, it also seems strange to me that we accept this packet.
> 
> The raise was triggered in a scenario when we tested high-load IPsec
> tunnels and flushed the conntrack hashs from userspace.
> 
> For me there is little difference in choosing DROP or ACCEPT as verdict.
> The packet/skb belongs to a formerly allowed connection, most likely
> this connection is still allowed (but the conntrack hash entry is about
> to be removed due to userspace is flushing the conntrack table).

__nf_conntrack_confirm() is only called for the first packet that we
see in a flow. If you just invoked the flush command 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).

> To minimize the impact (lost packets -> retransmit) I decided to allow
> the skb in flight, so were is no lost packet at this place.

I understand your original motivation was to be conservative.

> When the connection is not allowed anymore (but was allowed up to now,
> because the hash entry exists), the impact is one last packet 'slipping
> through'.

The general policy in conntrack is to not drop packets, but in this
case we'll leave things in inconsistent state (ie. we will likely
receive a reply packet in response to the original packet that has no
conntrack yet).

Thanks.

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Jörg Marx]

On 13.11.2014 13:08, Pablo Neira Ayuso wrote:

>> For me there is little difference in choosing DROP or ACCEPT as verdict.
>> > The packet/skb belongs to a formerly allowed connection, most likely
>> > this connection is still allowed (but the conntrack hash entry is about
>> > to be removed due to userspace is flushing the conntrack table).
> __nf_conntrack_confirm() is only called for the first packet that we
> see in a flow. If you just invoked the flush command 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).
Yes, you are right. As far as I remember it was very hard to trigger
that critical moment, when the first packet triggered the insertion into
the hash table. But the test and production systems showed this strange
behaviour, that no traffic was allowed to flow for exactly 600 seconds.

> 
>> > To minimize the impact (lost packets -> retransmit) I decided to allow
>> > the skb in flight, so were is no lost packet at this place.
> I understand your original motivation was to be conservative.
Yes.

> 
>> > When the connection is not allowed anymore (but was allowed up to now,
>> > because the hash entry exists), the impact is one last packet 'slipping
>> > through'.
Feel free to change the verdict, IMHO it doesn't matter at all as long
as the hash table is in a consistent state. The higher protocol layers
will deal with the missing packet.

> The general policy in conntrack is to not drop packets, but in this
> case we'll leave things in inconsistent state (ie. we will likely
> receive a reply packet in response to the original packet that has no
> conntrack yet).
Under heavy load this can happen anyway I guess?

Thanks and best regards
Jörg

-- 
Dipl.-Inform.
Jörg Marx
Bereichsleiter
Entwicklung Client- & Netzwerksicherheit
Geschäftsbereich Public Sector
secunet Security Networks AG
Ammonstr. 74
D-01067 Dresden, Germany 
Telefon +49 201 54 54-3517
Telefax +49 201 54 54-1323
joerg.marx@secunet.com
www.secunet.com

secunet Security Networks AG
Kronprinzenstr. 30
45128 Essen, Germany 
Amtsgericht Essen HRB 13615

Vorstand: Dr. Rainer Baumgart (Vors.), Thomas Pleines
Aufsichtsratsvorsitzender: Dr. Peter Zattler
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: conntrack: fix race in __nf_conntrack_confirm against get_next_corpse

[Pablo Neira Ayuso]

On Wed, Nov 12, 2014 at 08:35:00AM +0100, Jesper Dangaard Brouer wrote:
> > > -	/* We have to check the DYING flag inside the lock to prevent
> > > +
> > > +	/* 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 */
> > 
> > While at this, I think it would be good to fix comment style to:
> > 
> >         /* We have ...
> >          * ...
> >          */
> > 
> > I can fix this here, no need to resend, just let me know.
> 
> Okay, I was just trying to keep the changes as minimal as possible, if
> this should go into a stable-kernel.  Your choice.

I'm going to take this patch including the comment style fix, I would
like to avoid specific patches to fix coding style issues, and the
first line of this comment is updated. I think the patch will be still
small to fulfill -stable rules.

I'll send a follow a patch to change the return verdict to NF_DROP to
not mix up different things.

Thanks!