On Mon, Jun 30, 2014 at 3:01 PM, Alex Wang <al...@nicira.com> wrote:
> Due to the race condition in userspace, there is chance that two
> overlapping megaflows could be installed in datapath. And this
> causes userspace unable to delete the less inclusive megaflow flow
> even after it timeout, since the flow_del logic will stop at the
> first match of masked flow.
>
> This commit fixes the bug by making the kernel flow_del and flow_get
> logic check all masks in that case.
>
> Signed-off-by: Alex Wang <al...@nicira.com>
> Acked-by: Andy Zhou <az...@nicira.com>
> ---
> datapath/datapath.c | 8 ++++----
> datapath/flow_table.c | 25 +++++++++++++++++++++++++
> datapath/flow_table.h | 2 ++
> 3 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index e504fee..b72419b 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1117,8 +1117,8 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct
> genl_info *info)
> goto unlock;
> }
>
> - flow = ovs_flow_tbl_lookup(&dp->table, &key);
> - if (!flow || !ovs_flow_cmp_unmasked_key(flow, &match)) {
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (!flow) {
> err = -ENOENT;
> goto unlock;
> }
> @@ -1165,8 +1165,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct
> genl_info *info)
> err = ovs_flow_tbl_flush(&dp->table);
> goto unlock;
> }
> - flow = ovs_flow_tbl_lookup(&dp->table, &key);
> - if (unlikely(!flow || !ovs_flow_cmp_unmasked_key(flow, &match))) {
> + flow = ovs_flow_tbl_lookup_exact(&dp->table, &match);
> + if (unlikely(!flow)) {
> err = -ENOENT;
> goto unlock;
> }
Can you use exact-lookup in ovsflow_cmd_set(). and do same in
ovs_flow_cmd_new() when ovs_flow_cmp_unmasked_key() fails. We also
need to change return value to ENOENT.
> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index 9746822..dfd3997 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> @@ -686,6 +686,31 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table
> *tbl,
> return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &index);
> }
>
> +struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> + struct sw_flow_match *match)
> +{
> + struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> + struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
> + struct sw_flow *flow;
> + u32 __always_unused n_mask_hit;
> + int i;
> +
> + n_mask_hit = 0;
> +
since it always unused there is no need to set it.
> + for (i = 0; i < ma->max; i++) {
All valid mask are stored in (0, count] range, so we can loop from 0
to ma->count.
> + struct sw_flow_mask *mask;
> +
> + mask = rcu_dereference_ovsl(ma->masks[i]);
it is always called under ovs lock, so we need ovsl_dereference().
> + if (mask) {
> + flow = masked_flow_lookup(ti, match->key, mask,
> &n_mask_hit);
> + if (flow && ovs_flow_cmp_unmasked_key(flow, match)) {
> /* Found */
> + return flow;
> + }
> + }
> + }
> + return NULL;
> +}
> +
> int ovs_flow_tbl_num_masks(const struct flow_table *table)
> {
> struct mask_array *ma;
> diff --git a/datapath/flow_table.h b/datapath/flow_table.h
> index ee86953..a05d36a 100644
> --- a/datapath/flow_table.h
> +++ b/datapath/flow_table.h
> @@ -89,6 +89,8 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table
> *,
> u32 *n_mask_hit);
> struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
> const struct sw_flow_key *);
> +struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *,
> + struct sw_flow_match *match);
>
> bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
> struct sw_flow_match *match);
otherwise looks good.
Acked-by: Pravin B Shelar <pshe...@nicira.com>
> --
> 1.7.9.5
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
