From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pravin Shelar <pshelar-LZ6Gd1LRuIk@public.gmane.org>
Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support
Date: Tue, 7 Nov 2017 02:57:30 -0800
Message-ID: <CAOrHB_CnXbGJjCT+-_KLi7C0XN9BK8QvRdTpgbzBj7xGeHbL7w@mail.gmail.com>
References: <1509508981-66202-1-git-send-email-yi.y.yang@intel.com>
	<CAOrHB_COGWb78kN70QFKgA3w5v3zasAW=TJbzoE5zZYKao_GvA@mail.gmail.com>
	<20171106132248.6fa4c5a0@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Cc: Linux Kernel Network Developers <netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Eric Garver <e@erig.me>, ovs dev <dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>,
	"David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
To: Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Return-path: <ovs-dev-bounces-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>
In-Reply-To: <20171106132248.6fa4c5a0-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
List-Unsubscribe: <https://mail.openvswitch.org/mailman/options/ovs-dev>,
	<mailto:ovs-dev-request-yBygre7rU0TnMu66kgdUjQ@public.gmane.org?subject=unsubscribe>
List-Archive: <http://mail.openvswitch.org/pipermail/ovs-dev/>
List-Post: <mailto:ovs-dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org>
List-Help: <mailto:ovs-dev-request-yBygre7rU0TnMu66kgdUjQ@public.gmane.org?subject=help>
List-Subscribe: <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>,
	<mailto:ovs-dev-request-yBygre7rU0TnMu66kgdUjQ@public.gmane.org?subject=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 Mon, Nov 6, 2017 at 4:22 AM, Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 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.

>> > +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