Messages in this thread | | | From | "billbonaparte" <> | Subject | Re: netfilter: nf_conntrack: there maybe a bug in __nf_conntrack_confirm, when it race against get_next_corpse | Date | Tue, 4 Nov 2014 09:48:32 +0800 |
| |
(sorry to send this e-mail again, last mail is rejected by server due to non-acceptable content)
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.
>> 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.
yes, I think so. 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. but, any case seems to be okay. [unhandled content-type:application/octet-stream] | |