[PATCH net-next 1/2] openvswitch: Add a missing break statement.

[PATCH net-next 1/2] openvswitch: Add a missing break statement.

[Jarno Rajahalme]

Add a break statement to prevent fall-through from
OVS_KEY_ATTR_ETHERNET to OVS_KEY_ATTR_TUNNEL.  Without the break
actions setting ethernet addresses fail to validate with log messages
complaining about invalid tunnel attributes.

Fixes: 0a6410fbde ("openvswitch: netlink: support L3 packets")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/openvswitch/flow_netlink.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index d19044f..c87d359 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2195,6 +2195,7 @@ static int validate_set(const struct nlattr *a,
 	case OVS_KEY_ATTR_ETHERNET:
 		if (mac_proto != MAC_PROTO_ETHERNET)
 			return -EINVAL;
+		break;
 
 	case OVS_KEY_ATTR_TUNNEL:
 		if (masked)
-- 
2.1.4

[PATCH net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.

[Jarno Rajahalme]

Do not set skb->protocol to be the ethertype of the L3 header, unless
the packet only has the L3 header.  For a non-hardware offloaded VLAN
frame skb->protocol needs to be one of the VLAN ethertypes.

Any VLAN offloading is undone on the OVS netlink interface.  Due to
this all VLAN packets sent to openvswitch module from userspace are
non-offloaded.

Incorrect skb->protocol value on a full-size non-offloaded VLAN skb
causes packet drop due to failing MTU check, as the VLAN header should
not be counted in when considering MTU in ovs_vport_send().

Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/openvswitch/datapath.c |  1 -
 net/openvswitch/flow.c     | 20 +++++++++++---------
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 2d4c4d3..9c62b63 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -606,7 +606,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	rcu_assign_pointer(flow->sf_acts, acts);
 	packet->priority = flow->key.phy.priority;
 	packet->mark = flow->key.phy.skb_mark;
-	packet->protocol = flow->key.eth.type;
 
 	rcu_read_lock();
 	dp = get_dp_rcu(net, ovs_header->dp_ifindex);
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 08aa926..9be9fda 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -477,12 +477,17 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
 }
 
 /**
- * key_extract - extracts a flow key from an Ethernet frame.
+ * key_extract - extracts a flow key from a packet with or without an
+ * Ethernet header.
  * @skb: sk_buff that contains the frame, with skb->data pointing to the
- * Ethernet header
+ * beginning of the packet.
  * @key: output flow key
  *
- * The caller must ensure that skb->len >= ETH_HLEN.
+ * 'key->mac_proto' must be initialized to indicate the frame type.
+ * For an L3 frame 'key->mac_proto' must equal 'MAC_PROTO_NONE', and the
+ * caller must ensure that 'skb->protocol' is set to the ethertype of the L3
+ * header.  Otherwise the presence of an Ethernet header is assumed and
+ * the caller must ensure that skb->len >= ETH_HLEN.
  *
  * Returns 0 if successful, otherwise a negative errno value.
  *
@@ -497,9 +502,6 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
  *      on output, then just past the IP header, if one is present and
  *      of a correct length, otherwise the same as skb->network_header.
  *      For other key->eth.type values it is left untouched.
- *
- *    - skb->protocol: the type of the data starting at skb->network_header.
- *      Equals to key->eth.type.
  */
 static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 {
@@ -518,6 +520,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			return -EINVAL;
 
 		skb_reset_network_header(skb);
+		key->eth.type = skb->protocol;
 	} else {
 		eth = eth_hdr(skb);
 		ether_addr_copy(key->eth.src, eth->h_source);
@@ -531,15 +534,14 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 		if (unlikely(parse_vlan(skb, key)))
 			return -ENOMEM;
 
-		skb->protocol = parse_ethertype(skb);
-		if (unlikely(skb->protocol == htons(0)))
+		key->eth.type = parse_ethertype(skb);
+		if (unlikely(key->eth.type == htons(0)))
 			return -ENOMEM;
 
 		skb_reset_network_header(skb);
 		__skb_push(skb, skb->data - skb_mac_header(skb));
 	}
 	skb_reset_mac_len(skb);
-	key->eth.type = skb->protocol;
 
 	/* Network layer. */
 	if (key->eth.type == htons(ETH_P_IP)) {
-- 
2.1.4

Re: [PATCH net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.

[Jiri Benc]

On Tue, 22 Nov 2016 20:09:34 -0800, Jarno Rajahalme wrote:
> Do not set skb->protocol to be the ethertype of the L3 header, unless
> the packet only has the L3 header.  For a non-hardware offloaded VLAN
> frame skb->protocol needs to be one of the VLAN ethertypes.
> 
> Any VLAN offloading is undone on the OVS netlink interface.  Due to
> this all VLAN packets sent to openvswitch module from userspace are
> non-offloaded.

This is exactly why I wanted to always accelerate the vlan tag, the
same way it is done in other parts of the networking stack: to prevent
all those weird corner cases.

Looks to me this is the only real way forward.

This patch is wrong, it would leave skb->protocol as ETH_P_TEB for L2
frames received via ARPHRD_NONE interface.

 Jiri

Re: [PATCH net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.

[Jarno Rajahalme]

> On Nov 24, 2016, at 8:10 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 
> On Tue, 22 Nov 2016 20:09:34 -0800, Jarno Rajahalme wrote:
>> Do not set skb->protocol to be the ethertype of the L3 header, unless
>> the packet only has the L3 header.  For a non-hardware offloaded VLAN
>> frame skb->protocol needs to be one of the VLAN ethertypes.
>> 
>> Any VLAN offloading is undone on the OVS netlink interface.  Due to
>> this all VLAN packets sent to openvswitch module from userspace are
>> non-offloaded.
> 
> This is exactly why I wanted to always accelerate the vlan tag, the
> same way it is done in other parts of the networking stack: to prevent
> all those weird corner cases.
> 
> Looks to me this is the only real way forward.
> 

I’m not sure what you suggest here. Obviously the kernel ABI can not be changed as existing userspace code expects upcalled packets to be non-accelerated. Also, if userspace pushes vlan headers, the packet will actually have them.

> This patch is wrong, it would leave skb->protocol as ETH_P_TEB for L2
> frames received via ARPHRD_NONE interface.
> 

Would this incremental fix this:

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 9be9fda..37f1bb9 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -354,6 +354,8 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 		res = parse_vlan_tag(skb, &key->eth.vlan);
 		if (res <= 0)
 			return res;
+		if (skb->protocol == htons(ETH_P_TEB))
+			skb->protocol = key->eth.vlan.tpid;
 	}
 
 	/* Parse inner vlan tag. */


  Jarno

Re: [PATCH net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.

[Jiri Benc]

On Mon, 28 Nov 2016 14:29:39 -0800, Jarno Rajahalme wrote:
> I’m not sure what you suggest here. Obviously the kernel ABI can not
> be changed as existing userspace code expects upcalled packets to be
> non-accelerated. Also, if userspace pushes vlan headers, the packet
> will actually have them.

The user space API needs to be preserved, of course. I'm talking about
what happens internally in the kernel.

See this patchset: https://www.spinics.net/lists/netdev/msg398827.html

> Would this incremental fix this:
> 
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 9be9fda..37f1bb9 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -354,6 +354,8 @@ static int parse_vlan(struct sk_buff *skb, struct
> sw_flow_key *key) res = parse_vlan_tag(skb, &key->eth.vlan);
>  		if (res <= 0)
>  			return res;
> +		if (skb->protocol == htons(ETH_P_TEB))
> +			skb->protocol = key->eth.vlan.tpid;
>  	}
>  
>  	/* Parse inner vlan tag. */

I'll look at this tomorrow. But it seems we're adding more and more
hacks instead of cleaning up the vlan handling.

 Jiri

Re: [PATCH net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.

[Jarno Rajahalme]

> On Nov 28, 2016, at 2:42 PM, Jiri Benc <jbenc@redhat.com> wrote:
> 
> On Mon, 28 Nov 2016 14:29:39 -0800, Jarno Rajahalme wrote:
>> I’m not sure what you suggest here. Obviously the kernel ABI can not
>> be changed as existing userspace code expects upcalled packets to be
>> non-accelerated. Also, if userspace pushes vlan headers, the packet
>> will actually have them.
> 
> The user space API needs to be preserved, of course. I'm talking about
> what happens internally in the kernel.
> 
> See this patchset: https://www.spinics.net/lists/netdev/msg398827.html
> 

I did not try to apply this series yet, but given the recent L3 changes maybe it needs a rebase?

>> Would this incremental fix this:
>> 
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 9be9fda..37f1bb9 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -354,6 +354,8 @@ static int parse_vlan(struct sk_buff *skb, struct
>> sw_flow_key *key) res = parse_vlan_tag(skb, &key->eth.vlan);
>> 		if (res <= 0)
>> 			return res;
>> +		if (skb->protocol == htons(ETH_P_TEB))
>> +			skb->protocol = key->eth.vlan.tpid;
>> 	}
>> 
>> 	/* Parse inner vlan tag. */
> 
> I'll look at this tomorrow. But it seems we're adding more and more
> hacks instead of cleaning up the vlan handling.
> 

Right, I just noticed that the incremental only handles the VLAN case.

I’ll post a v2 later today with a proper fix that solves the immediate issue. IMO this should be fixed independently of the VLAN handling series for which I have no informed opinion yet.

  Jarno

> Jiri

Re: [PATCH net-next 1/2] openvswitch: Add a missing break statement.

[Pravin Shelar]

On Tue, Nov 22, 2016 at 8:09 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
> Add a break statement to prevent fall-through from
> OVS_KEY_ATTR_ETHERNET to OVS_KEY_ATTR_TUNNEL.  Without the break
> actions setting ethernet addresses fail to validate with log messages
> complaining about invalid tunnel attributes.
>
> Fixes: 0a6410fbde ("openvswitch: netlink: support L3 packets")
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---
>  net/openvswitch/flow_netlink.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index d19044f..c87d359 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -2195,6 +2195,7 @@ static int validate_set(const struct nlattr *a,
>         case OVS_KEY_ATTR_ETHERNET:
>                 if (mac_proto != MAC_PROTO_ETHERNET)
>                         return -EINVAL;
> +               break;
>
>         case OVS_KEY_ATTR_TUNNEL:
>                 if (masked)

Thanks for tracking it down.

Acked-by: Pravin B Shelar <pshelar@ovn.org>

Re: [PATCH net-next 1/2] openvswitch: Add a missing break statement.

[Jiri Benc]

On Tue, 22 Nov 2016 20:09:33 -0800, Jarno Rajahalme wrote:
> Add a break statement to prevent fall-through from
> OVS_KEY_ATTR_ETHERNET to OVS_KEY_ATTR_TUNNEL.  Without the break
> actions setting ethernet addresses fail to validate with log messages
> complaining about invalid tunnel attributes.
> 
> Fixes: 0a6410fbde ("openvswitch: netlink: support L3 packets")
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

Acked-by: Jiri Benc <jbenc@redhat.com>

It's a good practice to CC the one who messed up :-) Please do that
next time.

Thanks for noticing the bug and fixing it.

 Jiri