From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Yang, Yi" <yi.y.yang@intel.com>
Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support
Date: Tue, 7 Nov 2017 19:28:34 +0800
Message-ID: <20171107112833.GA56179@cran64.bj.intel.com>
References: <1509508981-66202-1-git-send-email-yi.y.yang@intel.com>
 <CAOrHB_COGWb78kN70QFKgA3w5v3zasAW=TJbzoE5zZYKao_GvA@mail.gmail.com>
 <20171106132248.6fa4c5a0@redhat.com>
 <CAOrHB_CnXbGJjCT+-_KLi7C0XN9BK8QvRdTpgbzBj7xGeHbL7w@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Jiri Benc <jbenc@redhat.com>,
        Linux Kernel Network Developers <netdev@vger.kernel.org>,
        ovs dev <dev@openvswitch.org>, Eric Garver <e@erig.me>,
        "David S. Miller" <davem@davemloft.net>
To: Pravin Shelar <pshelar@ovn.org>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mga11.intel.com ([192.55.52.93]:36201 "EHLO mga11.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752347AbdKGLo2 (ORCPT <rfc822;netdev@vger.kernel.org>);
        Tue, 7 Nov 2017 06:44:28 -0500
Content-Disposition: inline
In-Reply-To: <CAOrHB_CnXbGJjCT+-_KLi7C0XN9BK8QvRdTpgbzBj7xGeHbL7w@mail.gmail.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Tue, Nov 07, 2017 at 06:57:30PM +0800, Pravin Shelar wrote:
> On Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc <jbenc@redhat.com> 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?
 
> 
> >> > +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