netdev - Re: [ovs-dev] [PATCH net-next v11 5/6] openvswitch: add layer 3 flow/port support

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: <20160720000243.GA4688@penelope.isobedori.kobe.vergenet.net>
Date:	Wed, 20 Jul 2016 09:02:45 +0900
From:	Simon Horman <simon.horman@...ronome.com>
To:	pravin shelar <pshelar@....org>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	ovs dev <dev@...nvswitch.org>, Jiri Benc <jbenc@...hat.com>
Subject: Re: [ovs-dev] [PATCH net-next v11 5/6] openvswitch: add layer 3
 flow/port support

On Mon, Jul 18, 2016 at 03:34:52PM -0700, pravin shelar wrote:
> On Sun, Jul 17, 2016 at 9:50 PM, Simon Horman
> <simon.horman@...ronome.com> wrote:
> > [CC Jiri Benc for portion regarding GRE]
> >
> > Hi Pravin,
> >
> > On Fri, Jul 15, 2016 at 02:07:37PM -0700, pravin shelar wrote:
> >> On Wed, Jul 13, 2016 at 12:31 AM, Simon Horman
> >> <simon.horman@...ronome.com> wrote:
> >> > Hi Pravin,
> >> >
> >> > On Thu, Jul 07, 2016 at 01:54:15PM -0700, pravin shelar wrote:
> >> >> On Wed, Jul 6, 2016 at 10:59 AM, Simon Horman
> >> >> <simon.horman@...ronome.com> wrote:
> >> >
> >> > ...
> >>
> >> >
> >> >> > diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> >> >> > index 0ea128eeeab2..86f2cfb19de3 100644
> >> >> > --- a/net/openvswitch/flow.c
> >> >> > +++ b/net/openvswitch/flow.c
> >> >> ...
> >> >>
> >> >> > @@ -723,9 +729,17 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
> >> >> >         key->phy.skb_mark = skb->mark;
> >> >> >         ovs_ct_fill_key(skb, key);
> >> >> >         key->ovs_flow_hash = 0;
> >> >> > +       key->phy.is_layer3 = skb->mac_len == 0;
> >> >>
> >> >> I do not think mac_len can be used. mac_header needs to be checked.
> >> >> ...
> >> >
> >> > Yes, indeed. The update to use skb_mac_header_was_set() here accidently
> >> > slipped into the following patch, sorry about that.
> >> >
> >> > With that change in place I believe that this patch is internally
> >> > consistent because mac_header and mac_len are set correctly by the
> >> > call to key_extract() which is called by ovs_flow_key_extract() just
> >> > after where the excerpt above ends.
> >> >
> >> > That said, I do think that it is possible to rely on skb_mac_header_was_set
> >> > throughout the datapath, including action processing etc... I have provided
> >> > an incremental patch - which I created on top of this entire series - at
> >> > the end of this email. If you prefer that approach I am happy to take it,
> >> > though I do feel that using mac_len leads to slightly cleaner code. Let me
> >> > know what you think.
> >> >
> >>
> >>
> >> I am not sure if you can use only mac_len to detect L3 packet. This
> >> does not work with MPLS packets, mac_len is used to account MPLS
> >> headers pushed on skb. Therefore in case of a MPLS header on L3
> >> packet, mac_len would be non zero and we have to look at either
> >> mac_header or some other metadata like is_layer3 flag from key to
> >> check for L3 packet.
> >
> > At least within OvS mac_len does not include the length of the MPLS label
> > stack. Rather, the MPLS label stack length is the difference between the
> > end of (mac_header + mac_len) and network_header.
> >
> > So I think that the scheme does work as mac_len is 0 if there is no L2
> > header regardless of if an MPLS label stack is present or not.
> >
> 
> I was thinking in overall networking stack rather than just ovs
> datapath. I think we should have consistent method of detecting L3
> packet. As commented in previous mail it could be achieved using
> skb-protocol and device type.

This is somewhat of a surprise to me. As far as I recall when MPLS support
was added to OvS it and the accompanying support for MPLS GSO was the only
MPLS support present in the kernel. And at the time the scheme developed by
Jesse Gross, myself and others was as I describe above.

Internally OvS relies on this scheme and in particular it is used
by skb_mpls_header() to calculate the beginning of the MPLS label stack
accurately in the presence of VLAN tags.

Is it mpls_gso_segment() that you are concerned about?
If so, perhaps the problem could be addressed there.