[PATCH v2 1/2] lib/graph: lib/graph: fix memset with NULL

[PATCH v2 1/2] lib/graph: lib/graph: fix memset with NULL

[Marat Khalili]

This was flagged by undefined behaviour sanitizer: memset should not be
called with NULL first argument. (memset requires first argument to be
pointer to a memory object, so passing NULL may result in an undefined
behaviour including among other things optimizer potentially removing
code paths depending on stat->xstat_count being NULL.)

Sanitizer message:

    lib/graph/graph_stats.c:473:2: runtime error: null pointer passed as
    argument 1, which is declared to never be null

To fix the issue add a check that stat->xstat_count is not NULL before
the call.

Fixes: 070db97e017 ("graph: support node xstats")

Signed-off-by: Marat Khalili <marat.khalili@huawei.com>
---

v2: Following the suggestions from Jerin Jacob changed the Subject and
added Fixes line.

 lib/graph/graph_stats.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/graph/graph_stats.c b/lib/graph/graph_stats.c
index eac73cbf71..57cd72e7cc 100644
--- a/lib/graph/graph_stats.c
+++ b/lib/graph/graph_stats.c
@@ -470,7 +470,9 @@ cluster_node_arregate_stats(struct cluster_node *cluster, bool dispatch)
 	uint64_t *xstat;
 	uint8_t i;
 
-	memset(stat->xstat_count, 0, sizeof(uint64_t) * stat->xstat_cntrs);
+	if (stat->xstat_count != NULL)
+		memset(stat->xstat_count, 0,
+			sizeof(uint64_t) * stat->xstat_cntrs);
 	for (count = 0; count < cluster->nb_nodes; count++) {
 		node = cluster->nodes[count];
 
-- 
2.43.0

[PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats

[Marat Khalili]

This was flagged by undefined behaviour sanitizer: struct
rte_graph_cluster_stats is declared as `__rte_cache_aligned` but was
allocated using stdlib realloc which caused misaligned allocation. More
than one test needs to be executed in series in order to reproduce the
problem using graph_autotest, e.g:

    app/dpdk-test --no-huge --no-pci -m128 graph_autotest graph_autotest

First sanitizer message  (similar ones follow):

    lib/graph/graph_stats.c:209:13: runtime error: member access within
    misaligned address 0x606000008ea0 for type 'struct
    rte_graph_cluster_stats', which requires 64 byte alignment

To fix the issue remove `__rte_cache_aligned` attribute from struct
rte_graph_cluster_stats and struct rte_graph_cluster_node_stats that it
contains. There is no need to keep them cache-aligned since they are
only used in the slow path.

Fixes: af1ae8b6a32 ("graph: implement stats")

Signed-off-by: Marat Khalili <marat.khalili@huawei.com>
---

v2: Following the suggestions from Jerin Jacob changed the fix to simply
remove non-default alignment. Although there are many ways to make
alignment work, I personally agree that it is probably better to keep
the code simple as long as performance is not an issue.

Although reducing _actual_ alignment of allocated objects would probably
be a binary compatibility issue, here we are only changing _declared_
alignment to match the actual one, so it will more likely fix things
than break them.

devtools/test-meson-builds.sh completes successfully.

 lib/graph/graph_stats.c | 2 +-
 lib/graph/rte_graph.h   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/graph/graph_stats.c b/lib/graph/graph_stats.c
index 57cd72e7cc..4324717f33 100644
--- a/lib/graph/graph_stats.c
+++ b/lib/graph/graph_stats.c
@@ -29,7 +29,7 @@ struct cluster_node {
 	struct rte_node *nodes[];
 };
 
-struct __rte_cache_aligned rte_graph_cluster_stats {
+struct rte_graph_cluster_stats {
 	/* Header */
 	rte_graph_cluster_stats_cb_t fn;
 	uint32_t cluster_node_size; /* Size of struct cluster_node */
diff --git a/lib/graph/rte_graph.h b/lib/graph/rte_graph.h
index 097d0dc9d5..826204cad9 100644
--- a/lib/graph/rte_graph.h
+++ b/lib/graph/rte_graph.h
@@ -202,7 +202,7 @@ struct rte_graph_cluster_stats_param {
  *
  * @see struct rte_graph_cluster_stats_param::fn
  */
-struct __rte_cache_aligned rte_graph_cluster_node_stats {
+struct rte_graph_cluster_node_stats {
 	uint64_t ts;	    /**< Current timestamp. */
 	uint64_t calls;	    /**< Current number of calls made. */
 	uint64_t objs;      /**< Current number of objs processed. */
-- 
2.43.0

Re: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats

[Jerin Jacob]

On Tue, Jun 17, 2025 at 8:44 PM Marat Khalili <marat.khalili@huawei.com> wrote:
>
> This was flagged by undefined behaviour sanitizer: struct
> rte_graph_cluster_stats is declared as `__rte_cache_aligned` but was
> allocated using stdlib realloc which caused misaligned allocation. More
> than one test needs to be executed in series in order to reproduce the
> problem using graph_autotest, e.g:
>
>     app/dpdk-test --no-huge --no-pci -m128 graph_autotest graph_autotest
>
> First sanitizer message  (similar ones follow):
>
>     lib/graph/graph_stats.c:209:13: runtime error: member access within
>     misaligned address 0x606000008ea0 for type 'struct
>     rte_graph_cluster_stats', which requires 64 byte alignment
>
> To fix the issue remove `__rte_cache_aligned` attribute from struct
> rte_graph_cluster_stats and struct rte_graph_cluster_node_stats that it
> contains. There is no need to keep them cache-aligned since they are
> only used in the slow path.
>
> Fixes: af1ae8b6a32 ("graph: implement stats")
>
> Signed-off-by: Marat Khalili <marat.khalili@huawei.com>
> ---
>
> v2: Following the suggestions from Jerin Jacob changed the fix to simply
> remove non-default alignment. Although there are many ways to make
> alignment work, I personally agree that it is probably better to keep
> the code simple as long as performance is not an issue.
>
> Although reducing _actual_ alignment of allocated objects would probably
> be a binary compatibility issue, here we are only changing _declared_
> alignment to match the actual one, so it will more likely fix things
> than break them.
>
> devtools/test-meson-builds.sh completes successfully.
>
>  lib/graph/graph_stats.c | 2 +-
>  lib/graph/rte_graph.h   | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/graph/graph_stats.c b/lib/graph/graph_stats.c
> index 57cd72e7cc..4324717f33 100644
> --- a/lib/graph/graph_stats.c
> +++ b/lib/graph/graph_stats.c
> @@ -29,7 +29,7 @@ struct cluster_node {
>         struct rte_node *nodes[];
>  };
>
> -struct __rte_cache_aligned rte_graph_cluster_stats {
> +struct rte_graph_cluster_stats {
>         /* Header */
>         rte_graph_cluster_stats_cb_t fn;
>         uint32_t cluster_node_size; /* Size of struct cluster_node */
> diff --git a/lib/graph/rte_graph.h b/lib/graph/rte_graph.h
> index 097d0dc9d5..826204cad9 100644
> --- a/lib/graph/rte_graph.h
> +++ b/lib/graph/rte_graph.h
> @@ -202,7 +202,7 @@ struct rte_graph_cluster_stats_param {
>   *
>   * @see struct rte_graph_cluster_stats_param::fn
>   */
> -struct __rte_cache_aligned rte_graph_cluster_node_stats {
> +struct rte_graph_cluster_node_stats {

This is a fastpath structure. No need to change the alignment here.

>         uint64_t ts;        /**< Current timestamp. */
>         uint64_t calls;     /**< Current number of calls made. */
>         uint64_t objs;      /**< Current number of objs processed. */
> --
> 2.43.0
>

RE: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats

[Marat Khalili]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday 17 June 2025 16:28
> 
> > -struct __rte_cache_aligned rte_graph_cluster_node_stats {
> > +struct rte_graph_cluster_node_stats {
> 
> This is a fastpath structure. No need to change the alignment here.

rte_graph_cluster_stats includes it, so unfortunately would stay cache-aligned regardless of the attributes unless we make rte_graph_cluster_node_stats default-aligned as well. If you are sure that we need to keep node one cache-aligned we can return to rte_malloc solution (or posix_memalign, but I would prefer not to hand-code aligned realloc).

Re: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats

[Jerin Jacob]

On Tue, Jun 17, 2025 at 9:11 PM Marat Khalili <marat.khalili@huawei.com> wrote:
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday 17 June 2025 16:28
> >
> > > -struct __rte_cache_aligned rte_graph_cluster_node_stats {
> > > +struct rte_graph_cluster_node_stats {
> >
> > This is a fastpath structure. No need to change the alignment here.
>
> rte_graph_cluster_stats includes it, so unfortunately would stay cache-aligned regardless of the attributes unless we make rte_graph_cluster_node_stats default-aligned as well. If you are sure that we need to keep node one cache-aligned we can return to rte_malloc solution (or posix_memalign, but I would prefer not to hand-code aligned realloc).

I think, existing following code will take care of this. Are you
seeing the sanitizer issue if the change is only updating
rte_graph_cluster_stats alignment?


        /* For a given cluster, max nodes will be the max number of graphs */
        cluster_node_size += cluster->nb_graphs * sizeof(struct rte_node *);
        cluster_node_size = RTE_ALIGN(cluster_node_size, RTE_CACHE_LINE_SIZE);

>

RE: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats

[Marat Khalili]

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday 17 June 2025 16:50
> 
> > > > -struct __rte_cache_aligned rte_graph_cluster_node_stats {
> > > > +struct rte_graph_cluster_node_stats {
> > >
> > > This is a fastpath structure. No need to change the alignment here.
> >
> > rte_graph_cluster_stats includes it, so unfortunately would stay cache-
> aligned regardless of the attributes unless we make
> rte_graph_cluster_node_stats default-aligned as well. If you are sure that
> we need to keep node one cache-aligned we can return to rte_malloc
> solution (or posix_memalign, but I would prefer not to hand-code aligned
> realloc).
> 
> I think, existing following code will take care of this. Are you
> seeing the sanitizer issue if the change is only updating
> rte_graph_cluster_stats alignment?

Yes, still seeing the sanitizer issue. And if fails even before reaching clusters, on the struct rte_graph_cluster_stats itself because I believe alignment propagates from members to enclosing structs.

>         /* For a given cluster, max nodes will be the max number of graphs */
>         cluster_node_size += cluster->nb_graphs * sizeof(struct rte_node *);
>         cluster_node_size = RTE_ALIGN(cluster_node_size,
> RTE_CACHE_LINE_SIZE);

Please correct me if I am wrong, AFAIU it looks like this in memory:
- rte_graph_cluster_stats
- cluster_node
  - rte_graph_cluster_node_stats
- rte_node *[nb_graphs]
- padding of rte_node pointers array to cacheline
- cluster_node
  - rte_graph_cluster_node_stats
- rte_node *[nb_graphs]...

cluster_node_size calculation above ensures correct distance between cluster_node structs due to presence of the rte_node pointers arrays in between, but alignment of the first cluster_node still depends on the alignment of rte_graph_cluster_stats, and each of the next ones depends on the previous. We could of course re-align the first one manually (in each loop, after allocating some extra space), but to me it looks like the job rte_malloc could easier do for us. What do you think would be the right way forward?