Re: [PATCH net-next v15] openvswitch: enable NSH support

On Sat, Nov 04, 2017 at 10:29:46PM +0800, Pravin Shelar wrote:

On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang [off-list ref] wrote:

quoted

+int nsh_push(struct sk_buff *skb, const struct nshhdr *pushed_nh)
+{
+       struct nshhdr *nh;
+       size_t length = nsh_hdr_len(pushed_nh);
+       u8 next_proto;
+
+       if (skb->mac_len) {
+               next_proto = TUN_P_ETHERNET;
+       } else {
+               next_proto = tun_p_from_eth_p(skb->protocol);
+               if (!next_proto)
+                       return -EAFNOSUPPORT;

check for supported protocols can be moved to flow install validation
in __ovs_nla_copy_actions().

quoted

+       }
+
+       /* Add the NSH header */
+       if (skb_cow_head(skb, length) < 0)
+               return -ENOMEM;
+
+       skb_push(skb, length);
+       nh = (struct nshhdr *)(skb->data);
+       memcpy(nh, pushed_nh, length);
+       nh->np = next_proto;
+
+       skb->protocol = htons(ETH_P_NSH);
+       skb_reset_mac_header(skb);
+       skb_reset_network_header(skb);
+       skb_reset_mac_len(skb);
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(nsh_push);
+
+int nsh_pop(struct sk_buff *skb)
+{
+       struct nshhdr *nh;
+       size_t length;
+       __be16 inner_proto;
+
+       if (!pskb_may_pull(skb, NSH_BASE_HDR_LEN))
+               return -ENOMEM;
+       nh = (struct nshhdr *)(skb->data);
+       length = nsh_hdr_len(nh);
+       inner_proto = tun_p_to_eth_p(nh->np);

same as above, this check can be moved to flow install __ovs_nla_copy_actions().

Pravin, these two functions are not only for OVS, you can see it is
net/nsh/nsh.c, Jiri and Eric mentioned they also could be used by TC.

I understand you expect some checks should be moved to slow path, but
for there two cases, we can't remove them into __ovs_nla_copy_actions.

quoted

+       if (!pskb_may_pull(skb, length))
+               return -ENOMEM;
+
+       if (!inner_proto)
+               return -EAFNOSUPPORT;
+
+       skb_pull(skb, length);
+       skb_reset_mac_header(skb);
+       skb_reset_network_header(skb);
+       skb_reset_mac_len(skb);
+       skb->protocol = inner_proto;
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(nsh_pop);
+

...

quoted

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index a551232..dd1449d 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c

...

quoted

+static int pop_nsh(struct sk_buff *skb, struct sw_flow_key *key)
+{
+       int err;
+
+       if (ovs_key_mac_proto(key) != MAC_PROTO_NONE ||
+           skb->protocol != htons(ETH_P_NSH)) {
+               return -EINVAL;
+       }
+

These checks can be moved to flow install.

Done in v16, here is incremental patch. I have sent out v16.

diff -u b/net/openvswitch/actions.c b/net/openvswitch/actions.c

--- b/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c

@@ -400,11 +400,6 @@
 {
 	int err;
 
-	if (ovs_key_mac_proto(key) != MAC_PROTO_NONE ||
-	    skb->protocol != htons(ETH_P_NSH)) {
-		return -EINVAL;
-	}
-
 	err = nsh_pop(skb);
 	if (err)
 		return err;

diff -u b/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c

--- b/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c

@@ -2737,6 +2737,8 @@
 		break;
 
 	case OVS_KEY_ATTR_NSH:
+		if (eth_type != htons(ETH_P_NSH))
+			return -EINVAL;
 		if (!validate_nsh(nla_data(a), masked, false, log))
 			return -EINVAL;
 		break;

@@ -3006,6 +3008,8 @@
 			break;
 
 		case OVS_ACTION_ATTR_POP_NSH:
+			if (eth_type != htons(ETH_P_NSH))
+				return -EINVAL;
 			if (key->nsh.base.np == TUN_P_ETHERNET)
 				mac_proto = MAC_PROTO_ETHERNET;
 			else

quoted

+       err = nsh_pop(skb);
+       if (err)
+               return err;
+
+       /* safe right before invalidate_flow_key */
+       if (skb->protocol == htons(ETH_P_TEB))
+               key->mac_proto = MAC_PROTO_ETHERNET;
+       else
+               key->mac_proto = MAC_PROTO_NONE;
+       invalidate_flow_key(key);
+       return 0;
+}
+