Merge "Remove debug log from NetworkStatsService"
diff --git a/Tethering/Android.bp b/Tethering/Android.bp
index adcc236..19d0d5f 100644
--- a/Tethering/Android.bp
+++ b/Tethering/Android.bp
@@ -179,9 +179,9 @@
certificate: "networkstack",
manifest: "AndroidManifest.xml",
use_embedded_native_libs: true,
- // The permission configuration *must* be included to ensure security of the device
+ // The network stack *must* be included to ensure security of the device
required: [
- "NetworkPermissionConfig",
+ "NetworkStack",
"privapp_allowlist_com.android.tethering",
],
apex_available: ["com.android.tethering"],
@@ -199,9 +199,9 @@
certificate: "networkstack",
manifest: "AndroidManifest.xml",
use_embedded_native_libs: true,
- // The permission configuration *must* be included to ensure security of the device
+ // The network stack *must* be included to ensure security of the device
required: [
- "NetworkPermissionConfig",
+ "NetworkStackNext",
"privapp_allowlist_com.android.tethering",
],
apex_available: ["com.android.tethering"],
diff --git a/bpf_progs/netd.c b/bpf_progs/netd.c
index 94d5ed8..cb1714c 100644
--- a/bpf_progs/netd.c
+++ b/bpf_progs/netd.c
@@ -189,6 +189,11 @@
return *config;
}
+// DROP_IF_SET is set of rules that BPF_DROP if rule is globally enabled, and per-uid bit is set
+#define DROP_IF_SET (STANDBY_MATCH | OEM_DENY_1_MATCH | OEM_DENY_2_MATCH | OEM_DENY_3_MATCH)
+// DROP_IF_UNSET is set of rules that should DROP if globally enabled, and per-uid bit is NOT set
+#define DROP_IF_UNSET (DOZABLE_MATCH | POWERSAVE_MATCH | RESTRICTED_MATCH | LOW_POWER_STANDBY_MATCH)
+
static inline int bpf_owner_match(struct __sk_buff* skb, uint32_t uid, int direction) {
if (skip_owner_match(skb)) return BPF_PASS;
@@ -200,32 +205,13 @@
uint32_t uidRules = uidEntry ? uidEntry->rule : 0;
uint32_t allowed_iif = uidEntry ? uidEntry->iif : 0;
- if (enabledRules) {
- if ((enabledRules & DOZABLE_MATCH) && !(uidRules & DOZABLE_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & STANDBY_MATCH) && (uidRules & STANDBY_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & POWERSAVE_MATCH) && !(uidRules & POWERSAVE_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & RESTRICTED_MATCH) && !(uidRules & RESTRICTED_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & LOW_POWER_STANDBY_MATCH) && !(uidRules & LOW_POWER_STANDBY_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & OEM_DENY_1_MATCH) && (uidRules & OEM_DENY_1_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & OEM_DENY_2_MATCH) && (uidRules & OEM_DENY_2_MATCH)) {
- return BPF_DROP;
- }
- if ((enabledRules & OEM_DENY_3_MATCH) && (uidRules & OEM_DENY_3_MATCH)) {
- return BPF_DROP;
- }
- }
+ // Warning: funky bit-wise arithmetic: in parallel, for all DROP_IF_SET/UNSET rules
+ // check whether the rules are globally enabled, and if so whether the rules are
+ // set/unset for the specific uid. BPF_DROP if that is the case for ANY of the rules.
+ // We achieve this by masking out only the bits/rules we're interested in checking,
+ // and negating (via bit-wise xor) the bits/rules that should drop if unset.
+ if (enabledRules & (DROP_IF_SET | DROP_IF_UNSET) & (uidRules ^ DROP_IF_UNSET)) return BPF_DROP;
+
if (direction == BPF_INGRESS && skb->ifindex != 1) {
if (uidRules & IIF_MATCH) {
if (allowed_iif && skb->ifindex != allowed_iif) {
diff --git a/service/native/TrafficControllerTest.cpp b/service/native/TrafficControllerTest.cpp
index 1aca633..f63a3d9 100644
--- a/service/native/TrafficControllerTest.cpp
+++ b/service/native/TrafficControllerTest.cpp
@@ -790,6 +790,46 @@
EXPECT_TRUE(expectDumpsysContains(expectedLines));
}
+TEST_F(TrafficControllerTest, uidMatchTypeToString) {
+ // NO_MATCH(0) can't be verified because match type flag is added by OR operator.
+ // See TrafficController::addRule()
+ static const struct TestConfig {
+ UidOwnerMatchType uidOwnerMatchType;
+ std::string expected;
+ } testConfigs[] = {
+ // clang-format off
+ {HAPPY_BOX_MATCH, "HAPPY_BOX_MATCH"},
+ {DOZABLE_MATCH, "DOZABLE_MATCH"},
+ {STANDBY_MATCH, "STANDBY_MATCH"},
+ {POWERSAVE_MATCH, "POWERSAVE_MATCH"},
+ {HAPPY_BOX_MATCH, "HAPPY_BOX_MATCH"},
+ {RESTRICTED_MATCH, "RESTRICTED_MATCH"},
+ {LOW_POWER_STANDBY_MATCH, "LOW_POWER_STANDBY_MATCH"},
+ {IIF_MATCH, "IIF_MATCH"},
+ {LOCKDOWN_VPN_MATCH, "LOCKDOWN_VPN_MATCH"},
+ {OEM_DENY_1_MATCH, "OEM_DENY_1_MATCH"},
+ {OEM_DENY_2_MATCH, "OEM_DENY_2_MATCH"},
+ {OEM_DENY_3_MATCH, "OEM_DENY_3_MATCH"},
+ // clang-format on
+ };
+
+ for (const auto& config : testConfigs) {
+ SCOPED_TRACE(fmt::format("testConfig: [{}, {}]", config.uidOwnerMatchType,
+ config.expected));
+
+ // Test private function uidMatchTypeToString() via dumpsys.
+ ASSERT_TRUE(isOk(updateUidOwnerMaps({TEST_UID}, config.uidOwnerMatchType,
+ TrafficController::IptOpInsert)));
+ std::vector<std::string> expectedLines;
+ expectedLines.emplace_back(fmt::format("{} {}", TEST_UID, config.expected));
+ EXPECT_TRUE(expectDumpsysContains(expectedLines));
+
+ // Clean up the stubs.
+ ASSERT_TRUE(isOk(updateUidOwnerMaps({TEST_UID}, config.uidOwnerMatchType,
+ TrafficController::IptOpDelete)));
+ }
+}
+
TEST_F(TrafficControllerTest, getFirewallType) {
static const struct TestConfig {
ChildChain childChain;
@@ -805,6 +845,7 @@
{LOCKDOWN, DENYLIST},
{OEM_DENY_1, DENYLIST},
{OEM_DENY_2, DENYLIST},
+ {OEM_DENY_3, DENYLIST},
{INVALID_CHAIN, DENYLIST},
// clang-format on
};
diff --git a/service/src/com/android/server/connectivity/ClatCoordinator.java b/service/src/com/android/server/connectivity/ClatCoordinator.java
index 8cefd47..ea89be6 100644
--- a/service/src/com/android/server/connectivity/ClatCoordinator.java
+++ b/service/src/com/android/server/connectivity/ClatCoordinator.java
@@ -498,6 +498,31 @@
}
}
+ private void maybeCleanUp(ParcelFileDescriptor tunFd, ParcelFileDescriptor readSock6,
+ ParcelFileDescriptor writeSock6) {
+ if (tunFd != null) {
+ try {
+ tunFd.close();
+ } catch (IOException e) {
+ Log.e(TAG, "Fail to close tun file descriptor " + e);
+ }
+ }
+ if (readSock6 != null) {
+ try {
+ readSock6.close();
+ } catch (IOException e) {
+ Log.e(TAG, "Fail to close read socket " + e);
+ }
+ }
+ if (writeSock6 != null) {
+ try {
+ writeSock6.close();
+ } catch (IOException e) {
+ Log.e(TAG, "Fail to close write socket " + e);
+ }
+ }
+ }
+
/**
* Start clatd for a given interface and NAT64 prefix.
*/
@@ -546,8 +571,15 @@
// [3] Open, configure and bring up the tun interface.
// Create the v4-... tun interface.
+
+ // Initialize all required file descriptors with null pointer. This makes the following
+ // error handling easier. Simply always call #maybeCleanUp for closing file descriptors,
+ // if any valid ones, in error handling.
+ ParcelFileDescriptor tunFd = null;
+ ParcelFileDescriptor readSock6 = null;
+ ParcelFileDescriptor writeSock6 = null;
+
final String tunIface = CLAT_PREFIX + iface;
- final ParcelFileDescriptor tunFd;
try {
tunFd = mDeps.adoptFd(mDeps.createTunInterface(tunIface));
} catch (IOException e) {
@@ -556,7 +588,7 @@
final int tunIfIndex = mDeps.getInterfaceIndex(tunIface);
if (tunIfIndex == INVALID_IFINDEX) {
- tunFd.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Fail to get interface index for interface " + tunIface);
}
@@ -564,7 +596,7 @@
try {
mNetd.interfaceSetEnableIPv6(tunIface, false /* enabled */);
} catch (RemoteException | ServiceSpecificException e) {
- tunFd.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
Log.e(TAG, "Disable IPv6 on " + tunIface + " failed: " + e);
}
@@ -575,19 +607,17 @@
detectedMtu = mDeps.detectMtu(pfx96Str,
ByteBuffer.wrap(GOOGLE_DNS_4.getAddress()).getInt(), fwmark);
} catch (IOException e) {
- tunFd.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Detect MTU on " + tunIface + " failed: " + e);
}
final int mtu = adjustMtu(detectedMtu);
Log.i(TAG, "ipv4 mtu is " + mtu);
- // TODO: add setIptablesDropRule
-
// Config tun interface mtu, address and bring up.
try {
mNetd.interfaceSetMtu(tunIface, mtu);
} catch (RemoteException | ServiceSpecificException e) {
- tunFd.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Set MTU " + mtu + " on " + tunIface + " failed: " + e);
}
final InterfaceConfigurationParcel ifConfig = new InterfaceConfigurationParcel();
@@ -599,14 +629,13 @@
try {
mNetd.interfaceSetCfg(ifConfig);
} catch (RemoteException | ServiceSpecificException e) {
- tunFd.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Setting IPv4 address to " + ifConfig.ipv4Addr + "/"
+ ifConfig.prefixLength + " failed on " + ifConfig.ifName + ": " + e);
}
// [4] Open and configure local 464xlat read/write sockets.
// Opens a packet socket to receive IPv6 packets in clatd.
- final ParcelFileDescriptor readSock6;
try {
// Use a JNI call to get native file descriptor instead of Os.socket() because we would
// like to use ParcelFileDescriptor to manage file descriptor. But ctor
@@ -614,27 +643,23 @@
// descriptor to initialize ParcelFileDescriptor object instead.
readSock6 = mDeps.adoptFd(mDeps.openPacketSocket());
} catch (IOException e) {
- tunFd.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Open packet socket failed: " + e);
}
// Opens a raw socket with a given fwmark to send IPv6 packets in clatd.
- final ParcelFileDescriptor writeSock6;
try {
// Use a JNI call to get native file descriptor instead of Os.socket(). See above
// reason why we use jniOpenPacketSocket6().
writeSock6 = mDeps.adoptFd(mDeps.openRawSocket6(fwmark));
} catch (IOException e) {
- tunFd.close();
- readSock6.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Open raw socket failed: " + e);
}
final int ifIndex = mDeps.getInterfaceIndex(iface);
if (ifIndex == INVALID_IFINDEX) {
- tunFd.close();
- readSock6.close();
- writeSock6.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("Fail to get interface index for interface " + iface);
}
@@ -642,9 +667,7 @@
try {
mDeps.addAnycastSetsockopt(writeSock6.getFileDescriptor(), v6Str, ifIndex);
} catch (IOException e) {
- tunFd.close();
- readSock6.close();
- writeSock6.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("add anycast sockopt failed: " + e);
}
@@ -653,9 +676,7 @@
try {
cookie = mDeps.tagSocketAsClat(writeSock6.getFileDescriptor());
} catch (IOException e) {
- tunFd.close();
- readSock6.close();
- writeSock6.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("tag raw socket failed: " + e);
}
@@ -663,9 +684,7 @@
try {
mDeps.configurePacketSocket(readSock6.getFileDescriptor(), v6Str, ifIndex);
} catch (IOException e) {
- tunFd.close();
- readSock6.close();
- writeSock6.close();
+ maybeCleanUp(tunFd, readSock6, writeSock6);
throw new IOException("configure packet socket failed: " + e);
}
@@ -679,9 +698,9 @@
mDeps.untagSocket(cookie);
throw new IOException("Error start clatd on " + iface + ": " + e);
} finally {
- tunFd.close();
- readSock6.close();
- writeSock6.close();
+ // The file descriptors have been duplicated (dup2) to clatd in native_startClatd().
+ // Close these file descriptor stubs which are unused anymore.
+ maybeCleanUp(tunFd, readSock6, writeSock6);
}
// [6] Initialize and store clatd tracker object.
diff --git a/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java b/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java
index 3047a16..8dfe924 100644
--- a/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java
+++ b/tests/unit/java/com/android/server/connectivity/ClatCoordinatorTest.java
@@ -37,6 +37,7 @@
import static org.mockito.Mockito.clearInvocations;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
@@ -556,14 +557,28 @@
() -> coordinator.clatStart(BASE_IFACE, NETID, NAT64_IP_PREFIX));
}
- private void checkNotStartClat(final TestDependencies deps, final boolean verifyTunFd,
- final boolean verifyPacketSockFd, final boolean verifyRawSockFd) throws Exception {
+ private void checkNotStartClat(final TestDependencies deps, final boolean needToCloseTunFd,
+ final boolean needToClosePacketSockFd, final boolean needToCloseRawSockFd)
+ throws Exception {
// [1] Expect that modified TestDependencies can't start clatd.
+ // Use precise check to make sure that there is no unexpected file descriptor closing.
clearInvocations(TUN_PFD, RAW_SOCK_PFD, PACKET_SOCK_PFD);
assertNotStartClat(deps);
- if (verifyTunFd) verify(TUN_PFD).close();
- if (verifyPacketSockFd) verify(PACKET_SOCK_PFD).close();
- if (verifyRawSockFd) verify(RAW_SOCK_PFD).close();
+ if (needToCloseTunFd) {
+ verify(TUN_PFD).close();
+ } else {
+ verify(TUN_PFD, never()).close();
+ }
+ if (needToClosePacketSockFd) {
+ verify(PACKET_SOCK_PFD).close();
+ } else {
+ verify(PACKET_SOCK_PFD, never()).close();
+ }
+ if (needToCloseRawSockFd) {
+ verify(RAW_SOCK_PFD).close();
+ } else {
+ verify(RAW_SOCK_PFD, never()).close();
+ }
// [2] Expect that unmodified TestDependencies can start clatd.
// Used to make sure that the above modified TestDependencies has really broken the
@@ -582,8 +597,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), false /* verifyTunFd */,
- false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), false /* needToCloseTunFd */,
+ false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -595,8 +610,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), false /* verifyTunFd */,
- false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), false /* needToCloseTunFd */,
+ false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -607,8 +622,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), false /* verifyTunFd */,
- false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), false /* needToCloseTunFd */,
+ false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -620,8 +635,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -632,8 +647,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- false /* verifyPacketSockFd */, false /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ false /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -644,8 +659,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- true /* verifyPacketSockFd */, false /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ true /* needToClosePacketSockFd */, false /* needToCloseRawSockFd */);
}
@Test
@@ -657,8 +672,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
@Test
@@ -669,8 +684,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
@Test
@@ -682,8 +697,8 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
@Test
@@ -697,7 +712,7 @@
throw new IOException();
}
}
- checkNotStartClat(new FailureDependencies(), true /* verifyTunFd */,
- true /* verifyPacketSockFd */, true /* verifyRawSockFd */);
+ checkNotStartClat(new FailureDependencies(), true /* needToCloseTunFd */,
+ true /* needToClosePacketSockFd */, true /* needToCloseRawSockFd */);
}
}