netdev - Re: [PATCH net-next 2/3] net: mpls: Fixups for GSO

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]

Message-ID: <20160926175622.3b00d478@griffin>
Date:   Mon, 26 Sep 2016 17:56:22 +0200
From:   Jiri Benc <jbenc@...hat.com>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     pravin shelar <pshelar@....org>,
        Simon Horman <simon.horman@...ronome.com>,
        Pravin B Shelar <pshelar@...ira.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>, buytenh@...tstofly.org,
        "Eric W. Biederman" <ebiederm@...ssion.com>, rshearma@...cade.com,
        tom@...bertland.com, Thomas Graf <tgraf@...g.ch>,
        olivier.dugeon@...nge.com,
        Alexander Duyck <alexander.duyck@...il.com>,
        roopa@...ulusnetworks.com
Subject: Re: [PATCH net-next 2/3] net: mpls: Fixups for GSO

On Wed, 24 Aug 2016 10:37:51 -0600, David Ahern wrote:
> Something like this should be able to handle multiple labels. The
> inner network header is set once and the outer one pointing to MPLS
> is adjusted each time a label is pushed:
> 
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 1ecbd7715f6d..0f37b17e3a73 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -162,10 +162,16 @@ static int push_mpls(struct sk_buff *skb,
> struct sw_flow_key *key, if (skb_cow_head(skb, MPLS_HLEN) < 0)
>                 return -ENOMEM;
> 
> +       if (!skb->inner_protocol) {
> +               skb_set_inner_network_header(skb, skb->mac_len);
> +               skb_set_inner_protocol(skb, skb->protocol);
> +       }
> +
>         skb_push(skb, MPLS_HLEN);
>         memmove(skb_mac_header(skb) - MPLS_HLEN, skb_mac_header(skb),
>                 skb->mac_len);
>         skb_reset_mac_header(skb);
> +       skb_set_network_header(skb, skb->mac_len);

Sorry for chiming in after a month. The code above got in
(48d2ab609b6bb), I'm currently looking at this and it looks very
suspicious to me.

After push_mpls, network_header points to the start of MPLS headers.
Which I understand was the point of this patch. However, push_mpls also
calls invalidate_flow_key. Meaning that, depending on actions, we may
end up calling key_extract soon after. And key_extract sets the network
header *after* the MPLS headers.

That means that on output, for otherwise identical packet,
network_header can point before or after MPLS headers based on what
actions happened to be executed (recirculation, mainly).

If I'm not misreading the code or missing something, this can't be
right.

mpls_gso_segment does not care, it resets the network_header anyway.
What about drivers? What is the correct behavior?

 Jiri