* Re: netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse
[not found] ` <02f201cff260$8622e610$9268b230$@gmail.com>
@ 2014-10-28 10:11 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 3+ messages in thread
From: Jesper Dangaard Brouer @ 2014-10-28 10:11 UTC (permalink / raw)
To: billbonaparte
Cc: linux-kernel, 'Netfilter Developer Mailing List',
'Pablo Neira Ayuso', 'Patrick McHardy',
kadlec, davem, 'Changli Gao', 'Andrey Vagin',
brouer, netdev
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* re: netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse
@ 2014-11-07 6:47 Bill Bonaparte
0 siblings, 0 replies; 3+ messages in thread
From: Bill Bonaparte @ 2014-11-07 6:47 UTC (permalink / raw)
To: 'Jesper Dangaard Brouer'
Cc: fw, linux-kernel, 'Pablo Neira Ayuso',
'Patrick McHardy', kadlec, davem, 'Changli Gao',
'Andrey Vagin',
netfilter-devel, netdev
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse
[not found] <012601cff7d1$7ce2d620$76a88260$@gmail.com>
@ 2014-11-06 13:00 ` Jesper Dangaard Brouer
0 siblings, 0 replies; 3+ messages in thread
From: Jesper Dangaard Brouer @ 2014-11-06 13:00 UTC (permalink / raw)
To: billbonaparte
Cc: fw, linux-kernel, Pablo Neira Ayuso, Patrick McHardy, kadlec,
davem, Changli Gao, Andrey Vagin, brouer, netfilter-devel,
netdev
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-11-07 6:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <02ef01cff25f$29887f60$7c997e20$@gmail.com>
[not found] ` <02f201cff260$8622e610$9268b230$@gmail.com>
2014-10-28 10:11 ` netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse Jesper Dangaard Brouer
[not found] <012601cff7d1$7ce2d620$76a88260$@gmail.com>
2014-11-06 13:00 ` Jesper Dangaard Brouer
2014-11-07 6:47 Bill Bonaparte
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).