[ovs-dev] dpif-netlink: Prevent abort in probe_broken_meters().
diff mbox series

Message ID 20180817200548.64153-1-jpettit@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] dpif-netlink: Prevent abort in probe_broken_meters().
Related show

Commit Message

Justin Pettit Aug. 17, 2018, 8:05 p.m. UTC
Commit 92d0d515d ("dpif-netlink: Probe for broken Linux meter
implementations.") introduced a deadlock on the 'once' structure
declared in probe_broken_meters() with the following callstack:

        probe_broken_meters()
        probe_broken_meters__()
        dpif_netlink_meter_set()
        probe_broken_meters()

This commit introduce a modified version of dpif_netlink_meter_set()
that sets a meter without calling the probe.

Reported-by: Numan Siddique <nusiddiq@redhat.com>
Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 lib/dpif-netlink.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

Comments

Ben Pfaff Aug. 17, 2018, 8:08 p.m. UTC | #1
On Fri, Aug 17, 2018 at 01:05:48PM -0700, Justin Pettit wrote:
> Commit 92d0d515d ("dpif-netlink: Probe for broken Linux meter
> implementations.") introduced a deadlock on the 'once' structure
> declared in probe_broken_meters() with the following callstack:
> 
>         probe_broken_meters()
>         probe_broken_meters__()
>         dpif_netlink_meter_set()
>         probe_broken_meters()
> 
> This commit introduce a modified version of dpif_netlink_meter_set()
> that sets a meter without calling the probe.
> 
> Reported-by: Numan Siddique <nusiddiq@redhat.com>
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

I didn't test this; I assume you did.

Acked-by: Ben Pfaff <blp@ovn.org>
Justin Pettit Aug. 17, 2018, 8:15 p.m. UTC | #2
> On Aug 17, 2018, at 1:08 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> On Fri, Aug 17, 2018 at 01:05:48PM -0700, Justin Pettit wrote:
>> Commit 92d0d515d ("dpif-netlink: Probe for broken Linux meter
>> implementations.") introduced a deadlock on the 'once' structure
>> declared in probe_broken_meters() with the following callstack:
>> 
>>        probe_broken_meters()
>>        probe_broken_meters__()
>>        dpif_netlink_meter_set()
>>        probe_broken_meters()
>> 
>> This commit introduce a modified version of dpif_netlink_meter_set()
>> that sets a meter without calling the probe.
>> 
>> Reported-by: Numan Siddique <nusiddiq@redhat.com>
>> Signed-off-by: Justin Pettit <jpettit@ovn.org>
> 
> I didn't test this; I assume you did.
> 
> Acked-by: Ben Pfaff <blp@ovn.org>

Thanks for the quick review!  I pushed it to master and branch-2.10.

--Justin

Patch
diff mbox series

diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index e2810a83a656..e6d5a6ec5757 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3223,13 +3223,9 @@  dpif_netlink_meter_get_features(const struct dpif *dpif_,
 }
 
 static int
-dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id,
-                       struct ofputil_meter_config *config)
+dpif_netlink_meter_set__(struct dpif *dpif_, ofproto_meter_id meter_id,
+                         struct ofputil_meter_config *config)
 {
-    if (probe_broken_meters(dpif_)) {
-        return ENOMEM;
-    }
-
     struct dpif_netlink *dpif = dpif_netlink_cast(dpif_);
     struct ofpbuf buf, *msg;
     uint64_t stub[1024 / 8];
@@ -3303,6 +3299,17 @@  dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id,
     return 0;
 }
 
+static int
+dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id,
+                       struct ofputil_meter_config *config)
+{
+    if (probe_broken_meters(dpif_)) {
+        return ENOMEM;
+    }
+
+    return dpif_netlink_meter_set__(dpif_, meter_id, config);
+}
+
 /* Retrieve statistics and/or delete meter 'meter_id'.  Statistics are
  * stored in 'stats', if it is not null.  If 'command' is
  * OVS_METER_CMD_DEL, the meter is deleted and statistics are optionally
@@ -3416,9 +3423,10 @@  probe_broken_meters__(struct dpif *dpif)
     struct ofputil_meter_config config2 = { 2, OFPMF13_KBPS, 1, &band};
 
     /* Try adding two meters and make sure that they both come back with
-     * the proper meter id. */
-    dpif_netlink_meter_set(dpif, id1, &config1);
-    dpif_netlink_meter_set(dpif, id2, &config2);
+     * the proper meter id.  Use the "__" version so that we don't cause
+     * a recurve deadlock. */
+    dpif_netlink_meter_set__(dpif, id1, &config1);
+    dpif_netlink_meter_set__(dpif, id2, &config2);
 
     if (dpif_netlink_meter_get(dpif, id1, NULL, 0)
         || dpif_netlink_meter_get(dpif, id2, NULL, 0)) {