app/testpmd: fix action destruction memory leak
Checks
Commit Message
In case action handle destroy fails, the job memory was not freed
properly. This commit fixes the possible memory leak in the action
handle destruction failed case.
Fixes: c9dc03840873 ("ethdev: add indirect action async query")
Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
---
app/test-pmd/config.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
On Thu, Nov 17, 2022 at 9:56 AM Suanming Mou <suanmingm@nvidia.com> wrote:
>
> In case action handle destroy fails, the job memory was not freed
> properly. This commit fixes the possible memory leak in the action
> handle destruction failed case.
>
> Fixes: c9dc03840873 ("ethdev: add indirect action async query")
>
Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
LGTM.
Reviewed-by: David Marchand <david.marchand@redhat.com>
Thanks.
On 11/17/2022 8:55 AM, Suanming Mou wrote:
> In case action handle destroy fails, the job memory was not freed
> properly. This commit fixes the possible memory leak in the action
> handle destruction failed case.
>
> Fixes: c9dc03840873 ("ethdev: add indirect action async query")
>
> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> ---
> app/test-pmd/config.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 982549ffed..719bdd4261 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -2873,9 +2873,9 @@ port_queue_action_handle_destroy(portid_t port_id,
> job->type = QUEUE_JOB_TYPE_ACTION_DESTROY;
> job->pia = pia;
>
> - if (pia->handle &&
> - rte_flow_async_action_handle_destroy(port_id,
> + if (rte_flow_async_action_handle_destroy(port_id,
Why 'pia->handle' check removed, was it unnecessary to check it at first
place?
> queue_id, &attr, pia->handle, job, &error)) {
> + free(job);
> ret = port_flow_complain(&error);
> continue;
> }
Just to double check, when this if branch not taken,
'rte_flow_async_action_handle_destroy()' not failed case, testpmd
'port_queue_flow_pull()' functions frees the 'job', right?
Hi,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Friday, November 18, 2022 6:40 PM
> To: Suanming Mou <suanmingm@nvidia.com>; david.marchand@redhat.com;
> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
> <yuying.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH] app/testpmd: fix action destruction memory leak
>
> On 11/17/2022 8:55 AM, Suanming Mou wrote:
> > In case action handle destroy fails, the job memory was not freed
> > properly. This commit fixes the possible memory leak in the action
> > handle destruction failed case.
> >
> > Fixes: c9dc03840873 ("ethdev: add indirect action async query")
> >
> > Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
> > ---
> > app/test-pmd/config.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 982549ffed..719bdd4261 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -2873,9 +2873,9 @@ port_queue_action_handle_destroy(portid_t
> port_id,
> > job->type = QUEUE_JOB_TYPE_ACTION_DESTROY;
> > job->pia = pia;
> >
> > - if (pia->handle &&
> > - rte_flow_async_action_handle_destroy(port_id,
> > + if (rte_flow_async_action_handle_destroy(port_id,
>
> Why 'pia->handle' check removed, was it unnecessary to check it at first place?
>
> > queue_id, &attr, pia->handle, job, &error)) {
> > + free(job);
> > ret = port_flow_complain(&error);
> > continue;
> > }
>
> Just to double check, when this if branch not taken,
> 'rte_flow_async_action_handle_destroy()' not failed case, testpmd
> 'port_queue_flow_pull()' functions frees the 'job', right?
Yes, port_queue_flow_pull() will free the 'job'.
On 11/18/2022 12:21 PM, Suanming Mou wrote:
> Hi,
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Friday, November 18, 2022 6:40 PM
>> To: Suanming Mou <suanmingm@nvidia.com>; david.marchand@redhat.com;
>> Aman Singh <aman.deep.singh@intel.com>; Yuying Zhang
>> <yuying.zhang@intel.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH] app/testpmd: fix action destruction memory leak
>>
>> On 11/17/2022 8:55 AM, Suanming Mou wrote:
>>> In case action handle destroy fails, the job memory was not freed
>>> properly. This commit fixes the possible memory leak in the action
>>> handle destruction failed case.
>>>
>>> Fixes: c9dc03840873 ("ethdev: add indirect action async query")
>>>
>>> Signed-off-by: Suanming Mou <suanmingm@nvidia.com>
>>> ---
>>> app/test-pmd/config.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
>>> 982549ffed..719bdd4261 100644
>>> --- a/app/test-pmd/config.c
>>> +++ b/app/test-pmd/config.c
>>> @@ -2873,9 +2873,9 @@ port_queue_action_handle_destroy(portid_t
>> port_id,
>>> job->type = QUEUE_JOB_TYPE_ACTION_DESTROY;
>>> job->pia = pia;
>>>
>>> - if (pia->handle &&
>>> - rte_flow_async_action_handle_destroy(port_id,
>>> + if (rte_flow_async_action_handle_destroy(port_id,
>>
>> Why 'pia->handle' check removed, was it unnecessary to check it at first place?
This seems already discussed and agreed in other thread, so proceeding.
Applied to dpdk-next-net/main, thanks.
@@ -2873,9 +2873,9 @@ port_queue_action_handle_destroy(portid_t port_id,
job->type = QUEUE_JOB_TYPE_ACTION_DESTROY;
job->pia = pia;
- if (pia->handle &&
- rte_flow_async_action_handle_destroy(port_id,
+ if (rte_flow_async_action_handle_destroy(port_id,
queue_id, &attr, pia->handle, job, &error)) {
+ free(job);
ret = port_flow_complain(&error);
continue;
}