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

[Bill Bonaparte]

On Tue, 6 Nov 2014 21:01:00 
"Jesper" <brouter@redhat.com> wrote:
>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.

Thanks, Jesper. This is my first time to submit a patch, not know much about
the rules.  I will get it soon.

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

In most cases, we do the atomic operation on ct->status (with set_bit), but
in function nf_nat_setup_info, we
assume that unconfirmed ct is always holded by current cpu, and has no race
against other cpus, so we don't
use set_bit.  
the following code is extracted from the nf_nat_setup_info:
/* Non-atomic: we own this at the moment. */
  if (maniptype == NF_NAT_MANIP_SRC)
	ct->status |= IPS_SRC_NAT;
  else
	ct->status |= IPS_DST_NAT;

--
Best regards,
  Bill Bonaparte

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

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

[billbonaparte]

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

Jesper Dangaard Brouer <brouer@redhat.com> wrote:
>> In function __nf_conntrack_confirm, we check the conntrack if it was 
>> already dead, before insert it into hash-table.
>> We do this because if we insert an already 'dead' hash,  it will 
>> block further use of that particular connection.

>Have you run into this problem in practice, or is this based on a theory?

If we insert a dead conntrack into hash-table, let's see what will happen to
the packet which has the same tuple with that dead conntrack:
1) if it is a valid packet, it will enter in nf_conntrack_in hook, then
2) we will find a corresponding conntrack for that packet in
__nf_conntrack_find_get, and there is no doubt that we will find that dead
conntrack,
  according to the current implement, we don't use dead conntrack, so we
create a new conntrack which is unconfirmed.
3) because there is already a conntrack with the same tuples in hash-table,
the new conntrack can not be inserted into hash-table, it will return
NF_DROP,
4) the packet will be dropped due to the failure of nf_conntrack_confirm.
That's why inserting an already 'dead' hash will block further use of that
particular connection.

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

[billbonaparte]

Hi, all:
	sorry for sending this mail again, the last mail doesn't show text
clearly.
	In function __nf_conntrack_confirm, we check the conntrack if it was
alreay dead, before insert it into hash-table. 
	we do this because if we insert an already 'dead' hash,  it will
block further use of that particular connection.
	but we don't do that right.
    let's consider the following case:
	
	cpu1                                              cpu2
__nf_conntrack_confirm                          get_next_corpse
   lock corresponding hash-list                        ....
   check nf_ct_is_dying(ct)
for_each_possible_cpu(cpu) {
		......
spin_lock_bh(&pcpu->lock);
		......
set_bit(IPS_DYING_BIT, &ct->status);
   nf_ct_del_from_dying_or_unconfirmed_list(ct);
spin_unlock_bh(&pcpu_lock);
   add_timer(&ct->timeout);                          }	
   ct->status |= IPS_CONFIRMD;
   __nf_conntrack_hash_insert(ct);   /* the conntrack has been seted as
dying*/


	The above case reveal two problems:
	1. we may insert a dead conntrack to hash-table, it will block
further use of that particular connection.
	2. operation on ct->status should be atomic, because it race aginst
get_next_corpse.
	  due to this reason, the operation on ct->status in
nf_nat_setup_info should be atomic as well.

	if we want to resolve the first problem, we must delete the
unconfirmed conntrack from unconfirmed-list first, then check if it is
already dead.
	Am I right to do this ?
	Appreciate any comments and reply.

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

[Florian Westphal]

billbonaparte <programme110@gmail.com> wrote:
> 	In function __nf_conntrack_confirm, we check the conntrack if it was
> alreay dead, before insert it into hash-table. 
> 	we do this because if we insert an already 'dead' hash,  it will
> block further use of that particular connection.
> 	but we don't do that right.

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, and add

Fixes: 93bb0ceb75be2 (netfilter: conntrack: remove central spinlock nf_conntrack_lock)

tag to patch.

> 	The above case reveal two problems:
> 	1. we may insert a dead conntrack to hash-table, it will block
> further use of that particular connection.

Yes.

> 	2. operation on ct->status should be atomic, because it race aginst
> get_next_corpse.

Alternatively we could also get rid of the unconfirmed list handling in
get_next_corpse, it looks to me as if its simply not worth the trouble
to also caring about unconfirmed lists.

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

[Jesper Dangaard Brouer]

On Tue, 28 Oct 2014 11:37:31 +0800 "billbonaparte" <programme110@gmail.com> wrote:

> Hi, all:
> sorry for sending this mail again, the last mail doesn't show text
> clearly.

This one also mangles the text, so I cannot follow the race you are
describing.  I'll try to reconstruct...

> In function __nf_conntrack_confirm, we check the conntrack if it was
> already dead, before insert it into hash-table. 
> We do this because if we insert an already 'dead' hash,  it will
> block further use of that particular connection.

Have you run into this problem in practice, or is this based on a
theory?

> but we don't do that right.
> let's consider the following case:
> 
[tried to reconstruct]

> 	cpu1                             cpu2
> __nf_conntrack_confirm             get_next_corpse
>   lock corresponding hash-list      ....
>   check nf_ct_is_dying(ct)          ....
>    .....                           for_each_possible_cpu(cpu) {
>    .....                           (processing &pcpu->unconfirmed)
>    .....                           spin_lock_bh(&pcpu->lock);
>    .....                           set_bit(IPS_DYING_BIT, &ct->status);
>    .....                           spin_unlock_bh(&pcpu_lock);
>  spin_lock_bh(&pcpu->lock);
>  nf_ct_del_from_dying_or_unconfirmed_list(ct);
>  spin_unlock_bh(&pcpu_lock);
>
>  add_timer(&ct->timeout);
>  ct->status |= IPS_CONFIRMED;
>  __nf_conntrack_hash_insert(ct);
>   /* the conntrack has been seted as dying*/

Yes, I think you are correct.  There is a race.  As we are modifying
the ct->status, without holding the hash bucket lock.


> The above case reveal two problems:
> 	1. we may insert a dead conntrack to hash-table, it will block
> further use of that particular connection.
> 	2. operation on ct->status should be atomic, because it race aginst
> get_next_corpse.
> 	  due to this reason, the operation on ct->status in
> nf_nat_setup_info should be atomic as well.
> 
> 	if we want to resolve the first problem, we must delete the
> unconfirmed conntrack from unconfirmed-list first, then check if it is
> already dead.

Guess that would be one approach.

> 	Am I right to do this ?
> 	Appreciate any comments and reply.

Perhaps we could get rid of unconfirmed list handling in get_next_corpse?

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

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

[billbonaparte]

Hi, all:
	In function __nf_conntrack_confirm, we check the conntrack if it was
alreay dead, before insert it into hash-table. 
	we do this because if we insert an already 'dead' hash,  it will
block further use of that particular connection.
	but we don't do that right.
    let's consider the following case:
	
	cpu1
cpu2
	__nf_conntrack_confirm
get_next_corpse
   		lock corresponding hash-list
....
		check nf_ct_is_dying(ct)
for_each_possible_cpu(cpu) {
		......
spin_lock_bh(&pcpu->lock);
		......
set_bit(IPS_DYING_BIT, &ct->status);
		nf_ct_del_from_dying_or_unconfirmed_list(ct);
spin_unlock_bh(&pcpu_lock);
		add_timer(&ct->timeout);
}	
		ct->status |= IPS_CONFIRMD;
		__nf_conntrack_hash_insert(ct);


	
	The above case reveal two problems:
	1. we may insert a dead conntrack to hash-table, it will block
further use of that particular connection.
	2. operation on ct->status should be atomic, because it race aginst
get_next_corpse.
	  due to this reason, the operation on ct->status in
nf_nat_setup_info should be atomic as well.

	if we want to resolve the first problem, we must delete the
unconfirmed conntrack from unconfirmed-list first, then check if it is
already dead.
	Am I right to do this ?
	Appreciate any comments and reply.