Re: [PATCH net-next v15] openvswitch: enable NSH support
On Tue, Nov 07, 2017 at 05:01:28AM -0800, Pravin Shelar wrote: > On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yiwrote: > > On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote: > >> On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi wrote: > >> > On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote: > >> >> On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc wrote: > >> >> > On Sat, 4 Nov 2017 07:29:46 -0700, Pravin Shelar wrote: > >> >> >> > + 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? > >> > > >> Yes. > > > > So here is newly-added check code, is it ok? > > > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > > index fa07a17..b64b754 100644 > > --- a/net/openvswitch/flow_netlink.c > > +++ b/net/openvswitch/flow_netlink.c > > @@ -3001,20 +3001,34 @@ static int __ovs_nla_copy_actions(struct net *net, > > const struct nlattr *attr, > > mac_proto = MAC_PROTO_ETHERNET; > > break; > > > > - case OVS_ACTION_ATTR_PUSH_NSH: > > + case OVS_ACTION_ATTR_PUSH_NSH: { > > + u8 next_proto; > > + > next_proto can be moved to the if () block, otherwise, I am fine with > this change. Thanks, fixed it and sent out v17.
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yiwrote: > On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote: >> On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi wrote: >> > On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote: >> >> On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc wrote: >> >> > 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. >> >> > >> >> 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? >> > >> Yes. > > So here is newly-added check code, is it ok? > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index fa07a17..b64b754 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -3001,20 +3001,34 @@ static int __ovs_nla_copy_actions(struct net *net, > const struct nlattr *attr, > mac_proto = MAC_PROTO_ETHERNET; > break; > > - case OVS_ACTION_ATTR_PUSH_NSH: > + case OVS_ACTION_ATTR_PUSH_NSH: { > + u8 next_proto; > + next_proto can be moved to the if () block, otherwise, I am fine with this change. > + if (mac_proto != MAC_PROTO_ETHERNET) { > + next_proto = tun_p_from_eth_p(eth_type); > + if (!next_proto) > + return -EINVAL; > + } > mac_proto = MAC_PROTO_NONE; > if (!validate_nsh(nla_data(a), false, true, true)) > return -EINVAL; > break; > + } > + > + case OVS_ACTION_ATTR_POP_NSH: { > + __be16 inner_proto; > > - case OVS_ACTION_ATTR_POP_NSH: > if (eth_type != htons(ETH_P_NSH)) > return -EINVAL; > + inner_proto = tun_p_to_eth_p(key->nsh.base.np); > + if (!inner_proto) > + return -EINVAL; > if (key->nsh.base.np == TUN_P_ETHERNET) > mac_proto = MAC_PROTO_ETHERNET; > else > mac_proto = MAC_PROTO_NONE; > break; > + } > > default: > OVS_NLERR(log, "Unknown Action type %d", type);
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote: > On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yiwrote: > > On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote: > >> On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc wrote: > >> > 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. > >> > > >> 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? > > > Yes. So here is newly-added check code, is it ok? diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index fa07a17..b64b754 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -3001,20 +3001,34 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr, mac_proto = MAC_PROTO_ETHERNET; break; - case OVS_ACTION_ATTR_PUSH_NSH: + case OVS_ACTION_ATTR_PUSH_NSH: { + u8 next_proto; + + if (mac_proto != MAC_PROTO_ETHERNET) { + next_proto = tun_p_from_eth_p(eth_type); + if (!next_proto) + return -EINVAL; + } mac_proto = MAC_PROTO_NONE; if (!validate_nsh(nla_data(a), false, true, true)) return -EINVAL; break; + } + + case OVS_ACTION_ATTR_POP_NSH: { + __be16 inner_proto; - case OVS_ACTION_ATTR_POP_NSH: if (eth_type != htons(ETH_P_NSH)) return -EINVAL; + inner_proto = tun_p_to_eth_p(key->nsh.base.np); + if (!inner_proto) + return -EINVAL; if (key->nsh.base.np == TUN_P_ETHERNET) mac_proto = MAC_PROTO_ETHERNET; else mac_proto = MAC_PROTO_NONE; break; + } default: OVS_NLERR(log, "Unknown Action type %d", type);
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yiwrote: > On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote: >> On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc wrote: >> > 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. >> > >> 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? > Yes.
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 Bencwrote: > > 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. > > > 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? > > >> > +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
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Mon, Nov 6, 2017 at 4:22 AM, Jiri Bencwrote: > 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. > 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. >> > +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
Re: [PATCH net-next v15] openvswitch: enable NSH support
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
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Sun, Nov 5, 2017 at 8:19 PM, Yang, Yiwrote: > On Sat, Nov 04, 2017 at 10:29:46PM +0800, Pravin Shelar wrote: >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang 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 think it can be easily done by other caller of these function, or you can refactor the APIs itself. > 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. > It is not just about optimization, but having these check in flow install allows OVS userspace to probe level of NSH support in datapath.
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 Yangwrote: > > +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; > > +} > > +
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Tue, Oct 31, 2017 at 9:03 PM, Yi Yangwrote: > v14->v15 > - Check size in nsh_hdr_from_nlattr > - Fixed four small issues pointed out By Jiri and Eric > > v13->v14 > - Rename skb_push_nsh to nsh_push per Dave's comment > - Rename skb_pop_nsh to nsh_pop per Dave's comment > > v12->v13 > - Fix NSH header length check in set_nsh > > v11->v12 > - Fix missing changes old comments pointed out > - Fix new comments for v11 > > v10->v11 > - Fix the left three disputable comments for v9 >but not fixed in v10. > > v9->v10 > - Change struct ovs_key_nsh to >struct ovs_nsh_key_base base; >__be32 context[NSH_MD1_CONTEXT_SIZE]; > - Fix new comments for v9 > > v8->v9 > - Fix build error reported by daily intel build >because nsh module isn't selected by openvswitch > > v7->v8 > - Rework nested value and mask for OVS_KEY_ATTR_NSH > - Change pop_nsh to adapt to nsh kernel module > - Fix many issues per comments from Jiri Benc > > v6->v7 > - Remove NSH GSO patches in v6 because Jiri Benc >reworked it as another patch series and they have >been merged. > - Change it to adapt to nsh kernel module added by NSH >GSO patch series > > v5->v6 > - Fix the rest comments for v4. > - Add NSH GSO support for VxLAN-gpe + NSH and >Eth + NSH. > > v4->v5 > - Fix many comments by Jiri Benc and Eric Garver >for v4. > > v3->v4 > - Add new NSH match field ttl > - Update NSH header to the latest format >which will be final format and won't change >per its author's confirmation. > - Fix comments for v3. > > v2->v3 > - Change OVS_KEY_ATTR_NSH to nested key to handle >length-fixed attributes and length-variable >attriubte more flexibly. > - Remove struct ovs_action_push_nsh completely > - Add code to handle nested attribute for SET_MASKED > - Change PUSH_NSH to use the nested OVS_KEY_ATTR_NSH >to transfer NSH header data. > - Fix comments and coding style issues by Jiri and Eric > > v1->v2 > - Change encap_nsh and decap_nsh to push_nsh and pop_nsh > - Dynamically allocate struct ovs_action_push_nsh for >length-variable metadata. > > OVS master and 2.8 branch has merged NSH userspace > patch series, this patch is to enable NSH support > in kernel data path in order that OVS can support > NSH in compat mode by porting this. > > Signed-off-by: Yi Yang As commented earlier following are action related validations that can be moved to flow install phase. > --- > include/net/nsh.h| 3 + > include/uapi/linux/openvswitch.h | 29 > net/nsh/nsh.c| 59 > net/openvswitch/Kconfig | 1 + > net/openvswitch/actions.c| 119 +++ > net/openvswitch/flow.c | 51 +++ > net/openvswitch/flow.h | 7 + > net/openvswitch/flow_netlink.c | 315 > ++- > net/openvswitch/flow_netlink.h | 5 + > 9 files changed, 588 insertions(+), 1 deletion(-) > ... > diff --git a/net/nsh/nsh.c b/net/nsh/nsh.c > index 58fb827..2764682 100644 > --- a/net/nsh/nsh.c > +++ b/net/nsh/nsh.c > @@ -14,6 +14,65 @@ > #include > #include > > +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(). > + 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; > +
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Thu, Nov 2, 2017 at 6:40 PM, Yang, Yiwrote: > On Thu, Nov 02, 2017 at 05:06:47AM -0700, Pravin Shelar wrote: >> On Wed, Nov 1, 2017 at 7:50 PM, Yang, Yi wrote: >> > On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote: >> >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang wrote: >> >> > >> >> > OVS master and 2.8 branch has merged NSH userspace >> >> > patch series, this patch is to enable NSH support >> >> > in kernel data path in order that OVS can support >> >> > NSH in compat mode by porting this. >> >> > >> >> > Signed-off-by: Yi Yang >> >> > --- >> >> I have comment related to checksum, otherwise patch looks good to me. >> > >> > Pravin, thank you for your comments, the below part is incremental patch >> > for checksum, please help check it, I'll send out v16 with this after >> > you confirm. >> > >> This change looks good to me. >> I noticed couple of more issues. >> 1. Can you move the ovs_key_nsh to the union of ipv4 an ipv6? >> ipv4/ipv6/nsh key data is mutually exclusive so there is no need for >> separate space for nsh key in the ovs key. >> 2. We need to fix match_validate() with nsh check. Datapath can not >> allow any l3 or l4 match if the flow key contains nsh match and >> vice-versa. such flow key should be rejected. > > Pravin, the below incremental patch should fix the issues you pionted > out, please help confirm/ack, then I'll send out v16 with all acks > from you all for merge. BTW, it has been verified in my sfc test > environment. > Following patch looks good to me. But I think we needs similar eth_type check for nsh set action in validate_set() and in __ovs_nla_copy_actions() for NSH_POP action. Can you send patch with all changes? Thanks. > diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h > index 8eeae749..c670dd2 100644 > --- a/net/openvswitch/flow.h > +++ b/net/openvswitch/flow.h > @@ -149,8 +149,8 @@ struct sw_flow_key { > } nd; > }; > } ipv6; > + struct ovs_key_nsh nsh; /* network service header */ > }; > - struct ovs_key_nsh nsh; /* network service header */ > struct { > /* Connection tracking fields not packed above. */ > struct { > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > index 0d7d4ae..090103c 100644 > --- a/net/openvswitch/flow_netlink.c > +++ b/net/openvswitch/flow_netlink.c > @@ -178,7 +178,8 @@ static bool match_validate(const struct sw_flow_match > *match, > | (1 << OVS_KEY_ATTR_ICMPV6) > | (1 << OVS_KEY_ATTR_ARP) > | (1 << OVS_KEY_ATTR_ND) > - | (1 << OVS_KEY_ATTR_MPLS)); > + | (1 << OVS_KEY_ATTR_MPLS) > + | (1 << OVS_KEY_ATTR_NSH)); > > /* Always allowed mask fields. */ > mask_allowed |= ((1 << OVS_KEY_ATTR_TUNNEL) > @@ -287,6 +288,14 @@ static bool match_validate(const struct sw_flow_match > *match, > } > } > > + if (match->key->eth.type == htons(ETH_P_NSH)) { > + key_expected |= 1 << OVS_KEY_ATTR_NSH; > + if (match->mask && > + match->mask->key.eth.type == htons(0x)) { > + mask_allowed |= 1 << OVS_KEY_ATTR_NSH; > + } > + } > + > if ((key_attrs & key_expected) != key_expected) { > /* Key attributes check failed. */ > OVS_NLERR(log, "Missing key (keys=%llx, expected=%llx)",
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Thu, Nov 02, 2017 at 05:06:47AM -0700, Pravin Shelar wrote: > On Wed, Nov 1, 2017 at 7:50 PM, Yang, Yiwrote: > > On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote: > >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang wrote: > >> > > >> > OVS master and 2.8 branch has merged NSH userspace > >> > patch series, this patch is to enable NSH support > >> > in kernel data path in order that OVS can support > >> > NSH in compat mode by porting this. > >> > > >> > Signed-off-by: Yi Yang > >> > --- > >> I have comment related to checksum, otherwise patch looks good to me. > > > > Pravin, thank you for your comments, the below part is incremental patch > > for checksum, please help check it, I'll send out v16 with this after > > you confirm. > > > This change looks good to me. > I noticed couple of more issues. > 1. Can you move the ovs_key_nsh to the union of ipv4 an ipv6? > ipv4/ipv6/nsh key data is mutually exclusive so there is no need for > separate space for nsh key in the ovs key. > 2. We need to fix match_validate() with nsh check. Datapath can not > allow any l3 or l4 match if the flow key contains nsh match and > vice-versa. such flow key should be rejected. Pravin, the below incremental patch should fix the issues you pionted out, please help confirm/ack, then I'll send out v16 with all acks from you all for merge. BTW, it has been verified in my sfc test environment. diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h index 8eeae749..c670dd2 100644 --- a/net/openvswitch/flow.h +++ b/net/openvswitch/flow.h @@ -149,8 +149,8 @@ struct sw_flow_key { } nd; }; } ipv6; + struct ovs_key_nsh nsh; /* network service header */ }; - struct ovs_key_nsh nsh; /* network service header */ struct { /* Connection tracking fields not packed above. */ struct { diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index 0d7d4ae..090103c 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c @@ -178,7 +178,8 @@ static bool match_validate(const struct sw_flow_match *match, | (1 << OVS_KEY_ATTR_ICMPV6) | (1 << OVS_KEY_ATTR_ARP) | (1 << OVS_KEY_ATTR_ND) - | (1 << OVS_KEY_ATTR_MPLS)); + | (1 << OVS_KEY_ATTR_MPLS) + | (1 << OVS_KEY_ATTR_NSH)); /* Always allowed mask fields. */ mask_allowed |= ((1 << OVS_KEY_ATTR_TUNNEL) @@ -287,6 +288,14 @@ static bool match_validate(const struct sw_flow_match *match, } } + if (match->key->eth.type == htons(ETH_P_NSH)) { + key_expected |= 1 << OVS_KEY_ATTR_NSH; + if (match->mask && + match->mask->key.eth.type == htons(0x)) { + mask_allowed |= 1 << OVS_KEY_ATTR_NSH; + } + } + if ((key_attrs & key_expected) != key_expected) { /* Key attributes check failed. */ OVS_NLERR(log, "Missing key (keys=%llx, expected=%llx)",
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Thu, Nov 02, 2017 at 05:06:47AM -0700, Pravin Shelar wrote: > On Wed, Nov 1, 2017 at 7:50 PM, Yang, Yiwrote: > > On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote: > >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang wrote: > >> > > >> > OVS master and 2.8 branch has merged NSH userspace > >> > patch series, this patch is to enable NSH support > >> > in kernel data path in order that OVS can support > >> > NSH in compat mode by porting this. > >> > > >> > Signed-off-by: Yi Yang > >> > --- > >> I have comment related to checksum, otherwise patch looks good to me. > > > > Pravin, thank you for your comments, the below part is incremental patch > > for checksum, please help check it, I'll send out v16 with this after > > you confirm. > > > This change looks good to me. > I noticed couple of more issues. > 1. Can you move the ovs_key_nsh to the union of ipv4 an ipv6? > ipv4/ipv6/nsh key data is mutually exclusive so there is no need for > separate space for nsh key in the ovs key. > 2. We need to fix match_validate() with nsh check. Datapath can not > allow any l3 or l4 match if the flow key contains nsh match and > vice-versa. such flow key should be rejected. Ok, I'll fix them ASAP.
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Wed, Nov 1, 2017 at 7:50 PM, Yang, Yiwrote: > On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote: >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang wrote: >> > >> > OVS master and 2.8 branch has merged NSH userspace >> > patch series, this patch is to enable NSH support >> > in kernel data path in order that OVS can support >> > NSH in compat mode by porting this. >> > >> > Signed-off-by: Yi Yang >> > --- >> I have comment related to checksum, otherwise patch looks good to me. > > Pravin, thank you for your comments, the below part is incremental patch > for checksum, please help check it, I'll send out v16 with this after > you confirm. > This change looks good to me. I noticed couple of more issues. 1. Can you move the ovs_key_nsh to the union of ipv4 an ipv6? ipv4/ipv6/nsh key data is mutually exclusive so there is no need for separate space for nsh key in the ovs key. 2. We need to fix match_validate() with nsh check. Datapath can not allow any l3 or l4 match if the flow key contains nsh match and vice-versa. such flow key should be rejected.
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote: > On Tue, Oct 31, 2017 at 9:03 PM, Yi Yangwrote: > > > > OVS master and 2.8 branch has merged NSH userspace > > patch series, this patch is to enable NSH support > > in kernel data path in order that OVS can support > > NSH in compat mode by porting this. > > > > Signed-off-by: Yi Yang > > --- > I have comment related to checksum, otherwise patch looks good to me. Pravin, thank you for your comments, the below part is incremental patch for checksum, please help check it, I'll send out v16 with this after you confirm. diff --git a/net/nsh/nsh.c b/net/nsh/nsh.c index 2764682..d7da99a 100644 --- a/net/nsh/nsh.c +++ b/net/nsh/nsh.c @@ -36,6 +36,7 @@ int nsh_push(struct sk_buff *skb, const struct nshhdr *pushed_nh) nh = (struct nshhdr *)(skb->data); memcpy(nh, pushed_nh, length); nh->np = next_proto; + skb_postpush_rcsum(skb, nh, length); skb->protocol = htons(ETH_P_NSH); skb_reset_mac_header(skb); @@ -63,7 +64,7 @@ int nsh_pop(struct sk_buff *skb) if (!inner_proto) return -EAFNOSUPPORT; - skb_pull(skb, length); + skb_pull_rcsum(skb, length); skb_reset_mac_header(skb); skb_reset_network_header(skb); skb_reset_mac_len(skb); diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index dd1449d..5ba0e35 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -671,6 +671,7 @@ static int set_nsh(struct sk_buff *skb, struct sw_flow_key *flow_key, return err; nh = nsh_hdr(skb); + skb_postpull_rcsum(skb, nh, length); flags = nsh_get_flags(nh); flags = OVS_MASKED(flags, key.base.flags, mask.base.flags); flow_key->nsh.base.flags = flags; @@ -698,6 +699,7 @@ static int set_nsh(struct sk_buff *skb, struct sw_flow_key *flow_key, default: return -EINVAL; } + skb_postpush_rcsum(skb, nh, length); return 0; }
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Tue, Oct 31, 2017 at 9:03 PM, Yi Yangwrote: > v14->v15 > - Check size in nsh_hdr_from_nlattr > - Fixed four small issues pointed out By Jiri and Eric > > v13->v14 > - Rename skb_push_nsh to nsh_push per Dave's comment > - Rename skb_pop_nsh to nsh_pop per Dave's comment > > v12->v13 > - Fix NSH header length check in set_nsh > > v11->v12 > - Fix missing changes old comments pointed out > - Fix new comments for v11 > > v10->v11 > - Fix the left three disputable comments for v9 >but not fixed in v10. > > v9->v10 > - Change struct ovs_key_nsh to >struct ovs_nsh_key_base base; >__be32 context[NSH_MD1_CONTEXT_SIZE]; > - Fix new comments for v9 > > v8->v9 > - Fix build error reported by daily intel build >because nsh module isn't selected by openvswitch > > v7->v8 > - Rework nested value and mask for OVS_KEY_ATTR_NSH > - Change pop_nsh to adapt to nsh kernel module > - Fix many issues per comments from Jiri Benc > > v6->v7 > - Remove NSH GSO patches in v6 because Jiri Benc >reworked it as another patch series and they have >been merged. > - Change it to adapt to nsh kernel module added by NSH >GSO patch series > > v5->v6 > - Fix the rest comments for v4. > - Add NSH GSO support for VxLAN-gpe + NSH and >Eth + NSH. > > v4->v5 > - Fix many comments by Jiri Benc and Eric Garver >for v4. > > v3->v4 > - Add new NSH match field ttl > - Update NSH header to the latest format >which will be final format and won't change >per its author's confirmation. > - Fix comments for v3. > > v2->v3 > - Change OVS_KEY_ATTR_NSH to nested key to handle >length-fixed attributes and length-variable >attriubte more flexibly. > - Remove struct ovs_action_push_nsh completely > - Add code to handle nested attribute for SET_MASKED > - Change PUSH_NSH to use the nested OVS_KEY_ATTR_NSH >to transfer NSH header data. > - Fix comments and coding style issues by Jiri and Eric > > v1->v2 > - Change encap_nsh and decap_nsh to push_nsh and pop_nsh > - Dynamically allocate struct ovs_action_push_nsh for >length-variable metadata. > > OVS master and 2.8 branch has merged NSH userspace > patch series, this patch is to enable NSH support > in kernel data path in order that OVS can support > NSH in compat mode by porting this. > > Signed-off-by: Yi Yang > --- I have comment related to checksum, otherwise patch looks good to me. > include/net/nsh.h| 3 + > include/uapi/linux/openvswitch.h | 29 > net/nsh/nsh.c| 59 > net/openvswitch/Kconfig | 1 + > net/openvswitch/actions.c| 119 +++ > net/openvswitch/flow.c | 51 +++ > net/openvswitch/flow.h | 7 + > net/openvswitch/flow_netlink.c | 315 > ++- > net/openvswitch/flow_netlink.h | 5 + > 9 files changed, 588 insertions(+), 1 deletion(-) > ... > diff --git a/net/nsh/nsh.c b/net/nsh/nsh.c > index 58fb827..2764682 100644 > --- a/net/nsh/nsh.c > +++ b/net/nsh/nsh.c > @@ -14,6 +14,65 @@ > #include > #include > > +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; > + } > + > + /* 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; dont you need to update checksum for CHECKSUM_COMPLETE case? > + > + 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); > + if (!pskb_may_pull(skb, length)) > + return -ENOMEM; > + > + if (!inner_proto) > + return -EAFNOSUPPORT; > + > + skb_pull(skb, length); same as above, we need to update the checksum. > + 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); > + > static struct sk_buff
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Wed, Nov 01, 2017 at 12:03:01PM +0800, Yi Yang wrote: > v14->v15 > - Check size in nsh_hdr_from_nlattr > - Fixed four small issues pointed out By Jiri and Eric Thanks Yi. Acked-by: Eric Garver
Re: [PATCH net-next v15] openvswitch: enable NSH support
On Wed, 1 Nov 2017 12:03:01 +0800, Yi Yang wrote: > v14->v15 > - Check size in nsh_hdr_from_nlattr > - Fixed four small issues pointed out By Jiri and Eric Acked-by: Jiri Benc
[PATCH net-next v15] openvswitch: enable NSH support
v14->v15 - Check size in nsh_hdr_from_nlattr - Fixed four small issues pointed out By Jiri and Eric v13->v14 - Rename skb_push_nsh to nsh_push per Dave's comment - Rename skb_pop_nsh to nsh_pop per Dave's comment v12->v13 - Fix NSH header length check in set_nsh v11->v12 - Fix missing changes old comments pointed out - Fix new comments for v11 v10->v11 - Fix the left three disputable comments for v9 but not fixed in v10. v9->v10 - Change struct ovs_key_nsh to struct ovs_nsh_key_base base; __be32 context[NSH_MD1_CONTEXT_SIZE]; - Fix new comments for v9 v8->v9 - Fix build error reported by daily intel build because nsh module isn't selected by openvswitch v7->v8 - Rework nested value and mask for OVS_KEY_ATTR_NSH - Change pop_nsh to adapt to nsh kernel module - Fix many issues per comments from Jiri Benc v6->v7 - Remove NSH GSO patches in v6 because Jiri Benc reworked it as another patch series and they have been merged. - Change it to adapt to nsh kernel module added by NSH GSO patch series v5->v6 - Fix the rest comments for v4. - Add NSH GSO support for VxLAN-gpe + NSH and Eth + NSH. v4->v5 - Fix many comments by Jiri Benc and Eric Garver for v4. v3->v4 - Add new NSH match field ttl - Update NSH header to the latest format which will be final format and won't change per its author's confirmation. - Fix comments for v3. v2->v3 - Change OVS_KEY_ATTR_NSH to nested key to handle length-fixed attributes and length-variable attriubte more flexibly. - Remove struct ovs_action_push_nsh completely - Add code to handle nested attribute for SET_MASKED - Change PUSH_NSH to use the nested OVS_KEY_ATTR_NSH to transfer NSH header data. - Fix comments and coding style issues by Jiri and Eric v1->v2 - Change encap_nsh and decap_nsh to push_nsh and pop_nsh - Dynamically allocate struct ovs_action_push_nsh for length-variable metadata. OVS master and 2.8 branch has merged NSH userspace patch series, this patch is to enable NSH support in kernel data path in order that OVS can support NSH in compat mode by porting this. Signed-off-by: Yi Yang--- include/net/nsh.h| 3 + include/uapi/linux/openvswitch.h | 29 net/nsh/nsh.c| 59 net/openvswitch/Kconfig | 1 + net/openvswitch/actions.c| 119 +++ net/openvswitch/flow.c | 51 +++ net/openvswitch/flow.h | 7 + net/openvswitch/flow_netlink.c | 315 ++- net/openvswitch/flow_netlink.h | 5 + 9 files changed, 588 insertions(+), 1 deletion(-) diff --git a/include/net/nsh.h b/include/net/nsh.h index a1eaea2..350b1ad 100644 --- a/include/net/nsh.h +++ b/include/net/nsh.h @@ -304,4 +304,7 @@ static inline void nsh_set_flags_ttl_len(struct nshhdr *nsh, u8 flags, NSH_FLAGS_MASK | NSH_TTL_MASK | NSH_LEN_MASK); } +int nsh_push(struct sk_buff *skb, const struct nshhdr *pushed_nh); +int nsh_pop(struct sk_buff *skb); + #endif /* __NET_NSH_H */ diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h index 0cd6f88..ac2623b 100644 --- a/include/uapi/linux/openvswitch.h +++ b/include/uapi/linux/openvswitch.h @@ -333,6 +333,7 @@ enum ovs_key_attr { OVS_KEY_ATTR_CT_LABELS, /* 16-octet connection tracking label */ OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */ OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */ + OVS_KEY_ATTR_NSH, /* Nested set of ovs_nsh_key_* */ #ifdef __KERNEL__ OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info */ @@ -492,6 +493,30 @@ struct ovs_key_ct_tuple_ipv6 { __u8 ipv6_proto; }; +enum ovs_nsh_key_attr { + OVS_NSH_KEY_ATTR_UNSPEC, + OVS_NSH_KEY_ATTR_BASE, /* struct ovs_nsh_key_base. */ + OVS_NSH_KEY_ATTR_MD1, /* struct ovs_nsh_key_md1. */ + OVS_NSH_KEY_ATTR_MD2, /* variable-length octets for MD type 2. */ + __OVS_NSH_KEY_ATTR_MAX +}; + +#define OVS_NSH_KEY_ATTR_MAX (__OVS_NSH_KEY_ATTR_MAX - 1) + +struct ovs_nsh_key_base { + __u8 flags; + __u8 ttl; + __u8 mdtype; + __u8 np; + __be32 path_hdr; +}; + +#define NSH_MD1_CONTEXT_SIZE 4 + +struct ovs_nsh_key_md1 { + __be32 context[NSH_MD1_CONTEXT_SIZE]; +}; + /** * enum ovs_flow_attr - attributes for %OVS_FLOW_* commands. * @OVS_FLOW_ATTR_KEY: Nested %OVS_KEY_ATTR_* attributes specifying the flow @@ -808,6 +833,8 @@ struct ovs_action_push_eth { * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the * packet. * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet. + * @OVS_ACTION_ATTR_PUSH_NSH: push NSH header to the packet. + * @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet. * * Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all * fields within a