[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?

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

[Marat Khalili]

Gently reminding about this issue, I still hope to see graph tests passing under sanitizer.

We cannot just remove __rte_cache_aligned from rte_graph_cluster_stats without removing it from rte_graph_cluster_node_stats because of how C works. But we can perhaps use rte_malloc to align our allocations in a way that matches the declaration. Would it be an ok solution?

> -----Original Message-----
> From: Marat Khalili <marat.khalili@huawei.com>
> Sent: Tuesday 17 June 2025 17:40
> To: Jerin Jacob <jerinjacobk@gmail.com>
> Cc: Jerin Jacob <jerinj@marvell.com>; Kiran Kumar K
> <kirankumark@marvell.com>; Nithin Dabilpuram
> <ndabilpuram@marvell.com>; Zhirun Yan <yanzhirun_163@163.com>; Pavan
> Nikhilesh <pbhagavatula@marvell.com>; dev@dpdk.org
> Subject: RE: [PATCH v2 2/2] lib/graph: default-align rte_graph_cluster_stats
> 
> > -----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?

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

[David Marchand]

On Tue, Jun 17, 2025 at 5:14 PM Marat Khalili <marat.khalili@huawei.com> wrote:
>
> 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)

I would check against stat->xstat_cntrs since the array is allocated
based on this count.
This will be more consistent with the loop on this same counter later
in this function.


> +               memset(stat->xstat_count, 0,
> +                       sizeof(uint64_t) * stat->xstat_cntrs);

No need for going to a new line.


>         for (count = 0; count < cluster->nb_nodes; count++) {
>                 node = cluster->nodes[count];
>

Thanks for the fix.


-- 
David Marchand

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

[David Marchand]

On Mon, Jun 23, 2025 at 10:53 AM Marat Khalili <marat.khalili@huawei.com> wrote:
>
> Gently reminding about this issue, I still hope to see graph tests passing under sanitizer.

Thanks for trying to fix this, but without coverage on the topic, it
is likely we will have new errors in the future.

Now that the trace framework has been fixed (856aef55de95 ("trace: fix
undefined behavior in register")), enabling UBSan in the CI has become
easier.
Could you spend some time on reviewing patches on the topic?
Like for example: https://patchwork.dpdk.org/project/dpdk/list/?series=35509


>
> We cannot just remove __rte_cache_aligned from rte_graph_cluster_stats without removing it from rte_graph_cluster_node_stats because of how C works. But we can perhaps use rte_malloc to align our allocations in a way that matches the declaration. Would it be an ok solution?

rte_malloc is not required.
I have a fix in an branch of mine, that makes use of aligned_alloc
(which is C11), you need to manually copy data in stats_mem_populate.


-- 
David Marchand

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

[Marat Khalili]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Monday 23 June 2025 13:08
> 
> Could you spend some time on reviewing patches on the topic?
> Like for example:
> https://patchwork.dpdk.org/project/dpdk/list/?series=35509

Sure, will look!

> rte_malloc is not required.
> I have a fix in an branch of mine, that makes use of aligned_alloc
> (which is C11), you need to manually copy data in stats_mem_populate.

Sure aligned_alloc and manual copy for realloc should work as well. I will then re-submit addressing your comments to 1/2 and dropping 2/2, and will wait for your fix. Thanks for the reviews!

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

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, 23 June 2025 14.08
> 
> On Mon, Jun 23, 2025 at 10:53 AM Marat Khalili <marat.khalili@huawei.com>
> wrote:
> >
> > We cannot just remove __rte_cache_aligned from rte_graph_cluster_stats
> without removing it from rte_graph_cluster_node_stats because of how C works.
> But we can perhaps use rte_malloc to align our allocations in a way that
> matches the declaration. Would it be an ok solution?
> 
> rte_malloc is not required.
> I have a fix in an branch of mine, that makes use of aligned_alloc
> (which is C11), you need to manually copy data in stats_mem_populate.

MSVC doesn't support aligned_alloc().
For MSVC, you have to use _aligned_malloc() and _aligned_free(), like in the "lcore variables" library:
https://elixir.bootlin.com/dpdk/v25.03/source/lib/eal/common/eal_common_lcore_var.c