netdev - Re: [PATCH net-next v4 08/10] net: openvswitch: fix possible memleak on destroy flow-table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOrHB_B5dLuvoTxGpmaMiX9deEk9KjQHacqNKEpzHA2m5YS7jw@mail.gmail.com>
Date:   Thu, 17 Oct 2019 15:38:35 -0700
From:   Pravin Shelar <pshelar@....org>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>
Cc:     Greg Rose <gvrose8192@...il.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        ovs dev <dev@...nvswitch.org>
Subject: Re: [PATCH net-next v4 08/10] net: openvswitch: fix possible memleak
 on destroy flow-table

On Wed, Oct 16, 2019 at 5:50 AM <xiangxia.m.yue@...il.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@...il.com>
>
> When we destroy the flow tables which may contain the flow_mask,
> so release the flow mask struct.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
> Tested-by: Greg Rose <gvrose8192@...il.com>
> ---
>  net/openvswitch/flow_table.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
> index 5df5182..d5d768e 100644
> --- a/net/openvswitch/flow_table.c
> +++ b/net/openvswitch/flow_table.c
> @@ -295,6 +295,18 @@ static void table_instance_destroy(struct table_instance *ti,
>         }
>  }
>
> +static void tbl_mask_array_destroy(struct flow_table *tbl)
> +{
> +       struct mask_array *ma = ovsl_dereference(tbl->mask_array);
> +       int i;
> +
> +       /* Free the flow-mask and kfree_rcu the NULL is allowed. */
> +       for (i = 0; i < ma->max; i++)
> +               kfree_rcu(rcu_dereference_raw(ma->masks[i]), rcu);
> +
> +       kfree_rcu(rcu_dereference_raw(tbl->mask_array), rcu);
> +}
> +
>  /* No need for locking this function is called from RCU callback or
>   * error path.
>   */
> @@ -304,7 +316,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
>         struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
>
>         free_percpu(table->mask_cache);
> -       kfree_rcu(rcu_dereference_raw(table->mask_array), rcu);
> +       tbl_mask_array_destroy(table);
>         table_instance_destroy(ti, ufid_ti, false);
>  }

This should not be required. mask is linked to a flow and gets
released when flow is removed.
Does the memory leak occur when OVS module is abruptly unloaded and
userspace does not cleanup flow table?
In that case better fix could be calling ovs_flow_tbl_remove()
equivalent from table_instance_destroy when it is iterating flow
table.