mirror: Allow concurrent lookups.

Handler threads use a selection of mirror functions with the
assumption that the data referred to is RCU protected, while the
implementation has not provided for this, which can lead to an OVS
crash.

This patch fixes this by making the mbundle lookup RCU-safe by using
cmap instead of hmap and postponing mbundle memory free, as wells as
postponing the frees of the mirrors and the vlan bitmaps of each
mirror.

Note that mirror stats update is still not accurate if multiple
threads do it simultaneously.

A less complete version of this patch (using cmap and RCU postpone
just for the mbridge itself) was tested by Yunjian Wang and was found
to fix the observed crash when running a script that adds and deletes
a port repeatedly.

Reported-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

1 files changed, 17 insertions, 10 deletions

diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
index b00536ac7..eed63ec4a 100644
--- a/ofproto/ofproto-dpif-mirror.h
+++ b/ofproto/ofproto-dpif-mirror.h
@@ -25,19 +25,31 @@ typedef uint32_t mirror_mask_t;
 struct ofproto_dpif;
 struct ofbundle;
 
-struct mbridge *mbridge_create(void);
+/* The following functions are used by handler threads without any locking,
+ * assuming RCU protection. */
+
 struct mbridge *mbridge_ref(const struct mbridge *);
 void mbridge_unref(struct mbridge *);
 bool mbridge_has_mirrors(struct mbridge *);
-bool mbridge_need_revalidate(struct mbridge *);
-
-void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
-void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
 
 mirror_mask_t mirror_bundle_out(struct mbridge *, struct ofbundle *);
 mirror_mask_t mirror_bundle_src(struct mbridge *, struct ofbundle *);
 mirror_mask_t mirror_bundle_dst(struct mbridge *, struct ofbundle *);
 
+void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
+                         uint64_t bytes);
+bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
+                mirror_mask_t *dup_mirrors, struct ofbundle **out,
+                int *snaplen, int *out_vlan);
+
+/* The remaining functions are assumed to be called by the main thread only. */
+
+struct mbridge *mbridge_create(void);
+bool mbridge_need_revalidate(struct mbridge *);
+
+void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
+void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
+
 int mirror_set(struct mbridge *, void *aux, const char *name,
                struct ofbundle **srcs, size_t n_srcs,
                struct ofbundle **dsts, size_t n_dsts,
@@ -46,10 +58,5 @@ int mirror_set(struct mbridge *, void *aux, const char *name,
 void mirror_destroy(struct mbridge *, void *aux);
 int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
                      uint64_t *bytes);
-void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
-                         uint64_t bytes);
-bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
-                mirror_mask_t *dup_mirrors, struct ofbundle **out,
-                int *snaplen, int *out_vlan);
 
 #endif /* ofproto-dpif-mirror.h */