diff mbox series

[ovs-dev,v5,22/27] netdev-offload-dpdk: Protect concurrent offload destroy/query

Message ID 19d3b47ef290a31be471b5817db6ed3f361b12c9.1631094144.git.grive@u256.net
State Accepted
Commit b3e029f7c16560bba7b8d0f651b554c5e3ed9f54
Headers show
Series dpif-netdev: Parallel offload processing | expand

Checks

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

Commit Message

Gaetan Rivet Sept. 8, 2021, 9:47 a.m. UTC 
The rte_flow API in DPDK is now thread safe for insertion and deletion.
It is not however safe for concurrent query while the offload is being
inserted or deleted.

Insertion is not an issue as the rte_flow handle will be published to
other threads only once it has been inserted in the hardware, so the
query will only be able to proceed once it is already available.

For the deletion path however, offload status queries can be made while
an offload is being destroyed. This would create race conditions and
use-after-free if not properly protected.

As a pre-step before removing the OVS-level locks on the rte_flow API,
mutually exclude offload query and deletion from concurrent execution.

Signed-off-by: Gaetan Rivet <grive@u256.net>
Reviewed-by: Eli Britstein <elibr@nvidia.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/netdev-offload-dpdk.c | 39 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 3 deletions(-)
diff mbox series

Patch

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index e76c50b72..28cb2f96b 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -61,6 +61,8 @@  struct ufid_to_rte_flow_data {
     bool actions_offloaded;
     struct dpif_flow_stats stats;
     struct netdev *physdev;
+    struct ovs_mutex lock;
+    bool dead;
 };
 
 struct netdev_offload_dpdk_data {
@@ -238,6 +240,7 @@  ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
     data->physdev = netdev != physdev ? netdev_ref(physdev) : physdev;
     data->rte_flow = rte_flow;
     data->actions_offloaded = actions_offloaded;
+    ovs_mutex_init(&data->lock);
 
     cmap_insert(map, CONST_CAST(struct cmap_node *, &data->node), hash);
 
@@ -245,8 +248,16 @@  ufid_to_rte_flow_associate(const ovs_u128 *ufid, struct netdev *netdev,
     return data;
 }
 
+static void
+rte_flow_data_unref(struct ufid_to_rte_flow_data *data)
+{
+    ovs_mutex_destroy(&data->lock);
+    free(data);
+}
+
 static inline void
 ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data *data)
+    OVS_REQUIRES(data->lock)
 {
     size_t hash = hash_bytes(&data->ufid, sizeof data->ufid, 0);
     struct cmap *map = offload_data_map(data->netdev);
@@ -263,7 +274,7 @@  ufid_to_rte_flow_disassociate(struct ufid_to_rte_flow_data *data)
         netdev_close(data->netdev);
     }
     netdev_close(data->physdev);
-    ovsrcu_postpone(free, data);
+    ovsrcu_postpone(rte_flow_data_unref, data);
 }
 
 /*
@@ -2033,6 +2044,15 @@  netdev_offload_dpdk_flow_destroy(struct ufid_to_rte_flow_data *rte_flow_data)
     ovs_u128 *ufid;
     int ret;
 
+    ovs_mutex_lock(&rte_flow_data->lock);
+
+    if (rte_flow_data->dead) {
+        ovs_mutex_unlock(&rte_flow_data->lock);
+        return 0;
+    }
+
+    rte_flow_data->dead = true;
+
     rte_flow = rte_flow_data->rte_flow;
     physdev = rte_flow_data->physdev;
     netdev = rte_flow_data->netdev;
@@ -2062,6 +2082,8 @@  netdev_offload_dpdk_flow_destroy(struct ufid_to_rte_flow_data *rte_flow_data)
                  UUID_ARGS((struct uuid *) ufid));
     }
 
+    ovs_mutex_unlock(&rte_flow_data->lock);
+
     return ret;
 }
 
@@ -2194,8 +2216,19 @@  netdev_offload_dpdk_flow_get(struct netdev *netdev,
     struct rte_flow_error error;
     int ret = 0;
 
+    attrs->dp_extra_info = NULL;
+
     rte_flow_data = ufid_to_rte_flow_data_find(netdev, ufid, false);
-    if (!rte_flow_data || !rte_flow_data->rte_flow) {
+    if (!rte_flow_data || !rte_flow_data->rte_flow ||
+        rte_flow_data->dead || ovs_mutex_trylock(&rte_flow_data->lock)) {
+        return -1;
+    }
+
+    /* Check again whether the data is dead, as it could have been
+     * updated while the lock was not yet taken. The first check above
+     * was only to avoid unnecessary locking if possible.
+     */
+    if (rte_flow_data->dead) {
         ret = -1;
         goto out;
     }
@@ -2223,7 +2256,7 @@  netdev_offload_dpdk_flow_get(struct netdev *netdev,
     }
     memcpy(stats, &rte_flow_data->stats, sizeof *stats);
 out:
-    attrs->dp_extra_info = NULL;
+    ovs_mutex_unlock(&rte_flow_data->lock);
     return ret;
 }