On 12/08/2021 16:27, Ilya Maximets wrote:
> 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 <[email protected]>
>>
>> ---
>>
>> 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.
>
Ah yes, ignore my other mail.
>>
>> /* 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)
>> {
>>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
