From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yang, Yi" Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support Date: Tue, 7 Nov 2017 21:09:14 +0800 Message-ID: <20171107130914.GA93033@localhost.localdomain> References: <1509508981-66202-1-git-send-email-yi.y.yang@intel.com> <20171106132248.6fa4c5a0@redhat.com> <20171107112833.GA56179@cran64.bj.intel.com> <20171107115504.GA56784@cran64.bj.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: ovs dev , Linux Kernel Network Developers , Eric Garver , Jiri Benc , "David S. Miller" To: Pravin Shelar Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org Errors-To: ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org List-Id: netdev.vger.kernel.org On Tue, Nov 07, 2017 at 05:01:28AM -0800, Pravin Shelar wrote: > On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yi wrote: > > 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.