| Message ID | 20180817200548.64153-1-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] dpif-netlink: Prevent abort in probe_broken_meters(). | expand |
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>
> 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
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)) {
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(-)