From 96ffe3733c9077cbbd5c889a3085fe569ac6c6dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= <maze@google.com>
Date: Mon, 20 Jun 2022 13:50:48 -0700
Subject: [PATCH] refactor common logic into abortOnKeyOrValueSizeMismatch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Per request on:
  https://googleplex-android-review.googlesource.com/c/platform/frameworks/libs/net/+/18992756

While we're at it let's temporarily remove the check in .reset(fd)
  if (bpfGetFdMapFlags(mMapFd) != 0) abort(); // TODO: fix for BpfMapRO

We'll add it back when the code is in better shape,
and read-only vs read-write state of the map is something
we actually *know*.

Test: TreeHugger
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: Id8d65bdc529872685b42656e638f22048fafb7f6
---
 .../native/bpf_headers/include/bpf/BpfMap.h   | 32 +++++++++----------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/staticlibs/native/bpf_headers/include/bpf/BpfMap.h b/staticlibs/native/bpf_headers/include/bpf/BpfMap.h
index 297b20017b..47256fa098 100644
--- a/staticlibs/native/bpf_headers/include/bpf/BpfMap.h
+++ b/staticlibs/native/bpf_headers/include/bpf/BpfMap.h
@@ -50,10 +50,8 @@ class BpfMap {
     // (later on, for testing, we still make available a copy assignment operator)
     BpfMap<Key, Value>(const BpfMap<Key, Value>&) = delete;
 
-  protected:
-    // flag must be within BPF_OBJ_FLAG_MASK, ie. 0, BPF_F_RDONLY, BPF_F_WRONLY
-    BpfMap<Key, Value>(const char* pathname, uint32_t flags) {
-        mMapFd.reset(mapRetrieve(pathname, flags));
+  private:
+    void abortOnKeyOrValueSizeMismatch() {
         if (!mMapFd.ok()) abort();
         if (isAtLeastKernelVersion(4, 14, 0)) {
             if (bpfGetFdKeySize(mMapFd) != sizeof(Key)) abort();
@@ -61,6 +59,13 @@ class BpfMap {
         }
     }
 
+  protected:
+    // flag must be within BPF_OBJ_FLAG_MASK, ie. 0, BPF_F_RDONLY, BPF_F_WRONLY
+    BpfMap<Key, Value>(const char* pathname, uint32_t flags) {
+        mMapFd.reset(mapRetrieve(pathname, flags));
+        abortOnKeyOrValueSizeMismatch();
+    }
+
   public:
     explicit BpfMap<Key, Value>(const char* pathname) : BpfMap<Key, Value>(pathname, 0) {}
 
@@ -117,14 +122,11 @@ class BpfMap {
         if (!mMapFd.ok()) {
             return ErrnoErrorf("Pinned map not accessible or does not exist: ({})", path);
         }
-        if (isAtLeastKernelVersion(4, 14, 0)) {
-            // Normally we should return an error here instead of calling abort,
-            // but this cannot happen at runtime without a massive code bug (K/V type mismatch)
-            // and as such it's better to just blow the system up and let the developer fix it.
-            // Crashes are much more likely to be noticed than logs and missing functionality.
-            if (bpfGetFdKeySize(mMapFd) != sizeof(Key)) abort();
-            if (bpfGetFdValueSize(mMapFd) != sizeof(Value)) abort();
-        }
+        // Normally we should return an error here instead of calling abort,
+        // but this cannot happen at runtime without a massive code bug (K/V type mismatch)
+        // and as such it's better to just blow the system up and let the developer fix it.
+        // Crashes are much more likely to be noticed than logs and missing functionality.
+        abortOnKeyOrValueSizeMismatch();
         return {};
     }
 
@@ -202,11 +204,7 @@ class BpfMap {
     // check BpfMap.isValid() and look at errno and see why systemcall() failed.
     [[clang::reinitializes]] void reset(int fd) {
         mMapFd.reset(fd);
-        if ((fd >= 0) && isAtLeastKernelVersion(4, 14, 0)) {
-            if (bpfGetFdKeySize(mMapFd) != sizeof(Key)) abort();
-            if (bpfGetFdValueSize(mMapFd) != sizeof(Value)) abort();
-            if (bpfGetFdMapFlags(mMapFd) != 0) abort(); // TODO: fix for BpfMapRO
-        }
+        if (mMapFd.ok()) abortOnKeyOrValueSizeMismatch();
     }
 #endif