[ovs-dev] [ovs flow free panic v3] datapath: Fix kernel panic on ovs_flow_free

[Jesse Gross]

On Wed, Jan 15, 2014 at 1:06 PM, Andy Zhou <azhou at nicira.com> wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index b42fd8b..6ae4ecf 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1305,10 +1306,13 @@ static void __dp_destroy(struct datapath *dp)
>         list_del_rcu(&dp->list_node);
>
>         /* OVSP_LOCAL is datapath internal port. We need to make sure that
> -        * all port in datapath are destroyed first before freeing datapath.
> -        */
> +        * all ports in datapath are destroyed first before freeing datapath.
> +        */
>         ovs_dp_detach_port(ovs_vport_ovsl(dp, OVSP_LOCAL));
>
> +       /* Remove all flows while holding ovs_mutex */
> +       ovs_flow_tbl_flush(&dp->table);
> +
>         call_rcu(&dp->rcu, destroy_dp_rcu);

It might be simpler to just destroy the table at this point -
otherwise we flush now and then destroy later in the RCU callback
(even though these operations are essentially the same thing).

> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index 4232b82..3ebea78 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> -static void __table_instance_destroy(struct table_instance *ti)
> +static void table_instance_free_flows(struct table_instance *ti, bool deferred)

I believe this is only called from table_instance_destroy() - maybe we
should just fold it in? It is somewhat of a discrete block of code but
in this case I think consolidating functions helps readability since
there are so many paths.

> -skip_flows:
> +static void __table_instance_destroy(struct table_instance *ti)
> +{
>         free_buckets(ti->buckets);
>         kfree(ti);
>  }
> @@ -262,7 +262,8 @@ static void flow_tbl_destroy_rcu_cb(struct rcu_head *rcu)
>  {
>         struct table_instance *ti = container_of(rcu, struct table_instance, rcu);
>
> -       __table_instance_destroy(ti);
> +       free_buckets(ti->buckets);
> +       kfree(ti);
>  }

Should we use the helper function here?

This is definitely very intricate code. I think the basic principle
here is that the only operation that should be performed in a deferred
block is freeing memory - nothing more complex should take advantage
of an existing RCU callback, which means that we have a series of
callbacks in parallel instead of a chain. I think this principle is
now respected everywhere.