netdev - Re: [PATCH net] sched: consistently handle layer3 header accesses in the presence of VLANs

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] [day] [month] [year] [list]

Message-ID: <873668ekbj.fsf@toke.dk>
Date:   Fri, 03 Jul 2020 16:37:20 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Davide Caratti <dcaratti@...hat.com>, davem@...emloft.net
Cc:     netdev@...r.kernel.org, cake@...ts.bufferbloat.net,
        Jiri Pirko <jiri@...nulli.us>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Ilya Ponetayev <i.ponetaev@...systems.com>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [PATCH net] sched: consistently handle layer3 header accesses in the presence of VLANs

Davide Caratti <dcaratti@...hat.com> writes:

> hello Toke,
>
> thanks for answering!
>
> On Fri, 2020-07-03 at 14:05 +0200, Toke Høiland-Jørgensen wrote:
>>   while (proto == htons(ETH_P_8021Q) || proto == htons(ETH_P_8021AD)) {
>
> maybe this line be shortened, since if_vlan.h has [1]:
>
> while (eth_type_vlan(proto)) {
>  	...
> }

Good point, missed that! Will fix and send a v2.

> If I read well, the biggest change from functional point of view is that
> now qdiscs can set the ECN bit also on non-accelerated VLAN packets and
> QinQ-tagged packets, if the IP header is the outer-most header after VLAN;
> and the same applies to almost all net/sched former users of skb->protocol 
> or tc_skb_protocol().

Yup, that's the idea.

> Question (sorry in advance because it might be a dumb one :) ):
>
> do you know why cls_flower, act_ct, act_mpls and act_connmark keep reading
> skb->protocol? is that intentional?

Hmm, no not really. I only checked for calls to tc_skb_protocol(), not
for direct uses of skb->protocol. Will fix those as well :)

> (for act_mpls that doesn't look intentional, and probably the result is
> that the BOS bit is not set correctly if someone tries to push/pop a label
> for a non-accelerated or QinQ packet. But I didn't try it experimentally
> :) )

Hmm, you're certainly right that the MPLS code should use the helper to
get consistent use between accelerated/non-accelerated VLAN usage. But I
don't know enough about MPLS to judge whether it should be skipping the
VLAN tags or not. Sounds like you're saying the right thing is to skip
the VLAN tags there as well?

Looking at the others, it looks like act_connmark and act_ct both ought
to skip VLAN tags, while act_flower should probably keep it, since it
seems it has a VLAN match type. Or?

-Toke