netdev - Re: [PATCH net-next] tc: cls_bpf: make ingress and egress qdiscs consistent

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 for Android: free password hash cracker in your pocket

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

Message-ID: <551F1177.7090902@plumgrid.com>
Date:	Fri, 03 Apr 2015 15:17:27 -0700
From:	Alexei Starovoitov <ast@...mgrid.com>
To:	Daniel Borkmann <daniel@...earbox.net>,
	"David S. Miller" <davem@...emloft.net>
CC:	Jiri Pirko <jiri@...nulli.us>, Jamal Hadi Salim <jhs@...atatu.com>,
	linux-api@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] tc: cls_bpf: make ingress and egress qdiscs
 consistent

On 4/3/15 3:10 PM, Daniel Borkmann wrote:
> On 04/03/2015 11:52 PM, Alexei Starovoitov wrote:
>> On 4/3/15 2:46 PM, Daniel Borkmann wrote:
>>> On 04/03/2015 11:16 PM, Alexei Starovoitov wrote:
>>>> BPF programs attached to ingress and egress qdiscs see inconsistent
>>>> skb->data.
>>>> For ingress L2 header is already pulled, whereas for egress it's
>>>> present.
>>>> That makes writing programs more difficult.
>>>> Make them consistent by pushing L2 before running the programs and
>>>> pulling
>>>> it back afterwards.
>>>> Similar approach is taken by skb_defer_rx_timestamp() which does
>>>> push/pull
>>>> before calling ptp_classify_raw()->BPF_PROG_RUN().
>>>>
>>>> Signed-off-by: Alexei Starovoitov <ast@...mgrid.com>
>>>
>>> Thanks for looking into this. This ends up going via ingress_enqueue(),
>>
>> yes.
>>
>>> right? Maybe it would be better to add a new netlink attribute for
>>> ingress qdisc there that sets a flag in ingress_qdisc_data to pull the
>>> header space before calling tc_classify() and restore it later on?
>>> So, it would be configurable from tc. Would that work?
>>
>> you mean a flag that will affect all classifiers? I'm not sure other
>> classifiers care. Noone complained for years. I think it would be
>> overdesign. Here the fix is trivial, which is my preference.
>
> But the 'defect' is actually on the ingress qdisc side, right, not
> the classifier itself ... so if we do this in the classifier, we add
> two extra branches to the output path, which would never be taken.
>
> Plus, other classifiers wanting to look into ethernet headers would
> then also need to pull from /within/ their classifier as well. What
> about classic BPF users?
>
> I don't think fixing this in ingress qdisc is overdesign, but rather
> the better place to fix it. ;)

Strongly disagree.
1. there shouldn't be a choice at all for bpf. Because not pulling l2
means it's bug.
2. adding a flag means adding it to iproute2 with default off and making
users forgetting it from time to time and have no way of knowing why
their programs all of a sudden stopped working.

classic falls under the same rules. It doesn't make sense at all to run
a program on packet without L2 header. It's very odd both for classic
and extended programs.

Two 'if' conditions in critical path is bogus argument, since these
checks would be there in ingress as well. Same critical path.

--
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