Re: [EXTERNAL] [PATCH v3 14/18] graph: fix unaligned access in stats

[Kiran Kumar Kokkilagadda <kirankumark@marvell.com>]

[-- Attachment #1: Type: text/plain, Size: 14909 bytes --]



Get Outlook for Mac <https://aka.ms/GetOutlookForMac>

From: David Marchand <david.marchand@redhat.com>
Date: Tuesday, 8 July 2025 at 6:01 PM
To: dev@dpdk.org <dev@dpdk.org>
Cc: stable@dpdk.org <stable@dpdk.org>, Jerin Jacob <jerinj@marvell.com>, Kiran Kumar Kokkilagadda <kirankumark@marvell.com>, Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>, Zhirun Yan <yanzhirun_163@163.com>, Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>
Subject: [EXTERNAL] [PATCH v3 14/18] graph: fix unaligned access in stats
UBSan reports: .. /lib/graph/graph_stats. c: 208: 13: runtime error: member access within misaligned address 0x000054742c50 for type 'struct rte_graph_cluster_stats', which requires 64 byte alignment .. /lib/graph/graph_stats. c: 257: 12: runtime error: 
ZjQcmQRYFpfptBannerStart
Prioritize security for external emails:
Confirm sender and content safety before clicking links or opening attachments
<https://us-phishalarm-ewt.proofpoint.com/EWT/v1/CRVmXkqW!to3Z1f8UAnU69Q-c-ZxaLqw4Hrf0nRhoZPfYuYT9mJ7YnAXilGrf--fL-Zm7l1VQHd3bjUXwIMnvcu_suMpHO0hzN2TUxcSNBPH6eYM8qA$>
Report Suspicious <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/CRVmXkqW!to3Z1f8UAnU69Q-c-ZxaLqw4Hrf0nRhoZPfYuYT9mJ7YnAXilGrf--fL-Zm7l1VQHd3bjUXwIMnvcu_suMpHO0hzN2TUxcSNBPH6eYM8qA$>


ZjQcmQRYFpfptBannerEnd

UBSan reports:



    ../lib/graph/graph_stats.c:208:13: runtime error:

            member access within misaligned address 0x000054742c50

            for type 'struct rte_graph_cluster_stats',

            which requires 64 byte alignment



    ../lib/graph/graph_stats.c:257:12: runtime error:

            member access within misaligned address 0x00002219fd30

            for type 'struct rte_graph_cluster_stats',

            which requires 64 byte alignment



The current code goes into various complex (non aligned) reallocations /

memset / memcpy.



Simplify this by computing how many nodes are present in the

cluster of graphes.

Then directly call rte_malloc for the whole stats object.



As a bonus, this change also fixes leaks:

- if any error occurred before call to rte_malloc, since the xstats

  objects stored in the glibc allocated stats object were not freed,

- if an allocation failure occurs, with constructs using

  ptr = realloc(ptr, sz), since the original ptr is lost,



Fixes: af1ae8b6a32c ("graph: implement stats")

Cc: stable@dpdk.org



Signed-off-by: David Marchand <david.marchand@redhat.com>





Acked-by: Kiran Kumar Kokkilagadda <kirankumark@marvell.com<mailto:kirankumark@marvell.com>>



 lib/graph/graph_stats.c | 102 +++++++++++++++++++++++-----------------

 1 file changed, 58 insertions(+), 44 deletions(-)



diff --git a/lib/graph/graph_stats.c b/lib/graph/graph_stats.c

index 583ad8dbd5..e9b183bf13 100644

--- a/lib/graph/graph_stats.c

+++ b/lib/graph/graph_stats.c

@@ -37,7 +37,6 @@ struct __rte_cache_aligned rte_graph_cluster_stats {

               int socket_id;

               bool dispatch;

               void *cookie;

-              size_t sz;



               struct cluster_node clusters[];

 };

@@ -178,15 +177,55 @@ graph_cluster_stats_cb_dispatch(bool is_first, bool is_last, void *cookie,

               return graph_cluster_stats_cb(true, is_first, is_last, cookie, stat);

 };



+static uint32_t

+cluster_count_nodes(const struct cluster *cluster)

+{

+             rte_node_t *nodes = NULL;

+             uint32_t max_nodes = 0;

+

+             for (unsigned int i = 0; i < cluster->nb_graphs; i++) {

+                            struct graph_node *graph_node;

+

+                            STAILQ_FOREACH(graph_node, &cluster->graphs[i]->node_list, next) {

+                                            unsigned int n;

+

+                                            for (n = 0; n < max_nodes; n++) {

+                                                           if (nodes[n] != graph_node->node->id)

+                                                                          continue;

+                                                           break;

+                                            }

+                                            if (n == max_nodes) {

+                                                           rte_node_t *new_nodes;

+

+                                                           max_nodes++;

+                                                           new_nodes = realloc(nodes, max_nodes * sizeof(nodes[0]));

+                                                           if (new_nodes == NULL) {

+                                                                          free(nodes);

+                                                                          return 0;

+                                                           }

+                                                           nodes = new_nodes;

+                                                           nodes[n] = graph_node->node->id;

+                                            }

+                            }

+             }

+             free(nodes);

+

+             return max_nodes;

+}

+

 static struct rte_graph_cluster_stats *

 stats_mem_init(struct cluster *cluster,

                      const struct rte_graph_cluster_stats_param *prm)

 {

-              size_t sz = sizeof(struct rte_graph_cluster_stats);

               struct rte_graph_cluster_stats *stats;

               rte_graph_cluster_stats_cb_t fn;

               int socket_id = prm->socket_id;

               uint32_t cluster_node_size;

+             uint32_t max_nodes;

+

+             max_nodes = cluster_count_nodes(cluster);

+             if (max_nodes == 0)

+                            return NULL;



               /* Fix up callback */

               fn = prm->fn;

@@ -203,25 +242,23 @@ stats_mem_init(struct cluster *cluster,

               cluster_node_size += cluster->nb_graphs * sizeof(struct rte_node *);

               cluster_node_size = RTE_ALIGN(cluster_node_size, RTE_CACHE_LINE_SIZE);



-              stats = realloc(NULL, sz);

+             stats = rte_zmalloc_socket(NULL, sizeof(struct rte_graph_cluster_stats) +

+                            max_nodes * cluster_node_size, 0, socket_id);

               if (stats) {

-                             memset(stats, 0, sz);

                               stats->fn = fn;

                               stats->cluster_node_size = cluster_node_size;

                               stats->max_nodes = 0;

                               stats->socket_id = socket_id;

                               stats->cookie = prm->cookie;

-                             stats->sz = sz;

               }



               return stats;

 }



 static int

-stats_mem_populate(struct rte_graph_cluster_stats **stats_in,

+stats_mem_populate(struct rte_graph_cluster_stats *stats,

                                  struct rte_graph *graph, struct graph_node *graph_node)

 {

-              struct rte_graph_cluster_stats *stats = *stats_in;

               rte_node_t id = graph_node->node->id;

               struct cluster_node *cluster;

               struct rte_node *node;

@@ -247,21 +284,12 @@ stats_mem_populate(struct rte_graph_cluster_stats **stats_in,

                               cluster = RTE_PTR_ADD(cluster, stats->cluster_node_size);

               }



-              /* Hey, it is a new node, allocate space for it in the reel */

-              stats = realloc(stats, stats->sz + stats->cluster_node_size);

-              if (stats == NULL)

-                             SET_ERR_JMP(ENOMEM, err, "Realloc failed");

-              *stats_in = NULL;

-

-              /* Clear the new struct cluster_node area */

-              cluster = RTE_PTR_ADD(stats, stats->sz),

-              memset(cluster, 0, stats->cluster_node_size);

               memcpy(cluster->stat.name, graph_node->node->name, RTE_NODE_NAMESIZE);

               cluster->stat.id = graph_node->node->id;

               cluster->stat.hz = rte_get_timer_hz();

               node = graph_node_id_to_ptr(graph, id);

               if (node == NULL)

-                             SET_ERR_JMP(ENOENT, free, "Failed to find node %s in graph %s",

+                            SET_ERR_JMP(ENOENT, err, "Failed to find node %s in graph %s",

                                                  graph_node->node->name, graph->name);

               cluster->nodes[cluster->nb_nodes++] = node;

               if (graph_node->node->xstats) {

@@ -270,7 +298,7 @@ stats_mem_populate(struct rte_graph_cluster_stats **stats_in,

                                              sizeof(uint64_t) * graph_node->node->xstats->nb_xstats,

                                              RTE_CACHE_LINE_SIZE, stats->socket_id);

                               if (cluster->stat.xstat_count == NULL)

-                                             SET_ERR_JMP(ENOMEM, free, "Failed to allocate memory node %s graph %s",

+                                            SET_ERR_JMP(ENOMEM, err, "Failed to allocate memory node %s graph %s",

                                                                 graph_node->node->name, graph->name);



                               cluster->stat.xstat_desc = rte_zmalloc_socket(NULL,

@@ -278,7 +306,7 @@ stats_mem_populate(struct rte_graph_cluster_stats **stats_in,

                                              RTE_CACHE_LINE_SIZE, stats->socket_id);

                               if (cluster->stat.xstat_desc == NULL) {

                                              rte_free(cluster->stat.xstat_count);

-                                             SET_ERR_JMP(ENOMEM, free, "Failed to allocate memory node %s graph %s",

+                                            SET_ERR_JMP(ENOMEM, err, "Failed to allocate memory node %s graph %s",

                                                                 graph_node->node->name, graph->name);

                               }



@@ -288,30 +316,20 @@ stats_mem_populate(struct rte_graph_cluster_stats **stats_in,

                                                                            RTE_NODE_XSTAT_DESC_SIZE) < 0) {

                                                             rte_free(cluster->stat.xstat_count);

                                                             rte_free(cluster->stat.xstat_desc);

-                                                            SET_ERR_JMP(E2BIG, free,

+                                                           SET_ERR_JMP(E2BIG, err,

                                                                                "Error description overflow node %s graph %s",

                                                                                graph_node->node->name, graph->name);

                                              }

                               }

               }



-              stats->sz += stats->cluster_node_size;

               stats->max_nodes++;

-              *stats_in = stats;



               return 0;

-free:

-              free(stats);

 err:

               return -rte_errno;

 }



-static void

-stats_mem_fini(struct rte_graph_cluster_stats *stats)

-{

-              free(stats);

-}

-

 static void

 cluster_init(struct cluster *cluster)

 {

@@ -381,10 +399,7 @@ struct rte_graph_cluster_stats *

 rte_graph_cluster_stats_create(const struct rte_graph_cluster_stats_param *prm)

 {

               struct rte_graph_cluster_stats *stats, *rc = NULL;

-              struct graph_node *graph_node;

               struct cluster cluster;

-              struct graph *graph;

-              const char *pattern;

               rte_graph_t i;



               /* Sanity checks */

@@ -402,37 +417,36 @@ rte_graph_cluster_stats_create(const struct rte_graph_cluster_stats_param *prm)

               graph_spinlock_lock();

               /* Expand graph pattern and add the graph to the cluster */

               for (i = 0; i < prm->nb_graph_patterns; i++) {

-                             pattern = prm->graph_patterns[i];

-                             if (expand_pattern_to_cluster(&cluster, pattern))

+                            if (expand_pattern_to_cluster(&cluster, prm->graph_patterns[i]))

                                              goto bad_pattern;

               }



               /* Alloc the stats memory */

               stats = stats_mem_init(&cluster, prm);

               if (stats == NULL)

-                             SET_ERR_JMP(ENOMEM, bad_pattern, "Failed alloc stats memory");

+                            SET_ERR_JMP(ENOMEM, bad_pattern, "Failed rte_malloc for stats memory");



               /* Iterate over M(Graph) x N (Nodes in graph) */

               for (i = 0; i < cluster.nb_graphs; i++) {

+                            struct graph_node *graph_node;

+                            struct graph *graph;

+

                               graph = cluster.graphs[i];

                               STAILQ_FOREACH(graph_node, &graph->node_list, next) {

                                              struct rte_graph *graph_fp = graph->graph;

-                                             if (stats_mem_populate(&stats, graph_fp, graph_node))

+                                            if (stats_mem_populate(stats, graph_fp, graph_node))

                                                             goto realloc_fail;

                               }

                               if (graph->graph->model == RTE_GRAPH_MODEL_MCORE_DISPATCH)

                                              stats->dispatch = true;

               }



-              /* Finally copy to hugepage memory to avoid pressure on rte_realloc */

-              rc = rte_malloc_socket(NULL, stats->sz, 0, stats->socket_id);

-              if (rc)

-                             rte_memcpy(rc, stats, stats->sz);

-              else

-                             SET_ERR_JMP(ENOMEM, realloc_fail, "rte_malloc failed");

+             rc = stats;

+             stats = NULL;



 realloc_fail:

-              stats_mem_fini(stats);

+             if (stats != NULL)

+                            rte_graph_cluster_stats_destroy(stats);

 bad_pattern:

               graph_spinlock_unlock();

               cluster_fini(&cluster);

--

2.50.0



[-- Attachment #2: Type: text/html, Size: 70736 bytes --]