Hi Pablo,
2017-04-14 7:25 GMT+08:00 Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>:
> On Thu, Apr 13, 2017 at 08:53:47PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@xxxxxxxxx>
>>
>> User can update the ct->status via nfnetlink, but using a non-atomic
>> operation "ct->status |= status;". This is unsafe, and may clear
>> IPS_DYING_BIT bit set by another CPU unexpectedly. For example:
>> CPU0 CPU1
>> ctnetlink_change_status __nf_conntrack_find_get
>> old = ct->status nf_ct_gc_expired
>> - nf_ct_kill
>> - test_and_set_bit(IPS_DYING_BIT
>> new = old | status; -
>> ct->status = new; <-- oops, _DYING_ is cleared!
>
> This is fixing an issue that was introduced in ca7433df3a67.
Actually, even holding the global nf_conntrack_lock, this won't help anything.
Because data processing path is protected by rcu, so they can manipulate
the same _ct_ at the same time.
After having a close investigation, I found the above issue was
introduced by commit 76507f69c44e ("[NETFILTER]: nf_conntrack: use RCU
for conntrack hash"):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=76507f69c44ed199a1a68086145398459e55835d
The commit ca7433df3a67 ("netfilter: conntrack: seperate expect
locking from nf_conntrack_lock")
only introduced the dead lock indeed.
> So I would like this comes in a patch batch including the several
> patches that we need to fix the conntrack update path from ctnetlink.
OK, I will try to send a patch set.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
