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

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

On Tue, Aug 15, 2017 at 12:09:14AM +0800, Eric Garver wrote:
> On Thu, Aug 10, 2017 at 09:21:15PM +0800, Yi Yang wrote:
> 
> Hi Yi,
> 
> In general I'd like to echo Jiri's comments on the netlink attributes.
> I'd like to see the metadata separate.
> 
> I have a few other comments below.
> 
> Thanks.
> Eric.
> 
> [..]

Thanks Eric, I'm doing this and it is almost done, there is still an
issue to fix.

> > +{
> > +	return 4 * (ntohs(nsh->ver_flags_len) & NSH_LEN_MASK) >> NSH_LEN_SHIFT;
> 
> This is doing the multiplication before the shift. It works only because
> the shift is 0.
> 

Thank you for catching this, the right one:

((ntohs(nsh->ver_flags_len) & NSH_LEN_MASK) >> NSH_LEN_SHIFT) << 2

> > +static int push_nsh(struct sk_buff *skb, struct sw_flow_key *key,
> > +		    const struct ovs_action_push_nsh *oapn)
> > +{
> > +	struct nsh_hdr *nsh;
> > +	size_t length = NSH_BASE_HDR_LEN + oapn->mdlen;
> > +	u8 next_proto;
> > +
> > +	if (key->mac_proto == MAC_PROTO_ETHERNET) {
> > +		next_proto = NSH_P_ETHERNET;
> > +	} else {
> > +		switch (ntohs(skb->protocol)) {
> > +		case ETH_P_IP:
> > +			next_proto = NSH_P_IPV4;
> > +			break;
> > +		case ETH_P_IPV6:
> > +			next_proto = NSH_P_IPV6;
> > +			break;
> > +		case ETH_P_NSH:
> > +			next_proto = NSH_P_NSH;
> > +			break;
> > +		default:
> > +			return -ENOTSUPP;
> > +		}
> > +	}
> > +
> >
> 
> I believe you need to validate that oapn->mdlen is a multiple of 4.
>

I'll add this check.

> > +	switch (nsh->md_type) {
> > +	case NSH_M_TYPE1:
> > +		nsh->md1 = *(struct nsh_md1_ctx *)oapn->metadata;
> > +		break;
> > +	case NSH_M_TYPE2: {
> > +		/* The MD2 metadata in oapn is already padded to 4 bytes. */
> > +		size_t len = DIV_ROUND_UP(oapn->mdlen, 4) * 4;
> > +
> > +		memcpy(nsh->md2, oapn->metadata, len);
> 
> I don't see any validation of oapn->mdlen. Normally this happens in
> __ovs_nla_copy_actions(). It will be made easier if you add a separate
> MD attribute as Jiri has suggested.
>

Got it, will include this in next version.