mbox series

[nf-next,0/4] netfilter: nf_queue: rework refcount handling

Message ID 20200327022449.7411-1-fw@strlen.de
Headers show
Series netfilter: nf_queue: rework refcount handling | expand

Message

Florian Westphal March 27, 2020, 2:24 a.m. UTC 
running nft_queue.sh selftest with refcount debugging
enabled triggers following splat:

------------[ cut here ]------------
refcount_t: addition on 0; use-after-free.
WARNING: CPU: 0 PID: 2441 at lib/refcount.c:25 refcount_warn_saturate+0xbc/0x110
RIP: 0010:refcount_warn_saturate+0xbc/0x110
[..]
Call Trace:
 nf_queue_entry_get_refs+0x194/0x1b0
 nf_queue+0x38b/0x640
 nf_reinject+0x264/0x280
 nfqnl_recv_verdict+0x5d5/0x920
 nfnetlink_rcv_msg+0x27a/0x460

This is because nf_queue uses following pattern:
nf_queue_entry_get_refs()
  nf_queue() // leave rcu protection
  // nfnetlink, wait for verdict
  // sk might be closed now
nf_reinject // reenter rcu protection
  nf_queue_entry_release_refs // refcount can drop to 0
  // iterate/call remaining hooks and okfn

If the hook iteration results in another nf_queue() call, above splat
might be triggered.

This series fixes this by deferring the call to
nf_queue_entry_release_refs() until after the hook iteration/okfn
returns; i.e. another nf_queue invocation from nf_reinject path will
not observe a zero refcount.

This series also applies to nf, but given we're a bit further along in
release cycle nf-next might be better; this fix isn't simple,
unfortunately.

Comments

Pablo Neira Ayuso March 29, 2020, 3:07 p.m. UTC | #1 
On Fri, Mar 27, 2020 at 03:24:45AM +0100, Florian Westphal wrote:
[...]
> This series fixes this by deferring the call to
> nf_queue_entry_release_refs() until after the hook iteration/okfn
> returns; i.e. another nf_queue invocation from nf_reinject path will
> not observe a zero refcount.

Applied.