commit 74e873607b6945db8daf3303f599347f223d0132
author Ryan Zuklie <rzuklie@google.com> Thu Feb 23 16:22:39 2023 -0800
committer Ryan Zuklie <rzuklie@google.com> Fri Mar 03 14:29:57 2023 -0800
tree 13574309d67456b937a4126e6849f0a67b08f14e
parent 572bbe444f949083b1cf30d434445c621c7d1232

Make NetworkTraceHandler thread safe

This adds a mutex to guard access to all fields. The lock is taken
liberally since calls should rarely (if ever) collide (for example,
ConsumeAll is called at most every 100ms and shouldn't overlap).

Bug: 246985031
Test: atest libnetworkstats_test
Change-Id: I97791a808771bafe789091c9b54cbec0a31d1721

service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp

diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp
index 894ff7d..c9c79df 100644
--- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp
+++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp
@@ -79,8 +79,14 @@
 
 void NetworkTracePoller::SchedulePolling() {
   // Schedules another run of ourselves to recursively poll periodically.
-  mTaskRunner->PostDelayedTask([this]() { SchedulePolling(); }, mPollMs);
-  ConsumeAll();
+  mTaskRunner->PostDelayedTask(
+      [this]() {
+        mMutex.lock();
+        SchedulePolling();
+        ConsumeAllLocked();
+        mMutex.unlock();
+      },
+      mPollMs);
 }
 
 // static class method
@@ -110,6 +116,7 @@
 bool NetworkTracePoller::Start(uint32_t pollMs) {
   ALOGD("Starting datasource");
 
+  std::scoped_lock<std::mutex> lock(mMutex);
   auto status = mConfigurationMap.init(PACKET_TRACE_ENABLED_MAP_PATH);
   if (!status.ok()) {
     ALOGW("Failed to bind config map: %s", status.error().message().c_str());
@@ -141,6 +148,7 @@
 bool NetworkTracePoller::Stop() {
   ALOGD("Stopping datasource");
 
+  std::scoped_lock<std::mutex> lock(mMutex);
   auto res = mConfigurationMap.writeValue(0, false, BPF_ANY);
   if (!res.ok()) {
     ALOGW("Failed to disable tracing: %s", res.error().message().c_str());
@@ -153,6 +161,11 @@
 }
 
 bool NetworkTracePoller::ConsumeAll() {
+  std::scoped_lock<std::mutex> lock(mMutex);
+  return ConsumeAllLocked();
+}
+
+bool NetworkTracePoller::ConsumeAllLocked() {
   if (mRingBuffer == nullptr) {
     ALOGW("Tracing is not active");
     return false;

service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h

diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h
index 1ab35ef..2a2243b 100644
--- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h
+++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h
@@ -22,6 +22,7 @@
 #include <string>
 #include <unordered_map>
 
+#include "android-base/thread_annotations.h"
 #include "bpf/BpfMap.h"
 #include "bpf/BpfRingbuf.h"
 
@@ -40,29 +41,36 @@
   NetworkTracePoller(std::function<void(const PacketTrace&)> callback)
       : mCallback(std::move(callback)) {}
 
-  // Testonly: standalone functions without perfetto dependency.
-  bool Start(uint32_t pollMs);
-  bool Stop();
-  bool ConsumeAll();
+  // Starts tracing with the given poll interval.
+  bool Start(uint32_t pollMs) EXCLUDES(mMutex);
+
+  // Stops tracing and release any held state.
+  bool Stop() EXCLUDES(mMutex);
+
+  // Consumes all available events from the ringbuffer.
+  bool ConsumeAll() EXCLUDES(mMutex);
 
  private:
-  void SchedulePolling();
+  void SchedulePolling() REQUIRES(mMutex);
+  bool ConsumeAllLocked() REQUIRES(mMutex);
+
+  std::mutex mMutex;
 
   // How often to poll the ring buffer, defined by the trace config.
-  uint32_t mPollMs;
+  uint32_t mPollMs GUARDED_BY(mMutex);
 
   // The function to process PacketTrace, typically a Perfetto sink.
-  std::function<void(const PacketTrace&)> mCallback;
+  std::function<void(const PacketTrace&)> mCallback GUARDED_BY(mMutex);
 
   // The BPF ring buffer handle.
-  std::unique_ptr<BpfRingbuf<PacketTrace>> mRingBuffer;
+  std::unique_ptr<BpfRingbuf<PacketTrace>> mRingBuffer GUARDED_BY(mMutex);
 
   // The packet tracing config map (really a 1-element array).
-  BpfMap<uint32_t, bool> mConfigurationMap;
+  BpfMap<uint32_t, bool> mConfigurationMap GUARDED_BY(mMutex);
 
   // This must be the last member, causing it to be the first deleted. If it is
   // not, members required for callbacks can be deleted before it's stopped.
-  std::unique_ptr<perfetto::base::TaskRunner> mTaskRunner;
+  std::unique_ptr<perfetto::base::TaskRunner> mTaskRunner GUARDED_BY(mMutex);
 };
 
 // NetworkTraceHandler implements the android.network_packets data source. This