Merge "dscpPolicyTest: make it actually test caching" into main
diff --git a/bpf/headers/include/bpf_helpers.h b/bpf/headers/include/bpf_helpers.h
index 1a9fd31..ac5ffda 100644
--- a/bpf/headers/include/bpf_helpers.h
+++ b/bpf/headers/include/bpf_helpers.h
@@ -291,6 +291,12 @@
         bpf_ringbuf_submit_unsafe(v, 0);                                       \
     }
 
+#define DEFINE_BPF_RINGBUF(the_map, ValueType, size_bytes, usr, grp, md)                \
+    DEFINE_BPF_RINGBUF_EXT(the_map, ValueType, size_bytes, usr, grp, md,                \
+                           DEFAULT_BPF_MAP_SELINUX_CONTEXT, DEFAULT_BPF_MAP_PIN_SUBDIR, \
+                           PRIVATE, BPFLOADER_MIN_VER, BPFLOADER_MAX_VER,               \
+                           LOAD_ON_ENG, LOAD_ON_USER, LOAD_ON_USERDEBUG)
+
 /* There exist buggy kernels with pre-T OS, that due to
  * kernel patch "[ALPS05162612] bpf: fix ubsan error"
  * do not support userspace writes into non-zero index of bpf map arrays.
@@ -349,11 +355,17 @@
 #error "Bpf Map UID must be left at default of AID_ROOT for BpfLoader prior to v0.28"
 #endif
 
-#define DEFINE_BPF_MAP_UGM(the_map, TYPE, KeyType, ValueType, num_entries, usr, grp, md)     \
-    DEFINE_BPF_MAP_EXT(the_map, TYPE, KeyType, ValueType, num_entries, usr, grp, md,         \
-                       DEFAULT_BPF_MAP_SELINUX_CONTEXT, DEFAULT_BPF_MAP_PIN_SUBDIR, PRIVATE, \
-                       BPFLOADER_MIN_VER, BPFLOADER_MAX_VER, LOAD_ON_ENG,                    \
-                       LOAD_ON_USER, LOAD_ON_USERDEBUG)
+// for maps not meant to be accessed from userspace
+#define DEFINE_BPF_MAP_KERNEL_INTERNAL(the_map, TYPE, KeyType, ValueType, num_entries)           \
+    DEFINE_BPF_MAP_EXT(the_map, TYPE, KeyType, ValueType, num_entries, AID_ROOT, AID_ROOT,       \
+                       0000, "fs_bpf_loader", "", PRIVATE, BPFLOADER_MIN_VER, BPFLOADER_MAX_VER, \
+                       LOAD_ON_ENG, LOAD_ON_USER, LOAD_ON_USERDEBUG)
+
+#define DEFINE_BPF_MAP_UGM(the_map, TYPE, KeyType, ValueType, num_entries, usr, grp, md) \
+    DEFINE_BPF_MAP_EXT(the_map, TYPE, KeyType, ValueType, num_entries, usr, grp, md,     \
+                       DEFAULT_BPF_MAP_SELINUX_CONTEXT, DEFAULT_BPF_MAP_PIN_SUBDIR,      \
+                       PRIVATE, BPFLOADER_MIN_VER, BPFLOADER_MAX_VER,                    \
+                       LOAD_ON_ENG, LOAD_ON_USER, LOAD_ON_USERDEBUG)
 
 #define DEFINE_BPF_MAP(the_map, TYPE, KeyType, ValueType, num_entries) \
     DEFINE_BPF_MAP_UGM(the_map, TYPE, KeyType, ValueType, num_entries, \
diff --git a/bpf/loader/NetBpfLoad.cpp b/bpf/loader/NetBpfLoad.cpp
index 5d4cd42..4767dfa 100644
--- a/bpf/loader/NetBpfLoad.cpp
+++ b/bpf/loader/NetBpfLoad.cpp
@@ -97,6 +97,8 @@
     net_shared,         // (T+) fs_bpf_net_shared    /sys/fs/bpf/net_shared
     netd_readonly,      // (T+) fs_bpf_netd_readonly /sys/fs/bpf/netd_readonly
     netd_shared,        // (T+) fs_bpf_netd_shared   /sys/fs/bpf/netd_shared
+    loader,             // (U+) fs_bpf_loader        /sys/fs/bpf/loader
+                        // on T due to lack of sepolicy/genfscon rules it behaves simply as 'fs_bpf'
 };
 
 static constexpr domain AllDomains[] = {
@@ -106,6 +108,7 @@
     domain::net_shared,
     domain::netd_readonly,
     domain::netd_shared,
+    domain::loader,
 };
 
 static constexpr bool specified(domain d) {
@@ -151,6 +154,7 @@
         case domain::net_shared:    return "fs_bpf_net_shared";
         case domain::netd_readonly: return "fs_bpf_netd_readonly";
         case domain::netd_shared:   return "fs_bpf_netd_shared";
+        case domain::loader:        return "fs_bpf_loader";
     }
 }
 
@@ -174,6 +178,7 @@
         case domain::net_shared:    return "net_shared/";
         case domain::netd_readonly: return "netd_readonly/";
         case domain::netd_shared:   return "netd_shared/";
+        case domain::loader:        return "loader/";
     }
 };
 
diff --git a/bpf/progs/bpf_net_helpers.h b/bpf/progs/bpf_net_helpers.h
index a86c3e6..a5664ba 100644
--- a/bpf/progs/bpf_net_helpers.h
+++ b/bpf/progs/bpf_net_helpers.h
@@ -139,6 +139,24 @@
     if (skb->data_end - skb->data < len) bpf_skb_pull_data(skb, len);
 }
 
+// anti-compiler-optimizer no-op: explicitly force full calculation of 'v'
+//
+// The use for this is to force full calculation of a complex arithmetic (likely binary
+// bitops) value, and then check the result only once (thus likely reducing the number
+// of required conditional jump instructions that badly affect bpf verifier runtime)
+//
+// The compiler cannot look into the assembly statement, so it doesn't know it does nothing.
+// Since the statement takes 'v' as both input and output in a register (+r),
+// the compiler must fully calculate the precise value of 'v' before this,
+// and must use the (possibly modified) value of 'v' afterwards (thus cannot
+// do funky optimizations to use partial results from before the asm).
+//
+// As this is not flagged 'volatile' this may still be moved out of a loop,
+// or even entirely optimized out if 'v' is never used afterwards.
+//
+// See: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
+#define COMPILER_FORCE_CALCULATION(v) asm ("" : "+r" (v))
+
 struct egress_bool { bool egress; };
 #define INGRESS ((struct egress_bool){ .egress = false })
 #define EGRESS ((struct egress_bool){ .egress = true })
diff --git a/bpf/progs/dscpPolicy.c b/bpf/progs/dscpPolicy.c
index 39f2961..de9723d 100644
--- a/bpf/progs/dscpPolicy.c
+++ b/bpf/progs/dscpPolicy.c
@@ -25,8 +25,8 @@
 
 // The cache is never read nor written by userspace and is indexed by socket cookie % CACHE_MAP_SIZE
 #define CACHE_MAP_SIZE 32  // should be a power of two so we can % cheaply
-DEFINE_BPF_MAP_GRO(socket_policy_cache_map, PERCPU_ARRAY, uint32_t, RuleEntry, CACHE_MAP_SIZE,
-                   AID_SYSTEM)
+DEFINE_BPF_MAP_KERNEL_INTERNAL(socket_policy_cache_map, PERCPU_ARRAY, uint32_t, RuleEntry,
+                               CACHE_MAP_SIZE)
 
 DEFINE_BPF_MAP_GRW(ipv4_dscp_policies_map, ARRAY, uint32_t, DscpPolicy, MAX_POLICIES, AID_SYSTEM)
 DEFINE_BPF_MAP_GRW(ipv6_dscp_policies_map, ARRAY, uint32_t, DscpPolicy, MAX_POLICIES, AID_SYSTEM)
@@ -113,14 +113,30 @@
     // this array lookup cannot actually fail
     RuleEntry* existing_rule = bpf_socket_policy_cache_map_lookup_elem(&cacheid);
 
-    if (existing_rule &&
-        v6_equal(src_ip, existing_rule->src_ip) &&
-        v6_equal(dst_ip, existing_rule->dst_ip) &&
-        skb->ifindex == existing_rule->ifindex &&
-        sport == existing_rule->src_port &&
-        dport == existing_rule->dst_port &&
-        protocol == existing_rule->proto) {
-        if (existing_rule->dscp_val < 0) return;
+    if (!existing_rule) return; // impossible
+
+    uint64_t nomatch = 0;
+    nomatch |= v6_not_equal(src_ip, existing_rule->src_ip);
+    nomatch |= v6_not_equal(dst_ip, existing_rule->dst_ip);
+    nomatch |= (skb->ifindex ^ existing_rule->ifindex);
+    nomatch |= (sport ^ existing_rule->src_port);
+    nomatch |= (dport ^ existing_rule->dst_port);
+    nomatch |= (protocol ^ existing_rule->proto);
+    COMPILER_FORCE_CALCULATION(nomatch);
+
+    /*
+     * After the above funky bitwise arithmetic we have 'nomatch == 0' iff
+     *   src_ip == existing_rule->src_ip &&
+     *   dst_ip == existing_rule->dst_ip &&
+     *   skb->ifindex == existing_rule->ifindex &&
+     *   sport == existing_rule->src_port &&
+     *   dport == existing_rule->dst_port &&
+     *   protocol == existing_rule->proto
+     */
+
+    if (!nomatch) {
+        if (existing_rule->dscp_val < 0) return;  // cached no-op
+
         if (ipv4) {
             uint8_t newTos = UPDATE_TOS(existing_rule->dscp_val, tos);
             bpf_l3_csum_replace(skb, l2_header_size + IP4_OFFSET(check), htons(tos), htons(newTos),
@@ -132,7 +148,7 @@
             bpf_skb_store_bytes(skb, l2_header_size, &new_first_be32, sizeof(__be32),
                 BPF_F_RECOMPUTE_CSUM);
         }
-        return;
+        return;  // cached DSCP mutation
     }
 
     // Linear scan ipv4_dscp_policies_map since no stored params match skb.
@@ -187,7 +203,8 @@
         }
     }
 
-    RuleEntry value = {
+    // Update cache with found policy.
+    *existing_rule = (RuleEntry){
         .src_ip = src_ip,
         .dst_ip = dst_ip,
         .ifindex = skb->ifindex,
@@ -197,9 +214,6 @@
         .dscp_val = new_dscp,
     };
 
-    // Update cache with found policy.
-    bpf_socket_policy_cache_map_update_elem(&cacheid, &value, BPF_ANY);
-
     if (new_dscp < 0) return;
 
     // Need to store bytes after updating map or program will not load.
diff --git a/bpf/progs/dscpPolicy.h b/bpf/progs/dscpPolicy.h
index 6a6b711..413fb0f 100644
--- a/bpf/progs/dscpPolicy.h
+++ b/bpf/progs/dscpPolicy.h
@@ -28,9 +28,6 @@
 #define v6_not_equal(a, b) ((v6_hi_be64(a) ^ v6_hi_be64(b)) \
                           | (v6_lo_be64(a) ^ v6_lo_be64(b)))
 
-// Returns 'a == b' as boolean
-#define v6_equal(a, b) (!v6_not_equal((a), (b)))
-
 typedef struct {
     struct in6_addr src_ip;
     struct in6_addr dst_ip;
diff --git a/staticlibs/framework/com/android/net/module/util/LocationPermissionChecker.java b/staticlibs/framework/com/android/net/module/util/LocationPermissionChecker.java
index 28c33f3..e4d25cd 100644
--- a/staticlibs/framework/com/android/net/module/util/LocationPermissionChecker.java
+++ b/staticlibs/framework/com/android/net/module/util/LocationPermissionChecker.java
@@ -117,7 +117,11 @@
     @VisibleForTesting(visibility = PRIVATE)
     public @LocationPermissionCheckStatus int checkLocationPermissionInternal(
             String pkgName, @Nullable String featureId, int uid, @Nullable String message) {
-        checkPackage(uid, pkgName);
+        try {
+            checkPackage(uid, pkgName);
+        } catch (SecurityException e) {
+            return ERROR_LOCATION_PERMISSION_MISSING;
+        }
 
         // Apps with NETWORK_SETTINGS, NETWORK_SETUP_WIZARD, NETWORK_STACK & MAINLINE_NETWORK_STACK
         // are granted a bypass.
diff --git a/staticlibs/tests/unit/src/com/android/net/module/util/LocationPermissionCheckerTest.java b/staticlibs/tests/unit/src/com/android/net/module/util/LocationPermissionCheckerTest.java
index c8f8656..d773374 100644
--- a/staticlibs/tests/unit/src/com/android/net/module/util/LocationPermissionCheckerTest.java
+++ b/staticlibs/tests/unit/src/com/android/net/module/util/LocationPermissionCheckerTest.java
@@ -18,7 +18,6 @@
 import static android.Manifest.permission.NETWORK_SETTINGS;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
@@ -47,7 +46,6 @@
 
 import com.android.testutils.DevSdkIgnoreRule;
 
-import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
@@ -242,9 +240,9 @@
         mWifiScanAllowApps = AppOpsManager.MODE_ALLOWED;
         setupTestCase();
 
-        assertThrows(SecurityException.class,
-                () -> mChecker.checkLocationPermissionInternal(
-                        TEST_PKG_NAME, TEST_FEATURE_ID, mUid, null));
+        final int result = mChecker.checkLocationPermissionInternal(
+                        TEST_PKG_NAME, TEST_FEATURE_ID, mUid, null);
+        assertEquals(LocationPermissionChecker.ERROR_LOCATION_PERMISSION_MISSING, result);
     }
 
     @Test
@@ -305,14 +303,4 @@
                         TEST_PKG_NAME, TEST_FEATURE_ID, mUid, null);
         assertEquals(LocationPermissionChecker.SUCCEEDED, result);
     }
-
-
-    private static void assertThrows(Class<? extends Exception> exceptionClass, Runnable r) {
-        try {
-            r.run();
-            Assert.fail("Expected " + exceptionClass + " to be thrown.");
-        } catch (Exception exception) {
-            assertTrue(exceptionClass.isInstance(exception));
-        }
-    }
 }