Fix race condition in action profile implementation

While implementing support for compile-time default entries for indirect
tables, I realized that the locking for action profiles was completely
wrong. I think that it has been broken since action profile sharing
support was introduced, so it must have been broken for a year...

In particular, the action profile mutex wasn't held during the action
entry execution, which means that modifying the action profile while
applying an action from the action profile on a packet could result in a
race condition and the corruption of the action data used by the action.

I found & implemented a temporary solution by adding virtual functions
to MatchTableAbstract to acquire the action profile mutex (for
MatchTable, the virtual functions return an empty lock). I'm confident I
can find a more elegant solution if I have time...

include/bm/bm_sim/action_profile.h

@@ -37,6 +37,9 @@
 namespace bm {
 
 class ActionProfile : public NamedP4Object {
+  friend class MatchTableIndirect;
+  friend class MatchTableIndirectWS;
+
  public:
   using mbr_hdl_t = uint32_t;
   using grp_hdl_t = uint32_t;
@@ -111,8 +114,6 @@ class ActionProfile : public NamedP4Object {
 
   ActionProfile(const std::string &name, p4object_id_t id, bool with_selection);
 
-  ActionEntry &lookup(const Packet &pkt, const IndirectIndex &index);
-
   bool has_selection() const;
 
   void set_hash(std::unique_ptr<Calculation> h) {
@@ -152,41 +153,16 @@ class ActionProfile : public NamedP4Object {
 
   std::vector<Group> get_groups() const;
 
-  size_t get_num_members() const {
-    return num_members;
-  }
-
-  size_t get_num_groups() const {
-    return num_groups;
-  }
+  size_t get_num_members() const;
+  size_t get_num_groups() const;
 
   MatchErrorCode get_num_members_in_group(grp_hdl_t grp, size_t *nb) const;
 
-  bool is_valid_mbr(mbr_hdl_t mbr) const {
-    return mbr_handles.valid_handle(mbr);
-  }
-
-  bool is_valid_grp(grp_hdl_t grp) const {
-    return grp_handles.valid_handle(grp);
-  }
-
-  bool group_is_empty(grp_hdl_t grp) const;
-
   void reset_state();
 
   void serialize(std::ostream *out) const;
   void deserialize(std::istream *in, const P4Objects &objs);
 
-  // this method is called by MatchTableIndirect and assumes that the provided
-  // index is correct
-  // TODO(antonin): make this private and add MatchTableIndirect as a friend? or
-  // check the provided index and return a MatchErrorCode value?
-  void dump_entry(std::ostream *out, const IndirectIndex &index) const;
-
-  void ref_count_increase(const IndirectIndex &index);
-
-  void ref_count_decrease(const IndirectIndex &index);
-
  private:
   using ReadLock = boost::shared_lock<boost::shared_mutex>;
   using WriteLock = boost::unique_lock<boost::shared_mutex>;
@@ -290,9 +266,31 @@ class ActionProfile : public NamedP4Object {
 
   void entries_insert(mbr_hdl_t mbr, ActionEntry &&entry);
 
+  // lock can be acquired by MatchTableIndirect
   ReadLock lock_read() const { return ReadLock(t_mutex); }
   WriteLock lock_write() const { return WriteLock(t_mutex); }
 
+  // this method is called by MatchTableIndirect and assumes that the provided
+  // index is correct
+  void dump_entry(std::ostream *out, const IndirectIndex &index) const;
+
+  // called by MatchTableIndirect for reference counting (a member cannot be
+  // deleted if match entries are pointing to it)
+  void ref_count_increase(const IndirectIndex &index);
+  void ref_count_decrease(const IndirectIndex &index);
+
+  bool is_valid_mbr(mbr_hdl_t mbr) const {
+    return mbr_handles.valid_handle(mbr);
+  }
+
+  bool is_valid_grp(grp_hdl_t grp) const {
+    return grp_handles.valid_handle(grp);
+  }
+
+  bool group_is_empty(grp_hdl_t grp) const;
+
+  ActionEntry &lookup(const Packet &pkt, const IndirectIndex &index);
+
  private:
   mutable boost::shared_mutex t_mutex{};
   bool with_selection;

include/bm/bm_sim/match_tables.h

@@ -123,12 +123,16 @@ class MatchTableAbstract : public NamedP4Object {
 
   MatchErrorCode dump_entry(std::ostream *out,
                             entry_handle_t handle) const {
-    ReadLock lock = lock_read();
+    auto lock = lock_read();
+    // TODO(antonin): this seems too conservative: we know that indirect indices
+    // have to remain valid while a match entry is pointing to it.
+    auto lock_impl = lock_impl_read();
     return dump_entry_(out, handle);
   }
 
   std::string dump_entry_string(entry_handle_t handle) const {
-    ReadLock lock = lock_read();
+    auto lock = lock_read();
+    auto lock_impl = lock_impl_read();
     return dump_entry_string_(handle);
   }
 
@@ -224,6 +228,25 @@ class MatchTableAbstract : public NamedP4Object {
   virtual MatchErrorCode dump_entry_(std::ostream *out,
                                      entry_handle_t handle) const = 0;
 
+  // TODO(antonin): I realized that since the action profile refactor from 2017
+  // - to enable action profile sharing -, there were race conditions all over
+  // the place as the action profile itself was no longer protected by the mutex
+  // of its parent table (there is no single parent table any more). The most
+  // obvious issue was when calling apply_action(). Because of the complexity of
+  // the ActionEntry structure, lookup() returns it by reference. However, the
+  // action profile mutex was not held during the duration of apply_action. This
+  // means that modifications to the action profile ActionEntry vector (such as
+  // adding new members, potentially leading to memory reallocation) could read
+  // to the reference being "invalidated". This was actually very easy to
+  // trigger in a unit test with 2 competing threads, one calling apply_action
+  // repeatedly on a packet and one creating a multitude of members.
+  // As a temporary solution I have added these virtual methods to acquire the
+  // "implementation" mutex (in this case of an indirect table, the action
+  // profile's lock). I could think of several alternatives but nothing I
+  // liked.
+  virtual ReadLock lock_impl_read() const { return ReadLock(); }
+  virtual WriteLock lock_impl_write() const { return WriteLock(); }
+
   // the internal version does not acquire the lock
   std::string dump_entry_string_(entry_handle_t handle) const;
 
@@ -403,6 +426,9 @@ class MatchTableIndirect : public MatchTableAbstract {
 
   MatchErrorCode get_entry_(entry_handle_t handle, Entry *entry) const;
 
+  ReadLock lock_impl_read() const override;
+  WriteLock lock_impl_write() const override;
+
  protected:
   IndirectIndex default_index{};
   std::unique_ptr<MatchUnitAbstract<IndirectIndex> > match_unit;

src/bm_sim/action_profile.cpp

@@ -449,7 +449,7 @@ ActionProfile::get_member(mbr_hdl_t mbr, Member *member) const {
 std::vector<ActionProfile::Member>
 ActionProfile::get_members() const {
   ReadLock lock = lock_read();
-  std::vector<Member> members(get_num_members());
+  std::vector<Member> members(num_members);
   size_t idx = 0;
   for (const auto h : mbr_handles) {
     MatchErrorCode rc = get_member_(h, &members[idx++]);
@@ -480,7 +480,7 @@ ActionProfile::get_group(grp_hdl_t grp, Group *group) const {
 std::vector<ActionProfile::Group>
 ActionProfile::get_groups() const {
   ReadLock lock = lock_read();
-  std::vector<Group> groups(get_num_groups());
+  std::vector<Group> groups(num_groups);
   size_t idx = 0;
   for (const auto h : grp_handles) {
     MatchErrorCode rc = get_group_(h, &groups[idx++]);
@@ -502,6 +502,18 @@ ActionProfile::get_num_members_in_group(grp_hdl_t grp, size_t *nb) const {
   return MatchErrorCode::SUCCESS;
 }
 
+size_t
+ActionProfile::get_num_members() const {
+  ReadLock lock = lock_read();
+  return num_members;
+}
+
+size_t
+ActionProfile::get_num_groups() const {
+  ReadLock lock = lock_read();
+  return num_groups;
+}
+
 void
 ActionProfile::set_group_selector(GroupSelectionIface *selector) {
   WriteLock lock = lock_write();
@@ -515,6 +527,7 @@ ActionProfile::group_is_empty(grp_hdl_t grp) const {
 
 void
 ActionProfile::reset_state() {
+  WriteLock lock = lock_write();
   index_ref_count = IndirectIndexRefCount();
   mbr_handles.clear();
   action_entries.clear();
@@ -526,6 +539,7 @@ ActionProfile::reset_state() {
 
 void
 ActionProfile::serialize(std::ostream *out) const {
+  ReadLock lock = lock_read();
   (*out) << action_entries.size() << "\n";
   (*out) << num_members << "\n";
   for (const auto h : mbr_handles) {
@@ -542,6 +556,7 @@ ActionProfile::serialize(std::ostream *out) const {
 
 void
 ActionProfile::deserialize(std::istream *in, const P4Objects &objs) {
+  ReadLock lock = lock_read();
   size_t action_entries_size; (*in) >> action_entries_size;
   action_entries.resize(action_entries_size);
   (*in) >> num_members;