Re: [PATCH 6/6] net: move qdisc ingress filtering on top of netfilter ingress hooks

[Patrick McHardy <kaber@trash.net>]

On 29.04, Jamal Hadi Salim wrote:
> On 04/29/15 21:43, Patrick McHardy wrote:
> >On 30.04, Daniel Borkmann wrote:
> >>On 04/30/2015 02:37 AM, Patrick McHardy wrote:
> >>>On 30.04, Pablo Neira Ayuso wrote:
> 
> >>Totally agree with you that the situation is quite a mess. From tc ingress/
> >>egress side, at least my use case is to have an as minimal as possible entry
> >>point for cls_bpf/act_bpf, which is what we were working on recently. That
> >>is rather ``fresh'' compared to the remaining history of cls/act in tc.
> >
> >It's more than a mess. Leaving aside the fully broken code at ingress,
> >just look at the TC action APIs. Its "a failed state".
> 
> Since youve repeated about 100 that tc api being broken, maybe
> you can explain more rationally? By that i mean dont use words
> like words like "crap" or "failed state" or no chest-thumping.
> Lets say we totally stopped trying to reuse netfilter code, what are
> you talking about?
> 
> I think there is confusion about usability vs merits of performance.

I noticed tons of semantical holes when actually working on this, but
let me just give a single example which is still on the top of my head:

tcf_action_init:

        for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {                      
	        act = tcf_action_init_1(net, tb[i], est, name, ovr, bind);

So the API allows up to *magic number* (32) actions in one list, but not
more. So many will be atomic at least from a rtnetlink perspective, but
certainly not from a packet processing perspecting, from that one, they
will simply be processed as they are instantiated. So, we simulate
atomicity (what netlink is about) for a magic number of elements, but
do not even provide it at runtime.

Even ignoring the harder problem of having transactional semantics for
multiple actions, why even support *magic number* of elements in one
message and pretend atomicity? Smells a lot like premature optimization,
ignoring sementical expectations.

And yes, I do think it breaks with every concept of rtnetlink (messages
are atomic) for no reason at all. I remember TC actions where full of
these "special" interpretations. If you insist, I can do a review again,
but I did all that and stated it years ago.

Regarding TC as a whole, I think the problem is shared between userspace
and the kernel. iproute TC is certainly completely failed, its unusable
without looking at the kernel and iproute code, it hasn't even made the
slightest infrastructrual progess in the past 15 years (*(u16)RTN_DATA(bla))?)
and that is telling for itself. I don't extend that to ip, even though it
suffers from the same coding problems, but let's be honest, do you really
expect some magic is going to happen and TC userspace will suddenly become
usable?

I don't. And we intend to provide an alternative.