The subtable search function can be used at any time by a PMD thread.
Setting the subtable search function should be done atomically to
prevent garbage data from being read.
A dpcls_subtable_lookup_reprobe() call can happen at the same time that
DPCLS subtables are being sorted. This could lead to modifications done
by the reprobe() to be lost. Prevent this from happening by locking on
pmd->flow_mutex. After this change both the reprobe function and a
subtable sort will share the flow_mutex preventing modifications by
either one from being lost.
Also remove the pvector_publish() call. The pvector is not being changed
in dpcls_subtable_lookup_reprobe(), only the data pointed to by pointers
in the vector are being changed.
Fixes: 3d018c3ea79d ("dpif-netdev: add subtable lookup prio set command.")
Reported-by: Ilya Maximets <[email protected]>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-January/390757.html
Signed-off-by: Cian Ferriter <[email protected]>
---
lib/dpif-netdev-private-dpcls.h | 6 ++++--
lib/dpif-netdev.c | 20 ++++++++++++++++----
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/lib/dpif-netdev-private-dpcls.h b/lib/dpif-netdev-private-dpcls.h
index 7c4a840cb..0d5da73c7 100644
--- a/lib/dpif-netdev-private-dpcls.h
+++ b/lib/dpif-netdev-private-dpcls.h
@@ -83,8 +83,10 @@ struct dpcls_subtable {
/* The lookup function to use for this subtable. If there is a known
* property of the subtable (eg: only 3 bits of miniflow metadata is
* used for the lookup) then this can point at an optimized version of
- * the lookup function for this particular subtable. */
- dpcls_subtable_lookup_func lookup_func;
+ * the lookup function for this particular subtable. The lookup function
+ * can be used at any time by a PMD thread, so it's declared as an atomic
+ * here to prevent garbage from being read. */
+ ATOMIC(dpcls_subtable_lookup_func) lookup_func;
/* Caches the masks to match a packet to, reducing runtime calculations. */
uint64_t *mf_masks;
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index e28e0b554..71f608a7c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1074,7 +1074,9 @@ dpif_netdev_subtable_lookup_set(struct unixctl_conn
*conn, int argc OVS_UNUSED,
if (!cls) {
continue;
}
+ ovs_mutex_lock(&pmd->flow_mutex);
uint32_t subtbl_changes = dpcls_subtable_lookup_reprobe(cls);
+ ovs_mutex_unlock(&pmd->flow_mutex);
if (subtbl_changes) {
lookup_dpcls_changed++;
lookup_subtable_changed += subtbl_changes;
@@ -9736,9 +9738,12 @@ dpcls_create_subtable(struct dpcls *cls, const struct
netdev_flow_key *mask)
/* Get the preferred subtable search function for this (u0,u1) subtable.
* The function is guaranteed to always return a valid implementation, and
- * possibly an ISA optimized, and/or specialized implementation.
+ * possibly an ISA optimized, and/or specialized implementation. Initialize
+ * the subtable search function atomically to avoid garbage data being read
+ * by the PMD thread.
*/
- subtable->lookup_func = dpcls_subtable_get_best_impl(unit0, unit1);
+ atomic_init(&subtable->lookup_func,
+ dpcls_subtable_get_best_impl(unit0, unit1));
cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
/* Add the new subtable at the end of the pvector (with no hits yet) */
@@ -9767,6 +9772,10 @@ dpcls_find_subtable(struct dpcls *cls, const struct
netdev_flow_key *mask)
/* Checks for the best available implementation for each subtable lookup
* function, and assigns it as the lookup function pointer for each subtable.
* Returns the number of subtables that have changed lookup implementation.
+ * This function requires holding a flow_mutex when called. This is to make
+ * sure modifications done by this function are not overwritten. This could
+ * happen if dpcls_sort_subtable_vector() is called at the same time as this
+ * function.
*/
static uint32_t
dpcls_subtable_lookup_reprobe(struct dpcls *cls)
@@ -9779,10 +9788,13 @@ dpcls_subtable_lookup_reprobe(struct dpcls *cls)
uint32_t u0_bits = subtable->mf_bits_set_unit0;
uint32_t u1_bits = subtable->mf_bits_set_unit1;
void *old_func = subtable->lookup_func;
- subtable->lookup_func = dpcls_subtable_get_best_impl(u0_bits, u1_bits);
+
+ /* Set the subtable lookup function atomically to avoid garbage data
+ * being read by the PMD thread. */
+ atomic_store_relaxed(&subtable->lookup_func,
+ dpcls_subtable_get_best_impl(u0_bits, u1_bits));
subtables_changed += (old_func != subtable->lookup_func);
}
- pvector_publish(pvec);
return subtables_changed;
}
--
2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
