[dpdk-dev] [PATCH v6 10/11] ethdev: fix shallow copy of flow API RSS action

[Adrien Mazarguil <adrien.mazarguil@6wind.com>]

The rss_conf field is defined as a pointer to struct rte_eth_rss_conf.

Even assuming it is permanently allocated and a pointer copy is safe,
pointed data may change and not reflect an applied flow rule anymore.

This patch aligns with testpmd by making a deep copy instead.

Fixes: 18da437b5f63 ("ethdev: add flow rule copy function")
Cc: stable@dpdk.org
Cc: Gaetan Rivet <gaetan.rivet@6wind.com>

Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Acked-by: Nelio Laranjeiro <nelio.laranjeiro@6wind.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
---
 lib/librte_ether/rte_flow.c | 145 +++++++++++++++++++++++++++------------
 1 file changed, 102 insertions(+), 43 deletions(-)

diff --git a/lib/librte_ether/rte_flow.c b/lib/librte_ether/rte_flow.c
index a3823d874..ada280810 100644
--- a/lib/librte_ether/rte_flow.c
+++ b/lib/librte_ether/rte_flow.c
@@ -255,60 +255,119 @@ rte_flow_error_set(struct rte_flow_error *error,
 	return -code;
 }
 
-/** Compute storage space needed by item specification. */
-static void
-flow_item_spec_size(const struct rte_flow_item *item,
-		    size_t *size, size_t *pad)
+/** Pattern item specification types. */
+enum item_spec_type {
+	ITEM_SPEC,
+	ITEM_LAST,
+	ITEM_MASK,
+};
+
+/** Compute storage space needed by item specification and copy it. */
+static size_t
+flow_item_spec_copy(void *buf, const struct rte_flow_item *item,
+		    enum item_spec_type type)
 {
-	if (!item->spec) {
-		*size = 0;
+	size_t size = 0;
+	const void *item_spec =
+		type == ITEM_SPEC ? item->spec :
+		type == ITEM_LAST ? item->last :
+		type == ITEM_MASK ? item->mask :
+		NULL;
+
+	if (!item_spec)
 		goto empty;
-	}
 	switch (item->type) {
 		union {
 			const struct rte_flow_item_raw *raw;
-		} spec;
+		} src;
+		union {
+			struct rte_flow_item_raw *raw;
+		} dst;
 
-	/* Not a fall-through */
 	case RTE_FLOW_ITEM_TYPE_RAW:
-		spec.raw = item->spec;
-		*size = offsetof(struct rte_flow_item_raw, pattern) +
-			spec.raw->length * sizeof(*spec.raw->pattern);
+		src.raw = item_spec;
+		dst.raw = buf;
+		size = offsetof(struct rte_flow_item_raw, pattern) +
+			src.raw->length * sizeof(*src.raw->pattern);
+		if (dst.raw)
+			memcpy(dst.raw, src.raw, size);
 		break;
 	default:
-		*size = rte_flow_desc_item[item->type].size;
+		size = rte_flow_desc_item[item->type].size;
+		if (buf)
+			memcpy(buf, item_spec, size);
 		break;
 	}
 empty:
-	*pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
+	return RTE_ALIGN_CEIL(size, sizeof(double));
 }
 
-/** Compute storage space needed by action configuration. */
-static void
-flow_action_conf_size(const struct rte_flow_action *action,
-		      size_t *size, size_t *pad)
+/** Compute storage space needed by action configuration and copy it. */
+static size_t
+flow_action_conf_copy(void *buf, const struct rte_flow_action *action)
 {
-	if (!action->conf) {
-		*size = 0;
+	size_t size = 0;
+
+	if (!action->conf)
 		goto empty;
-	}
 	switch (action->type) {
 		union {
 			const struct rte_flow_action_rss *rss;
-		} conf;
+		} src;
+		union {
+			struct rte_flow_action_rss *rss;
+		} dst;
+		size_t off;
 
-	/* Not a fall-through. */
 	case RTE_FLOW_ACTION_TYPE_RSS:
-		conf.rss = action->conf;
-		*size = offsetof(struct rte_flow_action_rss, queue) +
-			conf.rss->num * sizeof(*conf.rss->queue);
+		src.rss = action->conf;
+		dst.rss = buf;
+		off = 0;
+		if (dst.rss)
+			*dst.rss = (struct rte_flow_action_rss){
+				.num = src.rss->num,
+			};
+		off += offsetof(struct rte_flow_action_rss, queue);
+		if (src.rss->num) {
+			size = sizeof(*src.rss->queue) * src.rss->num;
+			if (dst.rss)
+				memcpy(dst.rss->queue, src.rss->queue, size);
+			off += size;
+		}
+		off = RTE_ALIGN_CEIL(off, sizeof(double));
+		if (dst.rss) {
+			dst.rss->rss_conf = (void *)((uintptr_t)dst.rss + off);
+			*(struct rte_eth_rss_conf *)(uintptr_t)
+				dst.rss->rss_conf = (struct rte_eth_rss_conf){
+				.rss_key_len = src.rss->rss_conf->rss_key_len,
+				.rss_hf = src.rss->rss_conf->rss_hf,
+			};
+		}
+		off += sizeof(*src.rss->rss_conf);
+		if (src.rss->rss_conf->rss_key_len) {
+			off = RTE_ALIGN_CEIL(off, sizeof(double));
+			size = sizeof(*src.rss->rss_conf->rss_key) *
+				src.rss->rss_conf->rss_key_len;
+			if (dst.rss) {
+				((struct rte_eth_rss_conf *)(uintptr_t)
+				 dst.rss->rss_conf)->rss_key =
+					(void *)((uintptr_t)dst.rss + off);
+				memcpy(dst.rss->rss_conf->rss_key,
+				       src.rss->rss_conf->rss_key,
+				       size);
+			}
+			off += size;
+		}
+		size = off;
 		break;
 	default:
-		*size = rte_flow_desc_action[action->type].size;
+		size = rte_flow_desc_action[action->type].size;
+		if (buf)
+			memcpy(buf, action->conf, size);
 		break;
 	}
 empty:
-	*pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
+	return RTE_ALIGN_CEIL(size, sizeof(double));
 }
 
 /** Store a full rte_flow description. */
@@ -320,7 +379,6 @@ rte_flow_copy(struct rte_flow_desc *desc, size_t len,
 {
 	struct rte_flow_desc *fd = NULL;
 	size_t tmp;
-	size_t pad;
 	size_t off1 = 0;
 	size_t off2 = 0;
 	size_t size = 0;
@@ -345,24 +403,26 @@ rte_flow_copy(struct rte_flow_desc *desc, size_t len,
 				dst = memcpy(fd->data + off1, item,
 					     sizeof(*item));
 			off1 += sizeof(*item);
-			flow_item_spec_size(item, &tmp, &pad);
 			if (item->spec) {
 				if (fd)
-					dst->spec = memcpy(fd->data + off2,
-							   item->spec, tmp);
-				off2 += tmp + pad;
+					dst->spec = fd->data + off2;
+				off2 += flow_item_spec_copy
+					(fd ? fd->data + off2 : NULL, item,
+					 ITEM_SPEC);
 			}
 			if (item->last) {
 				if (fd)
-					dst->last = memcpy(fd->data + off2,
-							   item->last, tmp);
-				off2 += tmp + pad;
+					dst->last = fd->data + off2;
+				off2 += flow_item_spec_copy
+					(fd ? fd->data + off2 : NULL, item,
+					 ITEM_LAST);
 			}
 			if (item->mask) {
 				if (fd)
-					dst->mask = memcpy(fd->data + off2,
-							   item->mask, tmp);
-				off2 += tmp + pad;
+					dst->mask = fd->data + off2;
+				off2 += flow_item_spec_copy
+					(fd ? fd->data + off2 : NULL, item,
+					 ITEM_MASK);
 			}
 			off2 = RTE_ALIGN_CEIL(off2, sizeof(double));
 		} while ((item++)->type != RTE_FLOW_ITEM_TYPE_END);
@@ -387,12 +447,11 @@ rte_flow_copy(struct rte_flow_desc *desc, size_t len,
 				dst = memcpy(fd->data + off1, action,
 					     sizeof(*action));
 			off1 += sizeof(*action);
-			flow_action_conf_size(action, &tmp, &pad);
 			if (action->conf) {
 				if (fd)
-					dst->conf = memcpy(fd->data + off2,
-							   action->conf, tmp);
-				off2 += tmp + pad;
+					dst->conf = fd->data + off2;
+				off2 += flow_action_conf_copy
+					(fd ? fd->data + off2 : NULL, action);
 			}
 			off2 = RTE_ALIGN_CEIL(off2, sizeof(double));
 		} while ((action++)->type != RTE_FLOW_ACTION_TYPE_END);
-- 
2.11.0