nfqueue & bridge netfilter considered broken

* nfqueue & bridge netfilter considered broken
@ 2016-09-02  9:08 Florian Westphal
  2016-09-02  9:58 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 6+ messages in thread
From: Florian Westphal @ 2016-09-02  9:08 UTC (permalink / raw)
  To: netfilter-devel

Hi.

This is a note to summarize state of bridge + br_netfilter + nfqueue.

TL;DR: I am giving up.  I see no way to fix this in a sane fashion.

What I tried:
I - discard extra nfct entry when cloning.  Works, but obviously not
 compatible in any way (the clones are INVALID).

II - add locking where needed.  It gets out of hand rather quickly,
 it is also pushed on everyone else rather than just those places
 that cause clones with unconfirmed conntrack entries (only places
 that i know of is mcast routing and the bridge).

III - serialize on reinject.
 Idea was to change nfqnl_recv_verdict() to (instead of nf_reinject() )
 call a wrapper, do_nf_reinject, which essentially does:

static void do_nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
{
#if IS_ENABLED(NF_CONNTRACK)
   struct nf_conn *ct = entry->skb->nfct;

  if (!ct || nf_ct_is_confirmed(ct)) {
       nf_reinject(entry, verdict);
       return 0;
  }

  if (skb_cloned(entry->skb)) {
      module_get(THIS_MODULE);
      push_to_work_queue_for_reinject(entry, verdict);
      return;
  }
#endif
  nf_reinject(entry, verdict);
}

The work queue then makes sure that we feed such skb one-by-one.

Aside from this being ugly as f there are two problems:
 1. skb_cloned() being true does NOT mean that we have to serialize, skb->nfct might
    be unique after all, i.e. we again end up pushing work on users/cases
    where this might not be needed at all.

 2. We can't know if all clones get queued via nfqueue, so we could have something like:
    a. clone is created
    b. this clone is sent to userspace for queueing
    c. another clone is created
    d. this clone is NOT queued to userspace and about to continue traversal
    e. at same time, userspace reinjects the older clone on other cpu
    -> we race again despite such 'serializer'.

This leaves option number IV:
 - when cloning in bridge, also make a full copy of ->nfct.
 This has 2 issues:
   1. dependency from bridge to conntrack (yuck)
   2. confirmation will find clash and toss the clones ...
    ... so we will need more clash resolution hacks (double-yuck).

At this point I am considering submitting a patch to
remove nfqueue support for bridge netfilter altogether.

^ permalink raw reply	[flat|nested] 6+ messages in thread