patch 'graph: fix ID collisions' has been queued to stable release 23.11.2
Xueming Li
xuemingl at nvidia.com
Fri Jul 12 12:45:02 CEST 2024
Hi,
FYI, your patch has been queued to stable release 23.11.2
Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet.
It will be pushed if I get no objections before 07/14/24. So please
shout if anyone has objections.
Also note that after the patch there's a diff of the upstream commit vs the
patch applied to the branch. This will indicate if there was any rebasing
needed to apply to the stable branch. If there were code changes for rebasing
(ie: not only metadata diffs), please double check that the rebase was
correctly done.
Queued patches are on a temporary branch at:
https://git.dpdk.org/dpdk-stable/log/?h=23.11-staging
This queued commit can be viewed at:
https://git.dpdk.org/dpdk-stable/commit/?h=23.11-staging&id=be7644791c005967d23efd75a4df700aee2c7a8e
Thanks.
Xueming Li <xuemingl at nvidia.com>
---
>From be7644791c005967d23efd75a4df700aee2c7a8e Mon Sep 17 00:00:00 2001
From: Robin Jarry <rjarry at redhat.com>
Date: Tue, 18 Jun 2024 11:23:25 +0200
Subject: [PATCH] graph: fix ID collisions
Cc: Xueming Li <xuemingl at nvidia.com>
[ upstream commit d5c8b6bb74a06051232cbbd92d25220606b280c0 ]
The graph id is determined based on a global variable that is
incremented every time a graph is created, and decremented every time
a graph is destroyed. This only works if graphs are destroyed in the
reverse order in which they have been created.
The following code produces duplicate graph IDs which can lead to
use-after-free bugs and other undefined behaviours:
a = rte_graph_create(...); // id=0 graph_id=1
b = rte_graph_create(...); // id=1 graph_id=2
rte_graph_destroy(a); // graph_id=1
c = rte_graph_create(...); // id=1 graph_id=2 (duplicate with b)
rte_graph_destroy(c); // frees memory still used by b
Remove the global counter. Make sure that the graph list is always
ordered by increasing graph ids. When creating a new graph, pick a free
id which is not allocated.
Update unit tests to ensure it works as expected.
Fixes: a91fecc19c5c ("graph: implement create and destroy")
Signed-off-by: Robin Jarry <rjarry at redhat.com>
Acked-by: Kiran Kumar K <kirankumark at marvell.com>
---
app/test/test_graph.c | 72 ++++++++++++++++++++++++++++++++++++
lib/graph/graph.c | 86 ++++++++++++++++++++++++++++++++++---------
2 files changed, 141 insertions(+), 17 deletions(-)
diff --git a/app/test/test_graph.c b/app/test/test_graph.c
index 3dd017ebfb..eb4f9a61e3 100644
--- a/app/test/test_graph.c
+++ b/app/test/test_graph.c
@@ -695,6 +695,77 @@ test_graph_clone(void)
return ret;
}
+static int
+test_graph_id_collisions(void)
+{
+ static const char *node_patterns[] = {"test_node_source1", "test_node00"};
+ struct rte_graph_param gconf = {
+ .socket_id = SOCKET_ID_ANY,
+ .nb_node_patterns = 2,
+ .node_patterns = node_patterns,
+ };
+ rte_graph_t g1, g2, g3, g4;
+
+ g1 = rte_graph_create("worker1", &gconf);
+ if (g1 == RTE_GRAPH_ID_INVALID) {
+ printf("Graph 1 creation failed with error = %d\n", rte_errno);
+ return -1;
+ }
+ g2 = rte_graph_create("worker2", &gconf);
+ if (g2 == RTE_GRAPH_ID_INVALID) {
+ printf("Graph 2 creation failed with error = %d\n", rte_errno);
+ return -1;
+ }
+ g3 = rte_graph_create("worker3", &gconf);
+ if (g3 == RTE_GRAPH_ID_INVALID) {
+ printf("Graph 3 creation failed with error = %d\n", rte_errno);
+ return -1;
+ }
+ if (g1 == g2 || g2 == g3 || g1 == g3) {
+ printf("Graph ids should be different\n");
+ return -1;
+ }
+ if (rte_graph_destroy(g2) < 0) {
+ printf("Graph 2 suppression failed\n");
+ return -1;
+ }
+ g4 = rte_graph_create("worker4", &gconf);
+ if (g4 == RTE_GRAPH_ID_INVALID) {
+ printf("Graph 4 creation failed with error = %d\n", rte_errno);
+ return -1;
+ }
+ if (g1 == g3 || g1 == g4 || g3 == g4) {
+ printf("Graph ids should be different\n");
+ return -1;
+ }
+ g2 = rte_graph_clone(g1, "worker2", &gconf);
+ if (g2 == RTE_GRAPH_ID_INVALID) {
+ printf("Graph 4 creation failed with error = %d\n", rte_errno);
+ return -1;
+ }
+ if (g1 == g2 || g1 == g3 || g1 == g4 || g2 == g3 || g2 == g4 || g3 == g4) {
+ printf("Graph ids should be different\n");
+ return -1;
+ }
+ if (rte_graph_destroy(g1) < 0) {
+ printf("Graph 1 suppression failed\n");
+ return -1;
+ }
+ if (rte_graph_destroy(g2) < 0) {
+ printf("Graph 2 suppression failed\n");
+ return -1;
+ }
+ if (rte_graph_destroy(g3) < 0) {
+ printf("Graph 3 suppression failed\n");
+ return -1;
+ }
+ if (rte_graph_destroy(g4) < 0) {
+ printf("Graph 4 suppression failed\n");
+ return -1;
+ }
+ return 0;
+}
+
static int
test_graph_model_mcore_dispatch_node_lcore_affinity_set(void)
{
@@ -976,6 +1047,7 @@ static struct unit_test_suite graph_testsuite = {
TEST_CASE(test_lookup_functions),
TEST_CASE(test_create_graph),
TEST_CASE(test_graph_clone),
+ TEST_CASE(test_graph_id_collisions),
TEST_CASE(test_graph_model_mcore_dispatch_node_lcore_affinity_set),
TEST_CASE(test_graph_model_mcore_dispatch_core_bind_unbind),
TEST_CASE(test_graph_worker_model_set_get),
diff --git a/lib/graph/graph.c b/lib/graph/graph.c
index 26f0968a97..8ea2109645 100644
--- a/lib/graph/graph.c
+++ b/lib/graph/graph.c
@@ -19,11 +19,54 @@
static struct graph_head graph_list = STAILQ_HEAD_INITIALIZER(graph_list);
static rte_spinlock_t graph_lock = RTE_SPINLOCK_INITIALIZER;
-static rte_graph_t graph_id;
-
-#define GRAPH_ID_CHECK(id) ID_CHECK(id, graph_id)
/* Private functions */
+static struct graph *
+graph_from_id(rte_graph_t id)
+{
+ struct graph *graph;
+ STAILQ_FOREACH(graph, &graph_list, next) {
+ if (graph->id == id)
+ return graph;
+ }
+ rte_errno = EINVAL;
+ return NULL;
+}
+
+static rte_graph_t
+graph_next_free_id(void)
+{
+ struct graph *graph;
+ rte_graph_t id = 0;
+
+ STAILQ_FOREACH(graph, &graph_list, next) {
+ if (id < graph->id)
+ break;
+ id = graph->id + 1;
+ }
+
+ return id;
+}
+
+static void
+graph_insert_ordered(struct graph *graph)
+{
+ struct graph *after, *g;
+
+ after = NULL;
+ STAILQ_FOREACH(g, &graph_list, next) {
+ if (g->id < graph->id)
+ after = g;
+ else if (g->id > graph->id)
+ break;
+ }
+ if (after == NULL) {
+ STAILQ_INSERT_HEAD(&graph_list, graph, next);
+ } else {
+ STAILQ_INSERT_AFTER(&graph_list, after, graph, next);
+ }
+}
+
struct graph_head *
graph_list_head_get(void)
{
@@ -279,7 +322,8 @@ rte_graph_model_mcore_dispatch_core_bind(rte_graph_t id, int lcore)
{
struct graph *graph;
- GRAPH_ID_CHECK(id);
+ if (graph_from_id(id) == NULL)
+ goto fail;
if (!rte_lcore_is_enabled(lcore))
SET_ERR_JMP(ENOLINK, fail, "lcore %d not enabled", lcore);
@@ -309,7 +353,8 @@ rte_graph_model_mcore_dispatch_core_unbind(rte_graph_t id)
{
struct graph *graph;
- GRAPH_ID_CHECK(id);
+ if (graph_from_id(id) == NULL)
+ goto fail;
STAILQ_FOREACH(graph, &graph_list, next)
if (graph->id == id)
break;
@@ -406,7 +451,7 @@ rte_graph_create(const char *name, struct rte_graph_param *prm)
graph->socket = prm->socket_id;
graph->src_node_count = src_node_count;
graph->node_count = graph_nodes_count(graph);
- graph->id = graph_id;
+ graph->id = graph_next_free_id();
graph->parent_id = RTE_GRAPH_ID_INVALID;
graph->lcore_id = RTE_MAX_LCORE;
graph->num_pkt_to_capture = prm->num_pkt_to_capture;
@@ -422,8 +467,7 @@ rte_graph_create(const char *name, struct rte_graph_param *prm)
goto graph_mem_destroy;
/* All good, Lets add the graph to the list */
- graph_id++;
- STAILQ_INSERT_TAIL(&graph_list, graph, next);
+ graph_insert_ordered(graph);
graph_spinlock_unlock();
return graph->id;
@@ -467,7 +511,6 @@ rte_graph_destroy(rte_graph_t id)
graph_cleanup(graph);
STAILQ_REMOVE(&graph_list, graph, graph, next);
free(graph);
- graph_id--;
goto done;
}
graph = tmp;
@@ -520,7 +563,7 @@ graph_clone(struct graph *parent_graph, const char *name, struct rte_graph_param
graph->parent_id = parent_graph->id;
graph->lcore_id = parent_graph->lcore_id;
graph->socket = parent_graph->socket;
- graph->id = graph_id;
+ graph->id = graph_next_free_id();
/* Allocate the Graph fast path memory and populate the data */
if (graph_fp_mem_create(graph))
@@ -539,8 +582,7 @@ graph_clone(struct graph *parent_graph, const char *name, struct rte_graph_param
goto graph_mem_destroy;
/* All good, Lets add the graph to the list */
- graph_id++;
- STAILQ_INSERT_TAIL(&graph_list, graph, next);
+ graph_insert_ordered(graph);
graph_spinlock_unlock();
return graph->id;
@@ -561,7 +603,8 @@ rte_graph_clone(rte_graph_t id, const char *name, struct rte_graph_param *prm)
{
struct graph *graph;
- GRAPH_ID_CHECK(id);
+ if (graph_from_id(id) == NULL)
+ goto fail;
STAILQ_FOREACH(graph, &graph_list, next)
if (graph->id == id)
return graph_clone(graph, name, prm);
@@ -587,7 +630,8 @@ rte_graph_id_to_name(rte_graph_t id)
{
struct graph *graph;
- GRAPH_ID_CHECK(id);
+ if (graph_from_id(id) == NULL)
+ goto fail;
STAILQ_FOREACH(graph, &graph_list, next)
if (graph->id == id)
return graph->name;
@@ -604,7 +648,8 @@ rte_graph_node_get(rte_graph_t gid, uint32_t nid)
rte_graph_off_t off;
rte_node_t count;
- GRAPH_ID_CHECK(gid);
+ if (graph_from_id(gid) == NULL)
+ goto fail;
STAILQ_FOREACH(graph, &graph_list, next)
if (graph->id == gid) {
rte_graph_foreach_node(count, off, graph->graph,
@@ -729,7 +774,8 @@ graph_scan_dump(FILE *f, rte_graph_t id, bool all)
struct graph *graph;
RTE_VERIFY(f);
- GRAPH_ID_CHECK(id);
+ if (graph_from_id(id) == NULL)
+ goto fail;
STAILQ_FOREACH(graph, &graph_list, next) {
if (all == true) {
@@ -758,7 +804,13 @@ rte_graph_list_dump(FILE *f)
rte_graph_t
rte_graph_max_count(void)
{
- return graph_id;
+ struct graph *graph;
+ rte_graph_t count = 0;
+
+ STAILQ_FOREACH(graph, &graph_list, next)
+ count++;
+
+ return count;
}
RTE_LOG_REGISTER_DEFAULT(rte_graph_logtype, INFO);
--
2.34.1
---
Diff of the applied patch vs upstream commit (please double-check if non-empty:
---
--- - 2024-07-12 18:40:17.566574511 +0800
+++ 0076-graph-fix-ID-collisions.patch 2024-07-12 18:40:14.206594215 +0800
@@ -1 +1 @@
-From d5c8b6bb74a06051232cbbd92d25220606b280c0 Mon Sep 17 00:00:00 2001
+From be7644791c005967d23efd75a4df700aee2c7a8e Mon Sep 17 00:00:00 2001
@@ -4,0 +5,3 @@
+Cc: Xueming Li <xuemingl at nvidia.com>
+
+[ upstream commit d5c8b6bb74a06051232cbbd92d25220606b280c0 ]
@@ -27 +29,0 @@
-Cc: stable at dpdk.org
@@ -37 +39 @@
-index b8409bc604..2840a25b13 100644
+index 3dd017ebfb..eb4f9a61e3 100644
@@ -40 +42 @@
-@@ -696,6 +696,77 @@ test_graph_clone(void)
+@@ -695,6 +695,77 @@ test_graph_clone(void)
@@ -118 +120 @@
-@@ -977,6 +1048,7 @@ static struct unit_test_suite graph_testsuite = {
+@@ -976,6 +1047,7 @@ static struct unit_test_suite graph_testsuite = {
More information about the stable
mailing list