netdev - Re: [PATCH v3] 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: <20170816234940.GB123125@cran64.bj.intel.com>
Date:   Thu, 17 Aug 2017 07:49:41 +0800
From:   "Yang, Yi" <yi.y.yang@...el.com>
To:     Eric Garver <e@...g.me>
Cc:     netdev@...r.kernel.org, dev@...nvswitch.org, jbenc@...hat.com
Subject: Re: [PATCH v3] openvswitch: enable NSH support

On Wed, Aug 16, 2017 at 11:15:28PM +0800, Eric Garver wrote:
> On Wed, Aug 16, 2017 at 01:35:30PM +0800, Yi Yang wrote:
> > +
> > +#define NSH_DST_PORT    4790     /* UDP Port for NSH on VXLAN. */
> > +#define ETH_P_NSH       0x894F   /* Ethertype for NSH. */
> 
> ETH_P_NSH probably belongs in include/uapi/linux/if_ether.h with all the
> other ETH_P_* defines.
> 

Ok, I'll move it to include/uapi/linux/if_ether.h, but in userspace, we
still need to keep it in nsh.h.

> >  
> > +struct ovs_key_nsh {
> > +	__u8 flags;
> > +	__u8 mdtype;
> > +	__u8 np;
> > +	__u8 pad;
> > +	__be32 path_hdr;
> > +	__be32 context[NSH_MD1_CONTEXT_SIZE];
> > +};
> > +
> >  struct sw_flow_key {
> >  	u8 tun_opts[IP_TUNNEL_OPTS_MAX];
> >  	u8 tun_opts_len;
> > @@ -144,6 +154,7 @@ struct sw_flow_key {
> >  			};
> >  		} ipv6;
> >  	};
> > +	struct ovs_key_nsh nsh;         /* network service header */
> 
> Are you intentionally not reserving space in the flow key for
> OVS_NSH_KEY_ATTR_MD2? I know it's not supported yet, but much of the
> code is stubbed out for it - just making sure this wasn't an oversight.
> 

For MD type 2, we'll reuse tun_metedata keys in struct flow_tnl which
will be reworked and it will be shared by NSH and GENEVE, so we won't
have new keys in "struct ovs_key_nsh" for MD type 2.

> >  	struct {
> >  		/* Connection tracking fields not packed above. */
> >  		struct {
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index f07d10a..79059db 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -78,9 +78,11 @@ static bool actions_may_change_flow(const struct nlattr *actions)
> >  		case OVS_ACTION_ATTR_HASH:
> >  		case OVS_ACTION_ATTR_POP_ETH:
> >  		case OVS_ACTION_ATTR_POP_MPLS:
> > +		case OVS_ACTION_ATTR_POP_NSH:
> >  		case OVS_ACTION_ATTR_POP_VLAN:
> >  		case OVS_ACTION_ATTR_PUSH_ETH:
> >  		case OVS_ACTION_ATTR_PUSH_MPLS:
> > +		case OVS_ACTION_ATTR_PUSH_NSH:
> >  		case OVS_ACTION_ATTR_PUSH_VLAN:
> >  		case OVS_ACTION_ATTR_SAMPLE:
> >  		case OVS_ACTION_ATTR_SET:
> > @@ -322,12 +324,22 @@ size_t ovs_tun_key_attr_size(void)
> >  		+ nla_total_size(2);   /* OVS_TUNNEL_KEY_ATTR_TP_DST */
> >  }
> >  
> > +size_t ovs_nsh_key_attr_size(void)
> > +{
> > +	/* Whenever adding new OVS_NSH_KEY_ FIELDS, we should consider
> > +	 * updating this function.
> > +	 */
> > +	return  nla_total_size(8)      /* OVS_NSH_KEY_ATTR_BASE */
> > +		+ nla_total_size(16)   /* OVS_NSH_KEY_ATTR_MD1 */
> > +		+ nla_total_size(248); /* OVS_NSH_KEY_ATTR_MD2 */
> 
> _MD1 and _MD2 are mutually exclusive. You only need the larger of the
> two.
> 
> Maybe use OVS_PUSH_NSH_MAX_MD_LEN instead of 248.

Got it, will use nla_total_size(NSH_M_TYPE2_MAX_LEN - NSH_BASE_HDR_LEN)

> > +
> > +		switch (type) {
> > +		case OVS_NSH_KEY_ATTR_BASE: {
> > +			const struct ovs_nsh_key_base *base =
> > +				(struct ovs_nsh_key_base *)nla_data(a);
> > +			nsh->flags = base->flags;
> > +			nsh->mdtype = base->mdtype;
> > +			nsh->np = base->np;
> > +			nsh->path_hdr = base->path_hdr;
> > +			break;
> > +		}
> > +		case OVS_NSH_KEY_ATTR_MD1: {
> > +			const struct ovs_nsh_key_md1 *md1 =
> > +				(struct ovs_nsh_key_md1 *)nla_data(a);
> > +			memcpy(nsh->context, md1->context, sizeof(*md1));
> > +			break;
> > +		}
> > +		case OVS_NSH_KEY_ATTR_MD2:
> > +			/* Not supported yet */
> > +			break;
> 
> When we encounter these metadata attributes do we need to verify that
> nsh->mdtype is set correctly? Keep in mind we may parse ATTR_MD{1,2}
> before ATTR_BASE.

Good point, will add check code for this.