diff mbox series

[2/3] ethdev: fix memory leak when telemetry xstats

Message ID 20220416010747.40714-3-fengchengwen@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series bugfix for ethdev telemetry |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

fengchengwen April 16, 2022, 1:07 a.m. UTC 
  The 'eth_xstats' should be freed after setup telemetry dictionary. This
patch fixes it.

Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
Cc: stable@dpdk.org

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Andrew Rybchenko April 21, 2022, 6:51 a.m. UTC | #1 
On 4/16/22 04:07, Chengwen Feng wrote:
> The 'eth_xstats' should be freed after setup telemetry dictionary. This
> patch fixes it.
> 
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
David Marchand April 21, 2022, 8:09 a.m. UTC | #2 
On Sat, Apr 16, 2022 at 3:14 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> The 'eth_xstats' should be freed after setup telemetry dictionary. This
> patch fixes it.
>
> Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> Cc: stable@dpdk.org
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/ethdev/rte_ethdev.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 615383bde2..df20433c2d 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -5585,6 +5585,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
>         for (i = 0; i < num_xstats; i++)
>                 rte_tel_data_add_dict_u64(d, xstat_names[i].name,
>                                 eth_xstats[i].value);
> +       free(eth_xstats);
>         return 0;
>  }
>

We need some minimal testing for telemetry commands.

It could be a test automatically calling all available /ethdev/
commands on a running testpmd.
This test could be really simple, not even checking what is returned.
It would just try every command sequentially with no parameter first,
then with port 0 and finally with port 1.


Coupled with ASan in the CI, this current issue would have been caught.
For example, I tested manually with testpmd + net/null ports:

==3825787==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1040 byte(s) in 1 object(s) allocated from:
    #0 0x7f7048a2d91f in __interceptor_malloc (/usr/lib64/libasan.so.6+0xae91f)
    #1 0x32859e9 in eth_dev_handle_port_xstats
(/home/dmarchan/dpdk/build-gcc-asan/app/dpdk-testpmd+0x32859e9)
    #2 0x3346ac9 in perform_command
(/home/dmarchan/dpdk/build-gcc-asan/app/dpdk-testpmd+0x3346ac9)
    #3 0x3347a8e in client_handler
(/home/dmarchan/dpdk/build-gcc-asan/app/dpdk-testpmd+0x3347a8e)
    #4 0x7f7045751b19 in start_thread
/usr/src/debug/glibc-2.34-25.fc35.x86_64/nptl/pthread_create.c:443



Opinions?
Bruce Richardson April 21, 2022, 9:03 a.m. UTC | #3 
On Thu, Apr 21, 2022 at 10:09:56AM +0200, David Marchand wrote:
> On Sat, Apr 16, 2022 at 3:14 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
> >
> > The 'eth_xstats' should be freed after setup telemetry dictionary. This
> > patch fixes it.
> >
> > Fixes: c190daedb9b1 ("ethdev: add telemetry callbacks")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> >  lib/ethdev/rte_ethdev.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index 615383bde2..df20433c2d 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -5585,6 +5585,7 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
> >         for (i = 0; i < num_xstats; i++)
> >                 rte_tel_data_add_dict_u64(d, xstat_names[i].name,
> >                                 eth_xstats[i].value);
> > +       free(eth_xstats);
> >         return 0;
> >  }
> >
> 
> We need some minimal testing for telemetry commands.
> 
> It could be a test automatically calling all available /ethdev/
> commands on a running testpmd.
> This test could be really simple, not even checking what is returned.
> It would just try every command sequentially with no parameter first,
> then with port 0 and finally with port 1.
> 

That seems reasonable. However, I'd go a little further and have all
available commands called as an initial sanity check. Then we can use some
heuristics to go further, with the *dev/stats commands or xstats commands
all being called with numeric parameters as you suggest.

/Bruce
David Marchand April 22, 2022, 8:14 a.m. UTC | #4 
On Thu, Apr 21, 2022 at 11:04 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > We need some minimal testing for telemetry commands.
> >
> > It could be a test automatically calling all available /ethdev/
> > commands on a running testpmd.
> > This test could be really simple, not even checking what is returned.
> > It would just try every command sequentially with no parameter first,
> > then with port 0 and finally with port 1.
> >
>
> That seems reasonable. However, I'd go a little further and have all
> available commands called as an initial sanity check. Then we can use some
> heuristics to go further, with the *dev/stats commands or xstats commands
> all being called with numeric parameters as you suggest.

Ok, lgtm too.
Just to be clear, I don't have the time to work on this, so this is
open to volunteers.
diff mbox series

Patch

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 615383bde2..df20433c2d 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5585,6 +5585,7 @@  eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	for (i = 0; i < num_xstats; i++)
 		rte_tel_data_add_dict_u64(d, xstat_names[i].name,
 				eth_xstats[i].value);
+	free(eth_xstats);
 	return 0;
 }