[ovs-dev] [PATCH] dpif: fix memory leak of pmd_list after usage

Ilya Maximets i.maximets at ovn.org
Thu Aug 12 15:27:23 UTC 2021


On 8/12/21 5:11 PM, Harry van Haaren wrote:
> This commit fixes a memory leak when a pmd_list is retrieved
> from the sorted_poll_thread_list() function. Inside the function,
> the pmd list is allocated, but it was not freed once no longer
> required for the command functionality. These leaks were found
> by a static analysis tool.

Hmm.  Thanks for spotting this!

Could you, please, add unit tests for these functions?  Something very
basic will work.  Even if these functions will not do anything useful,
e.g. changing priority of the only implementation, asan should spot the
leak and fail the CI job that we have.

> 
> Fixes: 3d8f47bc04 ("dpif-netdev: Add command line and function pointer for miniflow extract")
> Fixes: abb807e27d ("dpif-netdev: Add command to switch dpif implementation.")
> Fixes: 3d018c3ea7 ("dpif-netdev: Add subtable lookup prio set command.")

Please, split the change for the last one to the separate patch, so
it can be backported to earlier branches.

> 
> Signed-off-by: Harry van Haaren <harry.van.haaren at intel.com>
> 
> ---
> 
>  lib/dpif-netdev.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)

The patch name should have a 'dpif-netdev' area prefix.

> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 03f460c7d1..99779bb402 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -960,12 +960,13 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>      }
>  
>      ovs_mutex_lock(&dp_netdev_mutex);
> +
> +    struct dp_netdev_pmd_thread **pmd_list = NULL;
>      SHASH_FOR_EACH (node, &dp_netdevs) {
>          struct dp_netdev *dp = node->data;
>  
>          /* Get PMD threads list, required to get DPCLS instances. */
>          size_t n;
> -        struct dp_netdev_pmd_thread **pmd_list;
>          sorted_poll_thread_list(dp, &pmd_list, &n);

This is called in a loop, so we're leaking the array on every
iteration.  Same for other parts of the patch.

>  
>          /* take port mutex as HMAP iters over them. */
> @@ -996,6 +997,7 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          ovs_mutex_unlock(&dp->port_mutex);
>      }
>      ovs_mutex_unlock(&dp_netdev_mutex);
> +    free(pmd_list);
>  
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      ds_put_format(&reply,
> @@ -1013,10 +1015,10 @@ dpif_netdev_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  {
>      struct ds reply = DS_EMPTY_INITIALIZER;
>      struct shash_node *node;
> +    struct dp_netdev_pmd_thread **pmd_list = NULL;
>  
>      ovs_mutex_lock(&dp_netdev_mutex);
>      SHASH_FOR_EACH (node, &dp_netdevs) {
> -        struct dp_netdev_pmd_thread **pmd_list;
>          struct dp_netdev *dp = node->data;
>          size_t n;
>  
> @@ -1026,6 +1028,8 @@ dpif_netdev_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          dp_netdev_impl_get(&reply, pmd_list, n);
>      }
>      ovs_mutex_unlock(&dp_netdev_mutex);
> +    free(pmd_list);
> +
>      unixctl_command_reply(conn, ds_cstr(&reply));
>      ds_destroy(&reply);
>  }
> @@ -1058,12 +1062,12 @@ dpif_netdev_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          return;
>      }
>  
> +    struct dp_netdev_pmd_thread **pmd_list = NULL;
>      SHASH_FOR_EACH (node, &dp_netdevs) {
>          struct dp_netdev *dp = node->data;
>  
>          /* Get PMD threads list, required to get DPCLS instances. */
>          size_t n;
> -        struct dp_netdev_pmd_thread **pmd_list;
>          sorted_poll_thread_list(dp, &pmd_list, &n);
>  
>          for (size_t i = 0; i < n; i++) {
> @@ -1080,6 +1084,7 @@ dpif_netdev_impl_set(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          };
>      }
>      ovs_mutex_unlock(&dp_netdev_mutex);
> +    free(pmd_list);
>  
>      /* Reply with success to command. */
>      struct ds reply = DS_EMPTY_INITIALIZER;
> @@ -1099,8 +1104,8 @@ dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>      struct shash_node *node;
>  
>      ovs_mutex_lock(&dp_netdev_mutex);
> +    struct dp_netdev_pmd_thread **pmd_list = NULL;
>      SHASH_FOR_EACH (node, &dp_netdevs) {
> -        struct dp_netdev_pmd_thread **pmd_list;
>          struct dp_netdev *dp = node->data;
>          size_t n;
>  
> @@ -1110,6 +1115,8 @@ dpif_miniflow_extract_impl_get(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          dp_mfex_impl_get(&reply, pmd_list, n);
>      }
>      ovs_mutex_unlock(&dp_netdev_mutex);
> +    free(pmd_list);
> +
>      unixctl_command_reply(conn, ds_cstr(&reply));
>      ds_destroy(&reply);
>  }
> @@ -1243,8 +1250,8 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
>       */
>      ovs_mutex_lock(&dp_netdev_mutex);
>  
> +    struct dp_netdev_pmd_thread **pmd_list = NULL;
>      SHASH_FOR_EACH (node, &dp_netdevs) {
> -        struct dp_netdev_pmd_thread **pmd_list;
>          struct dp_netdev *dp = node->data;
>          size_t n;
>  
> @@ -1269,6 +1276,7 @@ dpif_miniflow_extract_impl_set(struct unixctl_conn *conn, int argc,
>      }
>  
>      ovs_mutex_unlock(&dp_netdev_mutex);
> +    free(pmd_list);
>  
>      /* If PMD thread was specified, but it wasn't found, return error. */
>      if (pmd_thread_to_change != NON_PMD_CORE_ID && !pmd_thread_update_done) {
> 



More information about the dev mailing list