From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pravin Shelar Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support Date: Tue, 7 Nov 2017 05:01:28 -0800 Message-ID: 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="UTF-8" Cc: Jiri Benc , Linux Kernel Network Developers , ovs dev , Eric Garver , "David S. Miller" To: "Yang, Yi" Return-path: Received: from relay4-d.mail.gandi.net ([217.70.183.196]:39831 "EHLO relay4-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932706AbdKGNBb (ORCPT ); Tue, 7 Nov 2017 08:01:31 -0500 Received: from mail-wr0-f178.google.com (mail-wr0-f178.google.com [209.85.128.178]) (Authenticated sender: pshelar@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id A91B51720F0 for ; Tue, 7 Nov 2017 14:01:29 +0100 (CET) Received: by mail-wr0-f178.google.com with SMTP id u40so11752273wrf.10 for ; Tue, 07 Nov 2017 05:01:29 -0800 (PST) In-Reply-To: <20171107115504.GA56784@cran64.bj.intel.com> Sender: netdev-owner@vger.kernel.org List-ID: 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: >> >> >> > +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);