On Sat, 4 Nov 2017 07:29:46 -0700, Pravin Shelar 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().
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. > > +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. > > + 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