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

["Yang, Yi" <yi.y.yang@intel.com>]

On Sat, Nov 04, 2017 at 10:29:46PM +0800, Pravin Shelar wrote:
> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang <yi.y.yang@intel.com> wrote:
> > +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().
> 
> > +       }
> > +
> > +       /* 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.

> 
> > +       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);
> > +
> ...
> > 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
> ...
> > +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
> 
> > +       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;
> > +}
> > +