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