From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AE1CF454E9 for ; Tue, 25 Jun 2024 02:02:33 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A84A8410DC; Tue, 25 Jun 2024 02:02:33 +0200 (CEST) Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) by mails.dpdk.org (Postfix) with ESMTP id E7DFB40EDD for ; Tue, 25 Jun 2024 02:02:32 +0200 (CEST) Received: by mail-lf1-f51.google.com with SMTP id 2adb3069b0e04-52cd6784aa4so3882250e87.3 for ; Mon, 24 Jun 2024 17:02:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1719273752; x=1719878552; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=Xhns+MY+iNPnjV4dRFsriuI8p8peCfmgzbn5Okt3wa4=; b=YZPAOD3kR0MmoEmxxvTeKbUq9FEY1gzBJxO9lHeI6S6av6npljoVXmlU6pQTeGfgMa qLE5W9TIcBOHDNTkiHGU5zYZyoCpwVe3kMHTCkIbpfBZUZ2Yu8xNAR3MlxgtgPcFGYUV H/vdedqr7oGMmOWt5z6MLEKvfmuSE/TG05N0zcDl8FzSC5rMIk9B943bseqFcULYmCWf gnjYEI5sctMWN5kNCoyjbnlC5tQhd540pBpeh0Ssc8Us2G0JsGPdOzU1r1SJ/tQgMmfM q+1N2CuGuLxx4vWnvh3oPdTM5VbQH4NWqCZNnzmWXw0J44d4FxPdvt9QvUGNMaHU1Bsw F0Ug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719273752; x=1719878552; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=Xhns+MY+iNPnjV4dRFsriuI8p8peCfmgzbn5Okt3wa4=; b=EYEW4qoptOWWnN9qQHWm0CyC4dHzyWBZTQ8AvvrxxwN6yy47xRMQWUaSvgB+jOr2l4 NqHIcrCDZgAMRPXr4YkxpjZWOC/y8NLxP4YJkiv4qjs1IkKek++U/sBqZ2f5AvvNYCXX ZFpZWZWTG0QBlWfnzpIPPyxLFqVEgI1MzAxNFj6F4DkEZlJdXVwd695Y33aK5sFJ/S9j UXWVi0NMFLTFXg6AhVnBZUxC78fLQNz6Mag0Su/MYgTHDgR1jtzMR30RLmU/NrpaDexc 0tctipzds77tWAyoh/lilraz2d9YWcJMxVKlerXh/bnAnZAywDz5xplNMLvQDB5i8ws0 lG5A== X-Forwarded-Encrypted: i=1; AJvYcCV0yhxtiBDoj0GaRKH7JyPHI+d5ePWlM+hViep9DJqH8HhwaZkeCQ2WuxgTjaBIcyHx1tnvkivDrO8YfulBfZ4= X-Gm-Message-State: AOJu0YyOjyB2wQZ+5vSAyM74MUC+4z0LvW1wuEtFM6227JFfvccYS3ZL lV+Lp8h3thfzhR/ydOp43uuCOJZ2dunyoeQP2EWFleTLXdAP0EYv X-Google-Smtp-Source: AGHT+IHjb+c9eHv4nnhIYnAmVjMCF2a4kNzLXMEmeU36QADlGbOGuB6veFn2Abv0yO4g8RQoq3I6Ng== X-Received: by 2002:a05:6512:4c2:b0:52c:ce9a:2a90 with SMTP id 2adb3069b0e04-52ce1839949mr3318610e87.40.1719273752000; Mon, 24 Jun 2024 17:02:32 -0700 (PDT) Received: from localhost ([2a01:4b00:d036:ae00:7aef:1aaa:3dff:d546]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-424a2abcfd0sm755015e9.1.2024.06.24.17.02.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Jun 2024 17:02:31 -0700 (PDT) From: luca.boccassi@gmail.com To: Robin Jarry Cc: Kiran Kumar K , dpdk stable Subject: patch 'graph: fix ID collisions' has been queued to stable release 22.11.6 Date: Tue, 25 Jun 2024 00:58:47 +0100 Message-Id: <20240624235907.885628-61-luca.boccassi@gmail.com> X-Mailer: git-send-email 2.39.2 In-Reply-To: <20240624235907.885628-1-luca.boccassi@gmail.com> References: <20240624235907.885628-1-luca.boccassi@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Hi, FYI, your patch has been queued to stable release 22.11.6 Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet. It will be pushed if I get no objections before 06/27/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://github.com/bluca/dpdk-stable This queued commit can be viewed at: https://github.com/bluca/dpdk-stable/commit/145220677b0b20859be722c4cc6f90d882ce395d Thanks. Luca Boccassi --- >From 145220677b0b20859be722c4cc6f90d882ce395d Mon Sep 17 00:00:00 2001 From: Robin Jarry Date: Tue, 18 Jun 2024 11:23:25 +0200 Subject: [PATCH] graph: fix ID collisions [ 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 Acked-by: Kiran Kumar K --- lib/graph/graph.c | 72 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 11 deletions(-) diff --git a/lib/graph/graph.c b/lib/graph/graph.c index 3a617cc369..9027e62a26 100644 --- a/lib/graph/graph.c +++ b/lib/graph/graph.c @@ -18,11 +18,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) { @@ -327,7 +370,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(); /* Allocate the Graph fast path memory and populate the data */ if (graph_fp_mem_create(graph)) @@ -338,8 +381,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; @@ -378,7 +420,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; @@ -405,7 +446,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; @@ -422,7 +464,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, @@ -547,7 +590,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) { @@ -576,7 +620,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.39.2 --- Diff of the applied patch vs upstream commit (please double-check if non-empty: --- --- - 2024-06-25 00:22:16.877740054 +0100 +++ 0061-graph-fix-ID-collisions.patch 2024-06-25 00:22:13.213186220 +0100 @@ -1 +1 @@ -From d5c8b6bb74a06051232cbbd92d25220606b280c0 Mon Sep 17 00:00:00 2001 +From 145220677b0b20859be722c4cc6f90d882ce395d Mon Sep 17 00:00:00 2001 @@ -5,0 +6,2 @@ +[ upstream commit d5c8b6bb74a06051232cbbd92d25220606b280c0 ] + @@ -27 +28,0 @@ -Cc: stable@dpdk.org @@ -32,3 +33,2 @@ - app/test/test_graph.c | 72 ++++++++++++++++++++++++++++++++++++ - lib/graph/graph.c | 86 ++++++++++++++++++++++++++++++++++--------- - 2 files changed, 141 insertions(+), 17 deletions(-) + lib/graph/graph.c | 72 +++++++++++++++++++++++++++++++++++++++-------- + 1 file changed, 61 insertions(+), 11 deletions(-) @@ -36,90 +35,0 @@ -diff --git a/app/test/test_graph.c b/app/test/test_graph.c -index b8409bc604..2840a25b13 100644 ---- a/app/test/test_graph.c -+++ b/app/test/test_graph.c -@@ -696,6 +696,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) - { -@@ -977,6 +1048,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), @@ -127 +37 @@ -index 26f0968a97..8ea2109645 100644 +index 3a617cc369..9027e62a26 100644 @@ -130 +40 @@ -@@ -19,11 +19,54 @@ +@@ -18,11 +18,54 @@ @@ -188,21 +98 @@ -@@ -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) +@@ -327,7 +370,7 @@ rte_graph_create(const char *name, struct rte_graph_param *prm) @@ -214,4 +104,4 @@ - 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) + + /* Allocate the Graph fast path memory and populate the data */ + if (graph_fp_mem_create(graph)) +@@ -338,8 +381,7 @@ rte_graph_create(const char *name, struct rte_graph_param *prm) @@ -227 +117 @@ -@@ -467,7 +511,6 @@ rte_graph_destroy(rte_graph_t id) +@@ -378,7 +420,6 @@ rte_graph_destroy(rte_graph_t id) @@ -235,30 +125 @@ -@@ -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) +@@ -405,7 +446,8 @@ rte_graph_id_to_name(rte_graph_t id) @@ -274 +135 @@ -@@ -604,7 +648,8 @@ rte_graph_node_get(rte_graph_t gid, uint32_t nid) +@@ -422,7 +464,8 @@ rte_graph_node_get(rte_graph_t gid, uint32_t nid) @@ -284 +145 @@ -@@ -729,7 +774,8 @@ graph_scan_dump(FILE *f, rte_graph_t id, bool all) +@@ -547,7 +590,8 @@ graph_scan_dump(FILE *f, rte_graph_t id, bool all) @@ -294 +155 @@ -@@ -758,7 +804,13 @@ rte_graph_list_dump(FILE *f) +@@ -576,7 +620,13 @@ rte_graph_list_dump(FILE *f)