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

[Pravin Shelar <pshelar@ovn.org>]

On Thu, Nov 2, 2017 at 6:40 PM, Yang, Yi <yi.y.yang@intel.com> wrote:
> On Thu, Nov 02, 2017 at 05:06:47AM -0700, Pravin Shelar wrote:
>> On Wed, Nov 1, 2017 at 7:50 PM, Yang, Yi <yi.y.yang@intel.com> wrote:
>> > On Thu, Nov 02, 2017 at 08:52:40AM +0800, Pravin Shelar wrote:
>> >> On Tue, Oct 31, 2017 at 9:03 PM, Yi Yang <yi.y.yang@intel.com> wrote:
>> >> >
>> >> > OVS master and 2.8 branch has merged NSH userspace
>> >> > patch series, this patch is to enable NSH support
>> >> > in kernel data path in order that OVS can support
>> >> > NSH in compat mode by porting this.
>> >> >
>> >> > Signed-off-by: Yi Yang <yi.y.yang@intel.com>
>> >> > ---
>> >> I have comment related to checksum, otherwise patch looks good to me.
>> >
>> > Pravin, thank you for your comments, the below part is incremental patch
>> > for checksum, please help check it, I'll send out v16 with this after
>> > you confirm.
>> >
>> This change looks good to me.
>> I noticed couple of more issues.
>> 1. Can you move the ovs_key_nsh to the union of ipv4 an ipv6?
>> ipv4/ipv6/nsh key data is mutually exclusive so there is no need for
>> separate space for nsh key in the ovs key.
>> 2. We need to fix match_validate() with nsh check. Datapath can not
>> allow any l3 or l4 match if the flow key contains nsh match and
>> vice-versa. such flow key should be rejected.
>
> Pravin, the below incremental patch should fix the issues you pionted
> out, please help confirm/ack, then I'll send out v16 with all acks
> from you all for merge. BTW, it has been verified in my sfc test
> environment.
>
Following patch looks good to me. But I think we needs similar
eth_type check for nsh set action in validate_set() and in
__ovs_nla_copy_actions() for NSH_POP action.

Can you send patch with all changes?

Thanks.

> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index 8eeae749..c670dd2 100644
> --- a/net/openvswitch/flow.h
> +++ b/net/openvswitch/flow.h
> @@ -149,8 +149,8 @@ struct sw_flow_key {
>                                 } nd;
>                         };
>                 } ipv6;
> +               struct ovs_key_nsh nsh;         /* network service header */
>         };
> -       struct ovs_key_nsh nsh;         /* network service header */
>         struct {
>                 /* Connection tracking fields not packed above. */
>                 struct {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 0d7d4ae..090103c 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -178,7 +178,8 @@ static bool match_validate(const struct sw_flow_match *match,
>                         | (1 << OVS_KEY_ATTR_ICMPV6)
>                         | (1 << OVS_KEY_ATTR_ARP)
>                         | (1 << OVS_KEY_ATTR_ND)
> -                       | (1 << OVS_KEY_ATTR_MPLS));
> +                       | (1 << OVS_KEY_ATTR_MPLS)
> +                       | (1 << OVS_KEY_ATTR_NSH));
>
>         /* Always allowed mask fields. */
>         mask_allowed |= ((1 << OVS_KEY_ATTR_TUNNEL)
> @@ -287,6 +288,14 @@ static bool match_validate(const struct sw_flow_match *match,
>                 }
>         }
>
> +       if (match->key->eth.type == htons(ETH_P_NSH)) {
> +               key_expected |= 1 << OVS_KEY_ATTR_NSH;
> +               if (match->mask &&
> +                   match->mask->key.eth.type == htons(0xffff)) {
> +                       mask_allowed |= 1 << OVS_KEY_ATTR_NSH;
> +               }
> +       }
> +
>         if ((key_attrs & key_expected) != key_expected) {
>                 /* Key attributes check failed. */
>                 OVS_NLERR(log, "Missing key (keys=%llx, expected=%llx)",