netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Florian Westphal <fw@strlen.de>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Pablo Neira Ayuso <pablo@netfilter.org>,
	netfilter-devel@vger.kernel.org, davem@davemloft.net,
	netdev@vger.kernel.org, kaber@trash.net, jhs@mojatatu.com
Subject: Re: [PATCH 0/4] Netfilter ingress support (v3)
Date: Mon, 4 May 2015 18:19:56 +0200	[thread overview]
Message-ID: <20150504161956.GK22481@breakpoint.cc> (raw)
In-Reply-To: <20150504155639.GA14367@Alexeis-MBP.westell.com>

Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > Another round of the patchset to add Netfilter ingress support. This new
> > patchset introduces the necessary updates in 2 steps:
> > 
> > 1) Add minismalistic ingress hook infrastructure that allows to register one
> >    client at a time, so you hit -EBUSY in case the hook is in use. Basically,
> >    we have a function pointer that is rcu-protected to invoke the corresponding
> >    filter framework which has minimal performance impact in the critical ingress
> >    path and avoid more pollution in it. This patch also ports the ingress qdisc
> >    on top of this.
> ...
> > In summary, this provides the facility to keep both tc and netfilter in place,
> > while the user can select what they prefer to filter from ingress.
> 
> wow, I have to say I'm impressed. That's the most genius way to
> really kill TC.
> Patch 1 looks good, patch 2,3,4 are nicely building on top...
> until somebody starts asking how patch 5 will look.
> In the future netfilter ingress module will be loaded along with
> other iptables modules just like conntrack is today and users
> who would want to use ingress tc would have to _unload_
> netfilter_ingress module, but if it has interesting dependencies
> it may mean to unload iptables and the rest.

FWIW while I think this is a valid concern, I believe its unfounded.

netfilter_ingress must not force run-time
dependencies like 'oh, you want tc, too bad, no conntrack for you)'.

(and i don't see any need for such a dependency).

> So at the end the users will have a binary choice either to use
> iptables/nft or use tc, because they won't be able to co-exist
> because ingress_hook is the only one.
> I don't understand this 'tc hate'. Why go out of the way to
> make TC more difficult to use ?

I don't see this having anything to do with 'TC hate'.

Lets face it, tc and netfilter are horrid when it comes to play
nice with each other, and functionality provided by both partially
overlaps.

Just look at all the hacks we have in tc to use netfilter (iptables,
conntrack) functionality, or even iptables CLASSIFY target (aka
"tc filters are too hard for me").

In case there is later some really good reason why you need both
tc ingress and nft ingress simultaneously then we can always revisit this
and add a second hook for coexistence, at the expense of more
complexity.

> Just add _new_ hook for netfilter ingress and both subsystems
> can happily co-exist.

I don't see a use case where you'd need both tc ingress and nft
ingress at the same time.  Can you elaborate?

At the moment I think having two hooks provides no advantage, it only
complicates code.

  reply	other threads:[~2015-05-04 16:19 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-04 10:50 [PATCH 0/4] Netfilter ingress support (v3) Pablo Neira Ayuso
2015-05-04 10:50 ` [PATCH 1/4] net: add minimalistic ingress filter hook and port sch_ingress on top of it Pablo Neira Ayuso
2015-05-04 10:50 ` [PATCH 2/4] netfilter: cleanup struct nf_hook_ops indentation Pablo Neira Ayuso
2015-05-04 10:50 ` [PATCH 3/4] netfilter: add hook list to nf_hook_state Pablo Neira Ayuso
2015-05-04 10:50 ` [PATCH 4/4] net: add netfilter ingress hook Pablo Neira Ayuso
2015-05-04 15:56 ` [PATCH 0/4] Netfilter ingress support (v3) Alexei Starovoitov
2015-05-04 16:19   ` Florian Westphal [this message]
2015-05-04 17:21     ` Jamal Hadi Salim
2015-05-04 17:43       ` Florian Westphal
2015-05-04 18:47         ` Jamal Hadi Salim
2015-05-04 18:59           ` Florian Westphal
2015-05-04 20:05             ` Alexei Starovoitov
2015-05-04 22:21               ` Pablo Neira Ayuso
2015-05-04 23:04                 ` Thomas Graf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150504161956.GK22481@breakpoint.cc \
    --to=fw@strlen.de \
    --cc=alexei.starovoitov@gmail.com \
    --cc=davem@davemloft.net \
    --cc=jhs@mojatatu.com \
    --cc=kaber@trash.net \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).