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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOrHB_D2khYGGJxNA8hpE4Qnhm7ONKrH=kcNteV=iYWLRnaGHQ@mail.gmail.com>
Date:   Tue, 7 Nov 2017 03:58:35 -0800
From:   Pravin Shelar <pshelar@....org>
To:     "Yang, Yi" <yi.y.yang@...el.com>
Cc:     Jiri Benc <jbenc@...hat.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        ovs dev <dev@...nvswitch.org>, Eric Garver <e@...g.me>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v15] openvswitch: enable NSH support

On Tue, Nov 7, 2017 at 3:28 AM, Yang, Yi <yi.y.yang@...el.com> 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 <jbenc@...hat.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?
>
Yes.