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));
- }
- }
}