netfilter: ctnetlink: make it safer when updating ct->status
authorLiping Zhang <zlpnobody@gmail.com>
Mon, 17 Apr 2017 13:18:57 +0000 (21:18 +0800)
committerBen Hutchings <ben@decadent.org.uk>
Sat, 26 Aug 2017 01:14:03 +0000 (02:14 +0100)
commit883a0ef402e5ed967af0e5b83aad95b8703fd0de
tree79e4c0e683318180daee1b2dd10bed22e8f5f695
parent85763352fb2513a415a4d12208832f1c9012e410
netfilter: ctnetlink: make it safer when updating ct->status

commit 53b56da83d7899de375a9de153fd7f5397de85e6 upstream.

After converting to use rcu for conntrack hash, one CPU may update
the ct->status via ctnetlink, while another CPU may process the
packets and update the ct->status.

So the non-atomic operation "ct->status |= status;" via ctnetlink
becomes unsafe, and this may clear the 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!

Now using a series of atomic bit operation to solve the above issue.

Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly,
so make these two bits be unchangable too.

If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free,
but actually it is alloced by nf_conntrack_alloc.
If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
deference, as the nfct_seqadj(ct) maybe NULL.

Last, add some comments to describe the logic change due to the
commit a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
processing"), which makes me feel a little confusing.

Fixes: 76507f69c44e ("[NETFILTER]: nf_conntrack: use RCU for conntrack hash")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
[bwh: Backported to 3.2:
 - IPS_UNCHANGEABLE_MASK was not previously defined and ctnetlink_update_status()
   is not needed
 - enum ip_conntrack_status only assigns 13 bits
 - Adjust filename]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
include/linux/netfilter/nf_conntrack_common.h
net/netfilter/nf_conntrack_netlink.c