From d8474bb1cf7b4be61b2768e92e0ad47687262ace Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Mon, 25 Dec 2023 05:57:48 +0000 Subject: [PATCH] Check target sockets by UID range This commit optimizes the mechanism to examine the TCP sockets based on the VPN UID range. The automaticOnOffKeepalives will be paused/resumed by checking the TCP sockets status on the VPN network. The existing design considers all UIDs, not the UID ranges for the specific VPN network. This may lead to an inaccurate judgement to not to stop the keepalive even when there is no socket on the VPN network. This commit is also a part of follow-up work from aosp/2354202 to use structized class to parse the TCP sockets. Bug: 311119352 Bug: 265877161 Test: atest FrameworksNetTests Change-Id: I4a6b751cb904b9c18ed1df420cc880ea38022d6a --- .../AutomaticOnOffKeepaliveTracker.java | 61 +++++++++---------- .../AutomaticOnOffKeepaliveTrackerTest.java | 12 +++- 2 files changed, 41 insertions(+), 32 deletions(-) diff --git a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java index 8036ae91b5..94ba9de498 100644 --- a/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java +++ b/service/src/com/android/server/connectivity/AutomaticOnOffKeepaliveTracker.java @@ -25,9 +25,6 @@ import static android.system.OsConstants.AF_INET6; import static android.system.OsConstants.SOL_SOCKET; import static android.system.OsConstants.SO_SNDTIMEO; -import static com.android.net.module.util.netlink.NetlinkConstants.NLMSG_DONE; -import static com.android.net.module.util.netlink.NetlinkConstants.SOCKDIAG_MSG_HEADER_SIZE; -import static com.android.net.module.util.netlink.NetlinkConstants.SOCK_DIAG_BY_FAMILY; import static com.android.net.module.util.netlink.NetlinkUtils.IO_TIMEOUT_MS; import android.annotation.IntDef; @@ -90,6 +87,7 @@ import java.util.Set; */ public class AutomaticOnOffKeepaliveTracker { private static final String TAG = "AutomaticOnOffKeepaliveTracker"; + private static final boolean DBG = Log.isLoggable(TAG, Log.DEBUG); private static final int[] ADDRESS_FAMILIES = new int[] {AF_INET6, AF_INET}; private static final long LOW_TCP_POLLING_INTERVAL_MS = 1_000L; private static final int ADJUST_TCP_POLLING_DELAY_MS = 2000; @@ -794,22 +792,18 @@ public class AutomaticOnOffKeepaliveTracker { try { while (NetlinkUtils.enoughBytesRemainForValidNlMsg(bytes)) { - final int startPos = bytes.position(); + // NetlinkMessage.parse() will move the byte buffer position. + // TODO: Parse dst address information to filter socket. + final NetlinkMessage nlMsg = NetlinkMessage.parse( + bytes, OsConstants.NETLINK_INET_DIAG); + if (!(nlMsg instanceof InetDiagMessage)) { + if (DBG) Log.e(TAG, "Not a SOCK_DIAG_BY_FAMILY msg"); + return false; + } - final int nlmsgLen = bytes.getInt(); - final int nlmsgType = bytes.getShort(); - if (isEndOfMessageOrError(nlmsgType)) return false; - // TODO: Parse InetDiagMessage to get uid and dst address information to filter - // socket via NetlinkMessage.parse. - - // Skip the header to move to data part. - bytes.position(startPos + SOCKDIAG_MSG_HEADER_SIZE); - - if (isTargetTcpSocket(bytes, nlmsgLen, networkMark, networkMask)) { - if (Log.isLoggable(TAG, Log.DEBUG)) { - bytes.position(startPos); - final InetDiagMessage diagMsg = (InetDiagMessage) NetlinkMessage.parse( - bytes, OsConstants.NETLINK_INET_DIAG); + final InetDiagMessage diagMsg = (InetDiagMessage) nlMsg; + if (isTargetTcpSocket(diagMsg, networkMark, networkMask, vpnUidRanges)) { + if (DBG) { Log.d(TAG, String.format("Found open TCP connection by uid %d to %s" + " cookie %d", diagMsg.inetDiagMsg.idiag_uid, @@ -834,26 +828,31 @@ public class AutomaticOnOffKeepaliveTracker { return false; } - private boolean isEndOfMessageOrError(int nlmsgType) { - return nlmsgType == NLMSG_DONE || nlmsgType != SOCK_DIAG_BY_FAMILY; + private static boolean containsUid(Set> ranges, int uid) { + for (final Range range: ranges) { + if (range.contains(uid)) { + return true; + } + } + return false; } - private boolean isTargetTcpSocket(@NonNull ByteBuffer bytes, int nlmsgLen, int networkMark, - int networkMask) { - final int mark = readSocketDataAndReturnMark(bytes, nlmsgLen); + private boolean isTargetTcpSocket(@NonNull InetDiagMessage diagMsg, + int networkMark, int networkMask, @NonNull Set> vpnUidRanges) { + if (!containsUid(vpnUidRanges, diagMsg.inetDiagMsg.idiag_uid)) return false; + + final int mark = readSocketDataAndReturnMark(diagMsg); return (mark & networkMask) == networkMark; } - private int readSocketDataAndReturnMark(@NonNull ByteBuffer bytes, int nlmsgLen) { - final int nextMsgOffset = bytes.position() + nlmsgLen - SOCKDIAG_MSG_HEADER_SIZE; + private int readSocketDataAndReturnMark(@NonNull InetDiagMessage diagMsg) { int mark = NetlinkUtils.INIT_MARK_VALUE; // Get socket mark - // TODO: Add a parsing method in NetlinkMessage.parse to support this to skip the remaining - // data. - while (bytes.position() < nextMsgOffset) { - final StructNlAttr nlattr = StructNlAttr.parse(bytes); - if (nlattr != null && nlattr.nla_type == NetlinkUtils.INET_DIAG_MARK) { - mark = nlattr.getValueAsInteger(); + for (StructNlAttr attr : diagMsg.nlAttrs) { + if (attr.nla_type == NetlinkUtils.INET_DIAG_MARK) { + // The netlink attributes should contain only one INET_DIAG_MARK for each socket. + mark = attr.getValueAsInteger(); + break; } } return mark; diff --git a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java index 015b75a10e..4fcf8a8c63 100644 --- a/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java +++ b/tests/unit/java/com/android/server/connectivity/AutomaticOnOffKeepaliveTrackerTest.java @@ -162,7 +162,7 @@ public class AutomaticOnOffKeepaliveTrackerTest { + "00000000" // idiag_expires + "00000000" // idiag_rqueue + "00000000" // idiag_wqueue - + "00000000" // idiag_uid + + "39300000" // idiag_uid = 12345 + "00000000" // idiag_inode // rtattr + "0500" // len = 5 @@ -426,6 +426,16 @@ public class AutomaticOnOffKeepaliveTrackerTest { () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected(TEST_NETID, TEST_UID_RANGES))); } + @Test + public void testIsAnyTcpSocketConnected_noTargetUidSocket() throws Exception { + setupResponseWithSocketExisting(); + // Configured uid(12345) is not in the VPN range. + assertFalse(visibleOnHandlerThread(mTestHandler, + () -> mAOOKeepaliveTracker.isAnyTcpSocketConnected( + TEST_NETID, + new ArraySet<>(Arrays.asList(new Range<>(99999, 99999)))))); + } + @Test public void testIsAnyTcpSocketConnected_withIncorrectNetId() throws Exception { setupResponseWithSocketExisting();