netdev - Re: [PATCH v2.58] datapath: Add basic MPLS support to kernel

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: <20140603040406.GA22191@verge.net.au>
Date:	Tue, 3 Jun 2014 13:04:08 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Jesse Gross <jesse@...ira.com>
Cc:	"dev@...nvswitch.org" <dev@...nvswitch.org>,
	netdev <netdev@...r.kernel.org>, Ravi K <rkerur@...il.com>,
	Joe Stringer <joe@...d.net.nz>, Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH v2.58] datapath: Add basic MPLS support to kernel

Hi Jesse,

thanks for your feedback.

On Mon, Jun 02, 2014 at 05:58:10PM -0700, Jesse Gross wrote:
> On Sun, May 25, 2014 at 5:22 PM, Simon Horman <horms@...ge.net.au> wrote:
> > diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> > index 803a94c..8ce596c 100644
> > --- a/datapath/flow_netlink.c
> > +++ b/datapath/flow_netlink.c
> > +               case OVS_ACTION_ATTR_POP_MPLS:
> > +                       if (!eth_p_mpls(eth_type))
> > +                               return -EINVAL;
> 
> Should this also take into account the VLAN tag? It's really part of
> the EtherType although it has been stripped out here. Actually, maybe
> it's better to not track the vlan_tci separately at all during
> validation but just fold it into the EtherType.
> 
> The practical implication of this is that you wouldn't be able to pop
> out from underneath a VLAN. This may be a good thing if we are trying
> to avoid tag order issues - after all, you can't push underneath a
> VLAN either. I'm not sure what effects this has on the need to track
> mac_len, if any.

My thinking is that the ordering problem only surfaces in relation
to push MPLS actions where should it go in relation to VLAN tags.
For pop actions it seems to me that the outermost tag should be removed
regardless of its position in relation to other tags.

So I think that the code above is safe. Though now you mention
it I do notice that it only allows pop MPLS if there is at most
one VLAN tag present.

That said, I would not mind particularly disabling pop MPLS in the
presence of VLAN tags. At the very least it is related to the
painful issue of tag ordering.

I explored your idea of tracking only eth_type rather than both
it and vlan_tci. I did this by adding the following logic to
ovs_nla_copy_actions().

	if (key->eth.tci & htons(VLAN_TAG_PRESENT))
		eth_type = htons(ETH_P_8021Q);
	else
		eth_type = key->eth.type;

I then updated the usage of eth_type in ovs_nla_copy_actions__() accordingly.
One problem that I have run into is what to do about pop VLAN.

I don't believe its possible to know what the new eth type is.
This makes subsequent checks of the eth type for validate_set()
a little awkward. And seems to indicate that an extra parameter would
be needed.

For this reason I am inclined to leave the eth_type and vlan_tci
parameters in place. In this case there is no problem with pop VLAN
as the ether type inside the VLAN tag should be the value of eth_type.

> Otherwise, I'm happy with this. I think that we need to conclude the
> discussion on the other patch and update this appropriately first.

Yes, lets get that sorted out.

Assuming the other patch is accepted do you want me to increase the
coverage of the compatibility code (in this patch) up to whichever version
of the kernel the other patch is included in? It seems logical to me but I
do not have strong feelings about it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html