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

Subsystems: networking [general], openvswitch, the rest

3 messages, 2 authors, 2017-11-07 · open the first message on its own page

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

From: Yang, Yi <hidden>
Date: 2017-11-07 11:55:04 

On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote:
On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi [off-list ref] wrote:
quoted
On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote:
quoted
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?
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

From: Pravin Shelar <hidden>
Date: 2017-11-07 13:01:31 

On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yi [off-list ref] wrote:
quoted hunk
On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote:
quoted
On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi [off-list ref] wrote:
quoted
On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote:
quoted
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?
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

From: Yang, Yi <hidden>
Date: 2017-11-07 13:09:14 

On Tue, Nov 07, 2017 at 05:01:28AM -0800, Pravin Shelar wrote:
On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yi [off-list ref] wrote:
quoted
On Tue, Nov 07, 2017 at 03:58:35AM -0800, Pravin Shelar wrote:
quoted
On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi [off-list ref] wrote:
quoted
On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote:
quoted
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
+       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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help