Don't mark non-inet sockets connecting to inet destinations.
Currently netdClientConnect decides whether to send the socket to
FwmarkServer for marking based on whether the destination address
is IPv4 or IPv6. This is correct in most cases, but not when the
caller does something strange like attempt to connect an AF_UNIX
socket to an IP address. In this specific case, we currently
return EBADF because selinux does not allow passing UNIX sockets
to FwmarkServer, and it gets EBADF because it reads an fd of -1
from the UNIX socket.
Instead, mark sockets only if both the socket address family and
the destination address family are IPv4 or IPv6. In the specific
case of a UNIX socket being connected to an IP address, we will
now return EINVAL, which is marginally more correct since it's
what the caller would have seen if they had been talking directly
to the kernel.
Running the benchmarks indicates that the extra getsockopt call
does not substantially impact performance. On aosp_crosshatch-eng
with governor set to powersave, running one after the other once
resulted in the following, which I think is less than run-to-run
variation:
Before:
crosshatch:/ # /data/netd_benchmark --benchmark_filter=ipv6_high --benchmark_repetitions=10
...
---------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
---------------------------------------------------------------------------------------------------
ipv6_high_load/min_time:0.500/real_time/threads:1 2320979 ns 1111313 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2313373 ns 1110147 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2301823 ns 1106618 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2306347 ns 1108168 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2302559 ns 1106018 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2296351 ns 1104331 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2301156 ns 1108512 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2311693 ns 1114574 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2291240 ns 1104011 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1 2305734 ns 1112399 ns 300
ipv6_high_load/min_time:0.500/real_time/threads:1_mean 2305125 ns 1108609 ns 10
ipv6_high_load/min_time:0.500/real_time/threads:1_median 2304146 ns 1108340 ns 10
ipv6_high_load/min_time:0.500/real_time/threads:1_stddev 8611 ns 3495 ns 10
After:
crosshatch:/ # /data/netd_benchmark --benchmark_filter=ipv6_high --benchmark_repetitions=10
...
---------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations
---------------------------------------------------------------------------------------------------
ipv6_high_load/min_time:0.500/real_time/threads:1 2324244 ns 1127924 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2319556 ns 1121708 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2322220 ns 1124789 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2312343 ns 1117317 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2355727 ns 1118316 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2314515 ns 1120078 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2306399 ns 1120262 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2307413 ns 1120280 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2320109 ns 1121990 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1 2326430 ns 1125545 ns 303
ipv6_high_load/min_time:0.500/real_time/threads:1_mean 2320896 ns 1121821 ns 10
ipv6_high_load/min_time:0.500/real_time/threads:1_median 2319832 ns 1120994 ns 10
ipv6_high_load/min_time:0.500/real_time/threads:1_stddev 14004 ns 3340 ns 10
Fix: 124055201
Test: atest FrameworksNetTests
Test: system/netd/tests/runtests.sh
Test: atest CtsLibcoreTestCases:libcore.libcore.io.OsTest
Test: atest android.net.cts.ConnectivityManagerTest android.net.cts.MultinetworkApiTest
Change-Id: Ic50f13d8f63038fe439cb250b0bc12116f727ace
diff --git a/client/NetdClient.cpp b/client/NetdClient.cpp
index d70f9fa..fa45eb0 100644
--- a/client/NetdClient.cpp
+++ b/client/NetdClient.cpp
@@ -59,6 +59,28 @@
ConnectFunctionType libcConnect = nullptr;
SocketFunctionType libcSocket = nullptr;
+int checkSocket(int socketFd) {
+ if (socketFd < 0) {
+ return -EBADF;
+ }
+ int family;
+ socklen_t familyLen = sizeof(family);
+ if (getsockopt(socketFd, SOL_SOCKET, SO_DOMAIN, &family, &familyLen) == -1) {
+ return -errno;
+ }
+ if (!FwmarkClient::shouldSetFwmark(family)) {
+ return -EAFNOSUPPORT;
+ }
+ return 0;
+}
+
+bool shouldMarkSocket(int socketFd, const sockaddr* dst) {
+ // Only mark inet sockets that are connecting to inet destinations. This excludes, for example,
+ // inet sockets connecting to AF_UNSPEC (i.e., being disconnected), and non-inet sockets that
+ // for some reason the caller wants to attempt to connect to an inet destination.
+ return dst && FwmarkClient::shouldSetFwmark(dst->sa_family) && (checkSocket(socketFd) == 0);
+}
+
int closeFdAndSetErrno(int fd, int error) {
close(fd);
errno = -error;
@@ -89,8 +111,7 @@
}
int netdClientConnect(int sockfd, const sockaddr* addr, socklen_t addrlen) {
- const bool shouldSetFwmark = (sockfd >= 0) && addr
- && FwmarkClient::shouldSetFwmark(addr->sa_family);
+ const bool shouldSetFwmark = shouldMarkSocket(sockfd, addr);
if (shouldSetFwmark) {
FwmarkCommand command = {FwmarkCommand::ON_CONNECT, 0, 0, 0};
if (int error = FwmarkClient().send(&command, sockfd, nullptr)) {
@@ -171,21 +192,6 @@
return error;
}
-int checkSocket(int socketFd) {
- if (socketFd < 0) {
- return -EBADF;
- }
- int family;
- socklen_t familyLen = sizeof(family);
- if (getsockopt(socketFd, SOL_SOCKET, SO_DOMAIN, &family, &familyLen) == -1) {
- return -errno;
- }
- if (!FwmarkClient::shouldSetFwmark(family)) {
- return -EAFNOSUPPORT;
- }
- return 0;
-}
-
int dns_open_proxy() {
const char* cache_mode = getenv("ANDROID_DNS_MODE");
const bool use_proxy = (cache_mode == NULL || strcmp(cache_mode, "local") != 0);