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

On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote:

On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc [off-list ref] wrote:

quoted

On Sat, 4 Nov 2017 07:29:46 -0700, Pravin Shelar wrote:

quoted

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().

You mean the check for !next_proto? It needs to be present for
correctness of nsh_push. This function has to be self contained, it
will be used by more callers than opevswitch, namely tc.

It's actually not so much a check for "supported protocols", it's
rather a check of return value of a function that converts ethertype to
a 1 byte tunnel type. Blindly using a result of a function that may
return error would be wrong. Openvswitch is free to perform additional
checks but this needs to stay.

I am not disputing validity of the checks, but it could be done at
flow install phase.
For other use case we could refactor code. If it is too complex, I am
fine with duplicate code that check the protocol in flow install for
now.

Ok, I'll add check code in __ovs_nla_copy_actions for both nsh_push and
nsh_pop, but how can we get value of skb->protocol in
__ovs_nla_copy_actions? Is it argument eth_type of
__ovs_nla_copy_actions?
 

quoted

quoted

quoted

+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().

There's no check here.

quoted

quoted

+       if (!pskb_may_pull(skb, length))
+               return -ENOMEM;
+
+       if (!inner_proto)
+               return -EAFNOSUPPORT;

Did you mean this one instead? Then see above, this has to stay.

 Jiri