Re: [PATCH v3] openvswitch: enable NSH support

On Wed, Aug 16, 2017 at 11:19:21AM +0200, Jiri Benc wrote:

Please always CC reviewers of the previous versions, thanks.

Jiri, thank you for quick review. Sorry, I made a mistake on
sending and missed all the CCs, will indeed do this in next version.

quoted

+	__be16 ver_flags_len;
+	u8 md_type;
+	u8 next_proto;
+	__be32 path_hdr;
+	u8 metadata[NSH_M_TYPE2_MAX_LEN-8];
+};

Please get rid of this struct. It's a copy of struct nsh_hdr with some
space added to the bottom.

One of the options (though maybe not the best one, feel free to come up
with something better) is to change struct nsh_md1_ctx to:

struct nsh_md1_ctx {
	__be32 context[];
};

and change struct push_nsh_para:

struct push_nsh_para {
	struct nsh_hdr hdr;
	u8 metadata[NSH_M_TYPE2_MAX_LEN-8];
};

Another option (a better one, though a bit more work) is to get rid of
push_nsh_para completely and just pass a properly allocated nsh_hdr
around. Introduce macros and/or functions to help with the allocation.

Yeah, good point, we can use dynamic allocation and struct nsh_hdr * to
handle this.

quoted

+static inline struct nsh_md1_ctx *nsh_md1_ctx(struct nsh_hdr *nsh)
+{
+	return &nsh->md1;
+}
+
+static inline struct nsh_md2_tlv *nsh_md2_ctx(struct nsh_hdr *nsh)
+{
+	return nsh->md2;
+}

These are unused, please remove them.

Will remove them, userspace does use them.

quoted

--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h

[...]

quoted

+#define NSH_MD1_CONTEXT_SIZE 4

Please move this to nsh.h and use it there, too, instead of the open
coded 4.

ovs code is very ugly, it will convert array[4] in
datapath/linux/compat/include/linux/openvswitch.h to other struct, I
have to change context[4] to such format :-), we can use 4 here for
Linux kernel.

quoted

+static int push_nsh(struct sk_buff *skb, struct sw_flow_key *key,
+		    const struct push_nsh_para *pnp)
+{
+	struct nsh_hdr *nsh;
+	size_t length = ((ntohs(pnp->ver_flags_len) & NSH_LEN_MASK)
+			     >> NSH_LEN_SHIFT) << 2;

Once push_nsh_para is removed/changed, this can be changed to a call to
nsh_hdr_len.

Yes, will do that way.

quoted

+	flags = (ntohs(nsh->ver_flags_len) & NSH_FLAGS_MASK) >>
+		NSH_FLAGS_SHIFT;

nsh_get_flags

Missed this one :-)

quoted

+	case OVS_KEY_ATTR_NSH: {
+		struct ovs_key_nsh nsh;
+		struct ovs_key_nsh nsh_mask;
+		size_t size = nla_len(a) / 2;
+		struct nlattr attr[1 + DIV_ROUND_UP(sizeof(struct ovs_key_ipv6)
+						    , sizeof(struct nlattr))];
+		struct nlattr mask[1 + DIV_ROUND_UP(sizeof(struct ovs_key_ipv6)
+						    , sizeof(struct nlattr))];
+
+		attr->nla_type = nla_type(a);
+		mask->nla_type = attr->nla_type;
+		attr->nla_len = NLA_HDRLEN + size;
+		mask->nla_len = attr->nla_len;
+		memcpy(attr + 1, (char *)(a + 1), size);
+		memcpy(mask + 1, (char *)(a + 1) + size, size);

This is too hacky. Please find a better way to handle this.

One option is to create a struct with struct nlattr as the first member
followed by a char buffer. Still not nice but at least it's clear
what's the intent.

The issue is nested attributes only can use this way, nested attribute
for SET_MASKED is very special, we have to handle it specially.

quoted

+static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
+{
+	struct nsh_hdr *nsh = (struct nsh_hdr *)skb_network_header(skb);
+	u8 version, length;
+	u32 path_hdr;
+	int i;
+
+	memset(&key->nsh, 0, sizeof(struct ovs_key_nsh));
+	version = nsh_get_ver(nsh);
+	length = nsh_get_len(nsh);
+
+	key->nsh.flags = nsh_get_flags(nsh);
+	key->nsh.mdtype = nsh->md_type;
+	key->nsh.np = nsh->next_proto;
+	path_hdr = ntohl(nsh->path_hdr);

The path_hdr variable is unused.

Will remove it.

quoted

+	key->nsh.path_hdr = nsh->path_hdr;
+	switch (key->nsh.mdtype) {
+	case NSH_M_TYPE1:
+		if ((length << 2) != NSH_M_TYPE1_LEN)

Why length << 2?

len in NSH header is number of 4 octets, so need to multiply 4.

quoted

+			return -EINVAL;
+
+		for (i = 0; i < 4; i++)

NSH_MD1_CONTEXT_SIZE

Ok

quoted

+			key->nsh.context[i] = nsh->md1.context[i];
+
+		break;

Will go through the rest later. Feel free to send a new version
meanwhile.

Thanks,

 Jiri

Thank you so much for your comments, will work out new version ASAP.