[ovs-dev] [branch-2.1 branch-2.0 PATCH] datapath: Fix a bug.

[Alex Wang]

Thx Pravin, I'll address your comments.

Also, will send the patch for branch-2.0 -> master


On Mon, Jun 30, 2014 at 12:00 PM, Pravin Shelar <pshelar at nicira.com> wrote:

> Good catch.
>
>
> On Mon, Jun 30, 2014 at 10:59 AM, Alex Wang <alexw at 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 logic check
> > all masks in that case.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
> Can you also give specific subject for this patch?
>
> > ---
> >  datapath/datapath.c   |    4 ++--
> >  datapath/flow_table.c |   16 ++++++++++++++++
> >  datapath/flow_table.h |    3 +++
> >  3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/datapath/datapath.c b/datapath/datapath.c
> > index c15ded9..cb66ff3 100644
> > --- a/datapath/datapath.c
> > +++ b/datapath/datapath.c
> > @@ -974,8 +974,8 @@ static int ovs_flow_cmd_del(struct sk_buff *skb,
> struct genl_info *info)
> >         if (err)
> >                 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, &key, &match);
>
> match has ref to key, so you can just pass match as argument to this
> function.
>
> > +       if (!flow) {
> >                 err = -ENOENT;
> >                 goto unlock;
> >         }
>
> > diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> > index fa7ad36..a0d1ea7 100644
> > --- a/datapath/flow_table.c
> > +++ b/datapath/flow_table.c
> > @@ -482,6 +482,22 @@ struct sw_flow *ovs_flow_tbl_lookup(struct
> flow_table *tbl,
> >         return ovs_flow_tbl_lookup_stats(tbl, key, &n_mask_hit);
> >  }
> >
> > +struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> > +                                         const struct sw_flow_key *key,
> > +                                         struct sw_flow_match *match)
> > +{
> > +       struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> > +       struct sw_flow_mask *mask;
> > +       struct sw_flow *flow;
> > +
> > +       list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
> > +               flow = masked_flow_lookup(ti, key, mask);
> > +               if (flow && ovs_flow_cmp_unmasked_key(flow, match))  /*
> Found */
> > +                       return flow;
> > +       }
> This function is always called under ovs-mutex, so no need to use rcu
> iterator.
>
> > +       return NULL;
> > +}
> > +
> >  int ovs_flow_tbl_num_masks(const struct flow_table *table)
> >  {
> >         struct sw_flow_mask *mask;
> > diff --git a/datapath/flow_table.h b/datapath/flow_table.h
> > index baaeb10..3697427 100644
> > --- a/datapath/flow_table.h
> > +++ b/datapath/flow_table.h
> > @@ -72,6 +72,9 @@ struct sw_flow *ovs_flow_tbl_dump_next(struct
> table_instance *table,
> >  struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *,
> >                                     const struct sw_flow_key *,
> >                                     u32 *n_mask_hit);
> > +struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
> > +                                         const struct sw_flow_key *key,
> > +                                         struct sw_flow_match *match);
>
> We should use same exact-lookup for flow_get and when we try to modify
> flow.
>
> >  struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
> >                                     const struct sw_flow_key *);
> >
> > --
> > 1.7.9.5
> >
>