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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 30 Apr 2015 05:29:23 +0200
From:	Patrick McHardy <kaber@...sh.net>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	Daniel Borkmann <daniel@...earbox.net>,
	Pablo Neira Ayuso <pablo@...filter.org>,
	netfilter-devel@...r.kernel.org, davem@...emloft.net,
	netdev@...r.kernel.org
Subject: Re: [PATCH 6/6] net: move qdisc ingress filtering on top of
 netfilter ingress hooks

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html