diff mbox series

[ovs-dev] ofproto-dpif-upcall: Fix race condition while purging

Message ID 20210511083821.60724-1-roid@nvidia.com
State Changes Requested
Headers show
Series [ovs-dev] ofproto-dpif-upcall: Fix race condition while purging | expand

Commit Message

Roi Dayan May 11, 2021, 8:38 a.m. UTC 
From: Jianbo Liu <jianbol@nvidia.com>

There is a race condidtion between purger and handler. Handler may
create new ukey and install it while executing 'ovs-appctl
revalidator/purge' command. However, before handler calls
transition_ukey() in handle_upcalls(), purger may get this ukey from
umap, then evict and delete it. This will trigger ovs_abort in
transition_ukey() for handler because it is trying to set state to
EVICTED or OPERATIONAL, but ukey is already in DELETED state.
To fix this issue, purger must not delete ukey in VISIBLE state.

Fixes: 98bb4286970d ("tests: Add command to purge revalidators of flows.")
Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
Reviewed-by: Roi Dayan <roid@nvidia.com>
---
 ofproto/ofproto-dpif-upcall.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

Jianbo Liu May 21, 2021, 1:01 a.m. UTC | #1 
The 05/11/2021 11:38, Roi Dayan wrote:
> From: Jianbo Liu <jianbol@nvidia.com>
> 
> There is a race condidtion between purger and handler. Handler may
> create new ukey and install it while executing 'ovs-appctl
> revalidator/purge' command. However, before handler calls
> transition_ukey() in handle_upcalls(), purger may get this ukey from
> umap, then evict and delete it. This will trigger ovs_abort in
> transition_ukey() for handler because it is trying to set state to
> EVICTED or OPERATIONAL, but ukey is already in DELETED state.
> To fix this issue, purger must not delete ukey in VISIBLE state.

Friendly ping for review ...

Thanks!
Jianbo

> 
> Fixes: 98bb4286970d ("tests: Add command to purge revalidators of flows.")
> Signed-off-by: Jianbo Liu <jianbol@nvidia.com>
> Reviewed-by: Roi Dayan <roid@nvidia.com>
> ---
>  ofproto/ofproto-dpif-upcall.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index ccf97266c0b9..3add505ff652 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -327,6 +327,12 @@ struct ukey_op {
>      struct dpif_op dop;           /* Flow operation. */
>  };
>  
> +enum sweep_type {
> +    PURGE_NONE,
> +    PURGE_SOFT,
> +    PURGE_HARD,
> +};
> +
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>  static struct ovs_list all_udpifs = OVS_LIST_INITIALIZER(&all_udpifs);
>  
> @@ -345,7 +351,7 @@ static unsigned long udpif_get_n_flows(struct udpif *);
>  static void revalidate(struct revalidator *);
>  static void revalidator_pause(struct revalidator *);
>  static void revalidator_sweep(struct revalidator *);
> -static void revalidator_purge(struct revalidator *);
> +static void revalidator_purge(struct revalidator *, enum sweep_type);
>  static void upcall_unixctl_show(struct unixctl_conn *conn, int argc,
>                                  const char *argv[], void *aux);
>  static void upcall_unixctl_disable_megaflows(struct unixctl_conn *, int argc,
> @@ -541,7 +547,7 @@ udpif_stop_threads(struct udpif *udpif, bool delete_flows)
>  
>          if (delete_flows) {
>              for (i = 0; i < udpif->n_revalidators; i++) {
> -                revalidator_purge(&udpif->revalidators[i]);
> +                revalidator_purge(&udpif->revalidators[i], PURGE_HARD);
>              }
>          }
>  
> @@ -2772,7 +2778,8 @@ revalidator_pause(struct revalidator *revalidator)
>  }
>  
>  static void
> -revalidator_sweep__(struct revalidator *revalidator, bool purge)
> +revalidator_sweep__(struct revalidator *revalidator,
> +                    enum sweep_type sweep_type)
>  {
>      struct udpif *udpif;
>      uint64_t dump_seq, reval_seq;
> @@ -2803,13 +2810,13 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>              }
>              ukey_state = ukey->state;
>              if (ukey_state == UKEY_OPERATIONAL
> -                || (ukey_state == UKEY_VISIBLE && purge)) {
> +                || (ukey_state == UKEY_VISIBLE && sweep_type == PURGE_HARD)) {
>                  struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
>                  bool seq_mismatch = (ukey->dump_seq != dump_seq
>                                       && ukey->reval_seq != reval_seq);
>                  enum reval_result result;
>  
> -                if (purge) {
> +                if (sweep_type > PURGE_NONE) {
>                      result = UKEY_DELETE;
>                  } else if (!seq_mismatch) {
>                      result = UKEY_KEEP;
> @@ -2856,13 +2863,13 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>  static void
>  revalidator_sweep(struct revalidator *revalidator)
>  {
> -    revalidator_sweep__(revalidator, false);
> +    revalidator_sweep__(revalidator, PURGE_NONE);
>  }
>  
>  static void
> -revalidator_purge(struct revalidator *revalidator)
> +revalidator_purge(struct revalidator *revalidator, enum sweep_type sweep_type)
>  {
> -    revalidator_sweep__(revalidator, true);
> +    revalidator_sweep__(revalidator, sweep_type);
>  }
>  
>  /* In reaction to dpif purge, purges all 'ukey's with same 'pmd_id'. */
> @@ -3056,7 +3063,7 @@ upcall_unixctl_purge(struct unixctl_conn *conn, int argc OVS_UNUSED,
>          int n;
>  
>          for (n = 0; n < udpif->n_revalidators; n++) {
> -            revalidator_purge(&udpif->revalidators[n]);
> +            revalidator_purge(&udpif->revalidators[n], PURGE_SOFT);
>          }
>      }
>      unixctl_command_reply(conn, "");
> -- 
> 2.8.0
>
Simon Horman May 26, 2021, 12:59 p.m. UTC | #2 
On Fri, May 21, 2021 at 01:01:29AM +0000, Jianbo Liu wrote:
> The 05/11/2021 11:38, Roi Dayan wrote:
> > From: Jianbo Liu <jianbol@nvidia.com>
> > 
> > There is a race condidtion between purger and handler. Handler may
> > create new ukey and install it while executing 'ovs-appctl
> > revalidator/purge' command. However, before handler calls
> > transition_ukey() in handle_upcalls(), purger may get this ukey from
> > umap, then evict and delete it. This will trigger ovs_abort in
> > transition_ukey() for handler because it is trying to set state to
> > EVICTED or OPERATIONAL, but ukey is already in DELETED state.
> > To fix this issue, purger must not delete ukey in VISIBLE state.
> 
> Friendly ping for review ...

Thanks.

This looks ok to me, but it seems to make the testsuite unhappy.

https://github.com/horms2/ovs/actions/runs/878488417
https://travis-ci.org/github/horms2/ovs/builds/772454219
Jianbo Liu May 27, 2021, 2:13 a.m. UTC | #3 
The 05/26/2021 14:59, Simon Horman wrote:
> On Fri, May 21, 2021 at 01:01:29AM +0000, Jianbo Liu wrote:
> > The 05/11/2021 11:38, Roi Dayan wrote:
> > > From: Jianbo Liu <jianbol@nvidia.com>
> > > 
> > > There is a race condidtion between purger and handler. Handler may
> > > create new ukey and install it while executing 'ovs-appctl
> > > revalidator/purge' command. However, before handler calls
> > > transition_ukey() in handle_upcalls(), purger may get this ukey from
> > > umap, then evict and delete it. This will trigger ovs_abort in
> > > transition_ukey() for handler because it is trying to set state to
> > > EVICTED or OPERATIONAL, but ukey is already in DELETED state.
> > > To fix this issue, purger must not delete ukey in VISIBLE state.
> > 
> > Friendly ping for review ...
> 
> Thanks.
> 
> This looks ok to me, but it seems to make the testsuite unhappy.

Thanks. It looks dpif_netdev behaves differently from dpif_netlink.
I will fix it.
> 
> https://github.com/horms2/ovs/actions/runs/878488417
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fgithub%2Fhorms2%2Fovs%2Fbuilds%2F772454219&amp;data=04%7C01%7Cjianbol%40nvidia.com%7Cd5f41ad0947645f50ff708d920462a52%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637576308895924312%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp;sdata=drU7kbpU4RyMbveYqv2p3xQ39tE8h52t27hteFn%2FBIM%3D&amp;reserved=0
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index ccf97266c0b9..3add505ff652 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -327,6 +327,12 @@  struct ukey_op {
     struct dpif_op dop;           /* Flow operation. */
 };
 
+enum sweep_type {
+    PURGE_NONE,
+    PURGE_SOFT,
+    PURGE_HARD,
+};
+
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 static struct ovs_list all_udpifs = OVS_LIST_INITIALIZER(&all_udpifs);
 
@@ -345,7 +351,7 @@  static unsigned long udpif_get_n_flows(struct udpif *);
 static void revalidate(struct revalidator *);
 static void revalidator_pause(struct revalidator *);
 static void revalidator_sweep(struct revalidator *);
-static void revalidator_purge(struct revalidator *);
+static void revalidator_purge(struct revalidator *, enum sweep_type);
 static void upcall_unixctl_show(struct unixctl_conn *conn, int argc,
                                 const char *argv[], void *aux);
 static void upcall_unixctl_disable_megaflows(struct unixctl_conn *, int argc,
@@ -541,7 +547,7 @@  udpif_stop_threads(struct udpif *udpif, bool delete_flows)
 
         if (delete_flows) {
             for (i = 0; i < udpif->n_revalidators; i++) {
-                revalidator_purge(&udpif->revalidators[i]);
+                revalidator_purge(&udpif->revalidators[i], PURGE_HARD);
             }
         }
 
@@ -2772,7 +2778,8 @@  revalidator_pause(struct revalidator *revalidator)
 }
 
 static void
-revalidator_sweep__(struct revalidator *revalidator, bool purge)
+revalidator_sweep__(struct revalidator *revalidator,
+                    enum sweep_type sweep_type)
 {
     struct udpif *udpif;
     uint64_t dump_seq, reval_seq;
@@ -2803,13 +2810,13 @@  revalidator_sweep__(struct revalidator *revalidator, bool purge)
             }
             ukey_state = ukey->state;
             if (ukey_state == UKEY_OPERATIONAL
-                || (ukey_state == UKEY_VISIBLE && purge)) {
+                || (ukey_state == UKEY_VISIBLE && sweep_type == PURGE_HARD)) {
                 struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER;
                 bool seq_mismatch = (ukey->dump_seq != dump_seq
                                      && ukey->reval_seq != reval_seq);
                 enum reval_result result;
 
-                if (purge) {
+                if (sweep_type > PURGE_NONE) {
                     result = UKEY_DELETE;
                 } else if (!seq_mismatch) {
                     result = UKEY_KEEP;
@@ -2856,13 +2863,13 @@  revalidator_sweep__(struct revalidator *revalidator, bool purge)
 static void
 revalidator_sweep(struct revalidator *revalidator)
 {
-    revalidator_sweep__(revalidator, false);
+    revalidator_sweep__(revalidator, PURGE_NONE);
 }
 
 static void
-revalidator_purge(struct revalidator *revalidator)
+revalidator_purge(struct revalidator *revalidator, enum sweep_type sweep_type)
 {
-    revalidator_sweep__(revalidator, true);
+    revalidator_sweep__(revalidator, sweep_type);
 }
 
 /* In reaction to dpif purge, purges all 'ukey's with same 'pmd_id'. */
@@ -3056,7 +3063,7 @@  upcall_unixctl_purge(struct unixctl_conn *conn, int argc OVS_UNUSED,
         int n;
 
         for (n = 0; n < udpif->n_revalidators; n++) {
-            revalidator_purge(&udpif->revalidators[n]);
+            revalidator_purge(&udpif->revalidators[n], PURGE_SOFT);
         }
     }
     unixctl_command_reply(conn, "");