netdev - Re: [PATCH net v3] 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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 03 Jul 2020 14:35:38 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     toke@...hat.com
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org,
        cake@...ts.bufferbloat.net, dcaratti@...hat.com, jiri@...nulli.us,
        jhs@...atatu.com, xiyou.wangcong@...il.com, mrv@...atatu.com,
        brakmo@...com, i.ponetaev@...systems.com
Subject: Re: [PATCH net v3] sched: consistently handle layer3 header
 accesses in the presence of VLANs

From: Toke Høiland-Jørgensen <toke@...hat.com>
Date: Fri,  3 Jul 2020 22:26:43 +0200

> There are a couple of places in net/sched/ that check skb->protocol and act
> on the value there. However, in the presence of VLAN tags, the value stored
> in skb->protocol can be inconsistent based on whether VLAN acceleration is
> enabled. The commit quoted in the Fixes tag below fixed the users of
> skb->protocol to use a helper that will always see the VLAN ethertype.
> 
> However, most of the callers don't actually handle the VLAN ethertype, but
> expect to find the IP header type in the protocol field. This means that
> things like changing the ECN field, or parsing diffserv values, stops
> working if there's a VLAN tag, or if there are multiple nested VLAN
> tags (QinQ).
> 
> To fix this, change the helper to take an argument that indicates whether
> the caller wants to skip the VLAN tags or not. When skipping VLAN tags, we
> make sure to skip all of them, so behaviour is consistent even in QinQ
> mode.
> 
> To make the helper usable from the ECN code, move it to if_vlan.h instead
> of pkt_sched.h.
> 
> v3:
> - Remove empty lines
> - Move vlan variable definitions inside loop in skb_protocol()
> - Also use skb_protocol() helper in IP{,6}_ECN_decapsulate() and
>   bpf_skb_ecn_set_ce()
> 
> v2:
> - Use eth_type_vlan() helper in skb_protocol()
> - Also fix code that reads skb->protocol directly
> - Change a couple of 'if/else if' statements to switch constructs to avoid
>   calling the helper twice
> 
> Reported-by: Ilya Ponetayev <i.ponetaev@...systems.com>
> Fixes: d8b9605d2697 ("net: sched: fix skb->protocol use in case of accelerated vlan path")
> Signed-off-by: Toke Høiland-Jørgensen <toke@...hat.com>

Looks good, applied and queued up for -stable.

Thanks!