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

[Pravin Shelar <pshelar@ovn.org>]

On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yi <yi.y.yang@intel.com> 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 <yi.y.yang@intel.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@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?
>> >
>> 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);