diff mbox series

[ovs-dev,v2,2/3] northd.c: Lock to protect against possible od->group corruption.

Message ID 20211003224514.581059-2-hzhou@ovn.org
State Accepted
Headers show
Series [ovs-dev,v2,1/3] northd.c: Remove redundant condition check in ovn_dp_group_add_with_reference(). | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes fail github build: failed

Commit Message

Han Zhou Oct. 3, 2021, 10:45 p.m. UTC 
When parallel build is used, od->group can be updated by threads outside
of the function do_ovn_lflow_add_pd (for lb related flow building). So
use the function ovn_dp_group_add_with_reference() to update it in
function do_ovn_lflow_add() when it is not a newly created flow.

Signed-off-by: Han Zhou <hzhou@ovn.org>
---
 northd/northd.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

Comments

Anton Ivanov Oct. 4, 2021, 9:29 a.m. UTC | #1 
On 03/10/2021 23:45, Han Zhou wrote:
> When parallel build is used, od->group can be updated by threads outside
> of the function do_ovn_lflow_add_pd (for lb related flow building). So
> use the function ovn_dp_group_add_with_reference() to update it in
> function do_ovn_lflow_add() when it is not a newly created flow.
>
> Signed-off-by: Han Zhou <hzhou@ovn.org>
> ---
>   northd/northd.c | 42 +++++++++++++++++++++---------------------
>   1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 027c5b170..afd812700 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -4299,6 +4299,26 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
>       }
>   }
>   
> +static bool
> +ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
> +                                struct ovn_datapath *od)
> +                                OVS_NO_THREAD_SAFETY_ANALYSIS
> +{
> +    if (!use_logical_dp_groups || !lflow_ref) {
> +        return false;
> +    }
> +
> +    if (use_parallel_build) {
> +        ovs_mutex_lock(&lflow_ref->odg_lock);
> +        hmapx_add(&lflow_ref->od_group, od);
> +        ovs_mutex_unlock(&lflow_ref->odg_lock);
> +    } else {
> +        hmapx_add(&lflow_ref->od_group, od);
> +    }
> +
> +    return true;
> +}
> +
>   /* Adds a row with the specified contents to the Logical_Flow table.
>    * Version to use with dp_groups + parallel - when locking is required.
>    *
> @@ -4351,7 +4371,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
>           old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
>                                      actions, ctrl_meter, hash);
>           if (old_lflow) {
> -            hmapx_add(&old_lflow->od_group, od);
> +            ovn_dp_group_add_with_reference(old_lflow, od);
>               return old_lflow;
>           }
>       }
> @@ -4466,26 +4486,6 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
>                                  io_port, ctrl_meter, stage_hint, where, hash);
>   }
>   
> -static bool
> -ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
> -                                struct ovn_datapath *od)
> -                                OVS_NO_THREAD_SAFETY_ANALYSIS
> -{
> -    if (!use_logical_dp_groups || !lflow_ref) {
> -        return false;
> -    }
> -
> -    if (use_parallel_build) {
> -        ovs_mutex_lock(&lflow_ref->odg_lock);
> -        hmapx_add(&lflow_ref->od_group, od);
> -        ovs_mutex_unlock(&lflow_ref->odg_lock);
> -    } else {
> -        hmapx_add(&lflow_ref->od_group, od);
> -    }
> -
> -    return true;
> -}
> -
>   /* Adds a row with the specified contents to the Logical_Flow table. */
>   #define ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \
>                                     ACTIONS, IN_OUT_PORT, CTRL_METER, \


+1
Numan Siddique Oct. 5, 2021, 3:08 a.m. UTC | #2 
On Mon, Oct 4, 2021 at 5:29 AM Anton Ivanov
<anton.ivanov@cambridgegreys.com> wrote:
>
>
> On 03/10/2021 23:45, Han Zhou wrote:
> > When parallel build is used, od->group can be updated by threads outside
> > of the function do_ovn_lflow_add_pd (for lb related flow building). So
> > use the function ovn_dp_group_add_with_reference() to update it in
> > function do_ovn_lflow_add() when it is not a newly created flow.
> >
> > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > ---
> >   northd/northd.c | 42 +++++++++++++++++++++---------------------
> >   1 file changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 027c5b170..afd812700 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -4299,6 +4299,26 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
> >       }
> >   }
> >
> > +static bool
> > +ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
> > +                                struct ovn_datapath *od)
> > +                                OVS_NO_THREAD_SAFETY_ANALYSIS
> > +{
> > +    if (!use_logical_dp_groups || !lflow_ref) {
> > +        return false;
> > +    }
> > +
> > +    if (use_parallel_build) {
> > +        ovs_mutex_lock(&lflow_ref->odg_lock);
> > +        hmapx_add(&lflow_ref->od_group, od);
> > +        ovs_mutex_unlock(&lflow_ref->odg_lock);
> > +    } else {
> > +        hmapx_add(&lflow_ref->od_group, od);
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >   /* Adds a row with the specified contents to the Logical_Flow table.
> >    * Version to use with dp_groups + parallel - when locking is required.
> >    *
> > @@ -4351,7 +4371,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
> >           old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> >                                      actions, ctrl_meter, hash);
> >           if (old_lflow) {
> > -            hmapx_add(&old_lflow->od_group, od);
> > +            ovn_dp_group_add_with_reference(old_lflow, od);
> >               return old_lflow;
> >           }
> >       }
> > @@ -4466,26 +4486,6 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
> >                                  io_port, ctrl_meter, stage_hint, where, hash);
> >   }
> >
> > -static bool
> > -ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
> > -                                struct ovn_datapath *od)
> > -                                OVS_NO_THREAD_SAFETY_ANALYSIS
> > -{
> > -    if (!use_logical_dp_groups || !lflow_ref) {
> > -        return false;
> > -    }
> > -
> > -    if (use_parallel_build) {
> > -        ovs_mutex_lock(&lflow_ref->odg_lock);
> > -        hmapx_add(&lflow_ref->od_group, od);
> > -        ovs_mutex_unlock(&lflow_ref->odg_lock);
> > -    } else {
> > -        hmapx_add(&lflow_ref->od_group, od);
> > -    }
> > -
> > -    return true;
> > -}
> > -
> >   /* Adds a row with the specified contents to the Logical_Flow table. */
> >   #define ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \
> >                                     ACTIONS, IN_OUT_PORT, CTRL_METER, \
>
>
> +1
>

Acked-by: Numan Siddique <numans@ovn.org>

Numan

> --
> Anton R. Ivanov
> Cambridgegreys Limited. Registered in England. Company Number 10273661
> https://www.cambridgegreys.com/
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Han Zhou Oct. 5, 2021, 7:36 a.m. UTC | #3 
On Mon, Oct 4, 2021 at 8:08 PM Numan Siddique <numans@ovn.org> wrote:
>
> On Mon, Oct 4, 2021 at 5:29 AM Anton Ivanov
> <anton.ivanov@cambridgegreys.com> wrote:
> >
> >
> > On 03/10/2021 23:45, Han Zhou wrote:
> > > When parallel build is used, od->group can be updated by threads
outside
> > > of the function do_ovn_lflow_add_pd (for lb related flow building). So
> > > use the function ovn_dp_group_add_with_reference() to update it in
> > > function do_ovn_lflow_add() when it is not a newly created flow.
> > >
> > > Signed-off-by: Han Zhou <hzhou@ovn.org>
> > > ---
> > >   northd/northd.c | 42 +++++++++++++++++++++---------------------
> > >   1 file changed, 21 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index 027c5b170..afd812700 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -4299,6 +4299,26 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
ovn_datapath *od,
> > >       }
> > >   }
> > >
> > > +static bool
> > > +ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
> > > +                                struct ovn_datapath *od)
> > > +                                OVS_NO_THREAD_SAFETY_ANALYSIS
> > > +{
> > > +    if (!use_logical_dp_groups || !lflow_ref) {
> > > +        return false;
> > > +    }
> > > +
> > > +    if (use_parallel_build) {
> > > +        ovs_mutex_lock(&lflow_ref->odg_lock);
> > > +        hmapx_add(&lflow_ref->od_group, od);
> > > +        ovs_mutex_unlock(&lflow_ref->odg_lock);
> > > +    } else {
> > > +        hmapx_add(&lflow_ref->od_group, od);
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > +
> > >   /* Adds a row with the specified contents to the Logical_Flow table.
> > >    * Version to use with dp_groups + parallel - when locking is
required.
> > >    *
> > > @@ -4351,7 +4371,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct
ovn_datapath *od,
> > >           old_lflow = ovn_lflow_find(lflow_map, NULL, stage,
priority, match,
> > >                                      actions, ctrl_meter, hash);
> > >           if (old_lflow) {
> > > -            hmapx_add(&old_lflow->od_group, od);
> > > +            ovn_dp_group_add_with_reference(old_lflow, od);
> > >               return old_lflow;
> > >           }
> > >       }
> > > @@ -4466,26 +4486,6 @@ ovn_lflow_add_at(struct hmap *lflow_map,
struct ovn_datapath *od,
> > >                                  io_port, ctrl_meter, stage_hint,
where, hash);
> > >   }
> > >
> > > -static bool
> > > -ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
> > > -                                struct ovn_datapath *od)
> > > -                                OVS_NO_THREAD_SAFETY_ANALYSIS
> > > -{
> > > -    if (!use_logical_dp_groups || !lflow_ref) {
> > > -        return false;
> > > -    }
> > > -
> > > -    if (use_parallel_build) {
> > > -        ovs_mutex_lock(&lflow_ref->odg_lock);
> > > -        hmapx_add(&lflow_ref->od_group, od);
> > > -        ovs_mutex_unlock(&lflow_ref->odg_lock);
> > > -    } else {
> > > -        hmapx_add(&lflow_ref->od_group, od);
> > > -    }
> > > -
> > > -    return true;
> > > -}
> > > -
> > >   /* Adds a row with the specified contents to the Logical_Flow
table. */
> > >   #define ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY,
MATCH, \
> > >                                     ACTIONS, IN_OUT_PORT, CTRL_METER,
\
> >
> >
> > +1
> >
>
> Acked-by: Numan Siddique <numans@ovn.org>

Thanks Anton and Numan for the review. I applied the patch 1 and 2 in this
series to main branch and branch-21.09.

>
> Numan
>
> > --
> > Anton R. Ivanov
> > Cambridgegreys Limited. Registered in England. Company Number 10273661
> > https://www.cambridgegreys.com/
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
diff mbox series

Patch

diff --git a/northd/northd.c b/northd/northd.c
index 027c5b170..afd812700 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4299,6 +4299,26 @@  ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
     }
 }
 
+static bool
+ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
+                                struct ovn_datapath *od)
+                                OVS_NO_THREAD_SAFETY_ANALYSIS
+{
+    if (!use_logical_dp_groups || !lflow_ref) {
+        return false;
+    }
+
+    if (use_parallel_build) {
+        ovs_mutex_lock(&lflow_ref->odg_lock);
+        hmapx_add(&lflow_ref->od_group, od);
+        ovs_mutex_unlock(&lflow_ref->odg_lock);
+    } else {
+        hmapx_add(&lflow_ref->od_group, od);
+    }
+
+    return true;
+}
+
 /* Adds a row with the specified contents to the Logical_Flow table.
  * Version to use with dp_groups + parallel - when locking is required.
  *
@@ -4351,7 +4371,7 @@  do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,
         old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
                                    actions, ctrl_meter, hash);
         if (old_lflow) {
-            hmapx_add(&old_lflow->od_group, od);
+            ovn_dp_group_add_with_reference(old_lflow, od);
             return old_lflow;
         }
     }
@@ -4466,26 +4486,6 @@  ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
                                io_port, ctrl_meter, stage_hint, where, hash);
 }
 
-static bool
-ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,
-                                struct ovn_datapath *od)
-                                OVS_NO_THREAD_SAFETY_ANALYSIS
-{
-    if (!use_logical_dp_groups || !lflow_ref) {
-        return false;
-    }
-
-    if (use_parallel_build) {
-        ovs_mutex_lock(&lflow_ref->odg_lock);
-        hmapx_add(&lflow_ref->od_group, od);
-        ovs_mutex_unlock(&lflow_ref->odg_lock);
-    } else {
-        hmapx_add(&lflow_ref->od_group, od);
-    }
-
-    return true;
-}
-
 /* Adds a row with the specified contents to the Logical_Flow table. */
 #define ovn_lflow_add_with_hint__(LFLOW_MAP, OD, STAGE, PRIORITY, MATCH, \
                                   ACTIONS, IN_OUT_PORT, CTRL_METER, \