netdev - Re: [PATCH net-next v4] 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: <20170821062116.GA73069@cran64.bj.intel.com>
Date:   Mon, 21 Aug 2017 14:21:16 +0800
From:   "Yang, Yi" <yi.y.yang@...el.com>
To:     Eric Garver <e@...g.me>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "dev@...nvswitch.org" <dev@...nvswitch.org>,
        "blp@....org" <blp@....org>, "jbenc@...hat.com" <jbenc@...hat.com>,
        "jan.scheurich@...csson.com" <jan.scheurich@...csson.com>
Subject: Re: [PATCH net-next v4] openvswitch: enable NSH support

On Sat, Aug 19, 2017 at 03:09:47AM +0800, Eric Garver wrote:
> On Fri, Aug 18, 2017 at 03:24:31PM +0800, Yi Yang wrote:
> > v3->v4
> >  - Add new NSH match field ttl
> >  - Update NSH header to the latest format
> >    which will be final format and won't change
> >    per its author's confirmation.
> >  - Fix comments for v3.
> 
> Hi Yi,
> Only a few comments below since Jiri already supplied lots of feedback.
>

Eric, thank you for your comments, I have fixed them in v5, please help
review v5, thanks a lot.
 
> > @@ -1210,6 +1373,20 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >  		case OVS_ACTION_ATTR_POP_ETH:
> >  			err = pop_eth(skb, key);
> >  			break;
> > +
> > +		case OVS_ACTION_ATTR_PUSH_NSH: {
> > +			u8 buffer[256];
> 
> Use NSH_M_TYPE2_MAX_LEN

Replaced 256 to NSH_M_TYPE2_MAX_LEN in v5.

> > +		case OVS_NSH_KEY_ATTR_MD1: {
> > +			const struct ovs_nsh_key_md1 *md1 =
> > +				(struct ovs_nsh_key_md1 *)nla_data(a);
> > +
> > +			has_md1 = true;
> > +			memcpy(nsh->context, md1->context, sizeof(*md1));
> > +			break;
> > +		}
> > +		case OVS_NSH_KEY_ATTR_MD2:
> > +			/* Not supported yet */
> 
> return -ENOTPSUPP if it's not supported.

Did that way in v5.

> > +		case OVS_NSH_KEY_ATTR_MD1: {
> > +			const struct ovs_nsh_key_md1 *md1 =
> > +				(struct ovs_nsh_key_md1 *)nla_data(a);
> > +
> > +			has_md1 = true;
> > +			for (i = 0; i < NSH_MD1_CONTEXT_SIZE; i++)
> > +				SW_FLOW_KEY_PUT(match, nsh.context[i],
> > +						md1->context[i], is_mask);
> > +			break;
> > +		}
> > +		case OVS_NSH_KEY_ATTR_MD2:
> > +			/* Not supported yet */
> 
> return -ENOTPSUPP if it's not supported.

Done in v5.

> > @@ -2636,6 +2984,17 @@ 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:
> 
> You need to some validation here, especially the metadata lengths.
> Relying on action_lens is not enough because it's variable.
> 

Good point, I added validate_nsh for this.