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]

Date:   Mon, 22 Aug 2016 16:51:35 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     David Ahern <dsa@...ulusnetworks.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        buytenh@...tstofly.org, ebiederm@...ssion.com,
        rshearma@...cade.com, tom@...bertland.com, tgraf@...g.ch,
        olivier.dugeon@...nge.com, alexander.duyck@...il.com,
        roopa@...ulusnetworks.com, Pravin B Shelar <pshelar@...ira.com>
Subject: Re: [PATCH net-next 2/3] net: mpls: Fixups for GSO

On Mon, Aug 22, 2016 at 07:11:27AM -0600, David Ahern wrote:
> On 8/22/16 6:21 AM, Simon Horman wrote:
> >> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> >> index 1ecbd7715f6d..6d78f162a88b 100644
> >> --- a/net/openvswitch/actions.c
> >> +++ b/net/openvswitch/actions.c
> >> @@ -167,6 +167,12 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> >>  		skb->mac_len);
> >>  	skb_reset_mac_header(skb);
> >>  
> >> +	/* for GSO: set MPLS as network header and encapsulated protocol
> >> +	 * header as inner network header
> >> +	 */
> >> +	skb_set_network_header(skb, skb->mac_len);
> >> +	skb_set_inner_network_header(skb, skb->mac_len + MPLS_HLEN);
> >> +
> >>  	new_mpls_lse = (__be32 *)skb_mpls_header(skb);
> >>  	*new_mpls_lse = mpls->mpls_lse;
> > 
> > Is the above calculation correct if push_mpls() is called multiple times?
> > 
> 
> No. Does OVS support more than 1? I really need someone who is familiar with the OVS code to make sure it works for all use cases. e.g., set skb_set_inner_network_header() before pushing a series of MPLS labels.

Yes that is supported.

The scheme that OvS uses so far is that mac_len denotes the number of bytes
from the start of the MAC header until its end. In the absence of MPLS that
will be the beginning of the network header. And in the presence of MPLS it
will be the beginning of the MPLS label stack. The network header is... the
network header. This allows the MAC header, MPLS label stack and network
header to be tracked.

Pravin (CCed) may have different ideas but I wonder if the above scheme can
be preserved while also meeting the needs of your new MPLS GSO scheme if
you set skb_set_network_header() and skb_set_inner_network_header() in
net/openvswitch/actions.c:do_output().

It may also be possible to teach OvS to use skb_set_network_header to
denote the beginning of the MPLS LSE and skb_set_inner_network_header to
denote the network header in the presence of MPLS. Which is my current
understanding of what you are trying to achieve. But I think its likely
that I misunderstand things as it seems strange to me to pretend that an
MPLS LSE is a network header and the outer most network header is an inner
network header