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

["Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Tue, Nov 07, 2017 at 05:01:28AM -0800, Pravin Shelar wrote:
> On Tue, Nov 7, 2017 at 3:55 AM, Yang, Yi <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> >> > On Sat, 4 Nov 2017 07:29:46 -0700, Pravin Shelar wrote:
> >> >> >> > +       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.

Thanks, fixed it and sent out v17.