From 6f5439134016e5ace46d4577212d5b54b7f80178 Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Thu, 4 Aug 2022 12:51:53 +0800 Subject: [PATCH] BpfCoordinator: avoid attach/deatach duplicate downstream program The forwarding pairs may use the same downstream. Ex: 464xlat tethering has rmnet0:ncm0, v4-rmnet0:ncm0 forwarding pairs. Need to avoid duplicate downstream program. Bug: 241106456 Test: atest BpfCoordinatorTest Change-Id: Ic41b3ad856e4876808f8497a760081afcfd04988 --- .../tethering/BpfCoordinator.java | 19 ++++++++- .../tethering/BpfCoordinatorTest.java | 39 +++++++++++++------ 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index 49442a6800..f8169cbd6b 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -905,10 +905,16 @@ public class BpfCoordinator { if (forwardingPairExists(intIface, extIface)) return; + boolean firstUpstreamForThisDownstream = !isAnyForwardingPairOnDownstream(intIface); boolean firstDownstreamForThisUpstream = !isAnyForwardingPairOnUpstream(extIface); forwardingPairAdd(intIface, extIface); - mBpfCoordinatorShim.attachProgram(intIface, UPSTREAM); + // Attach if the downstream is the first time to be used in a forwarding pair. + // Ex: IPv6 only interface has two forwarding pair, iface and v4-iface, on the + // same downstream. + if (firstUpstreamForThisDownstream) { + mBpfCoordinatorShim.attachProgram(intIface, UPSTREAM); + } // Attach if the upstream is the first time to be used in a forwarding pair. if (firstDownstreamForThisUpstream) { mBpfCoordinatorShim.attachProgram(extIface, DOWNSTREAM); @@ -922,7 +928,9 @@ public class BpfCoordinator { forwardingPairRemove(intIface, extIface); // Detaching program may fail because the interface has been removed already. - mBpfCoordinatorShim.detachProgram(intIface); + if (!isAnyForwardingPairOnDownstream(intIface)) { + mBpfCoordinatorShim.detachProgram(intIface); + } // Detach if no more forwarding pair is using the upstream. if (!isAnyForwardingPairOnUpstream(extIface)) { mBpfCoordinatorShim.detachProgram(extIface); @@ -1827,6 +1835,13 @@ public class BpfCoordinator { return mForwardingPairs.containsKey(extIface); } + private boolean isAnyForwardingPairOnDownstream(@NonNull String intIface) { + for (final HashSet downstreams : mForwardingPairs.values()) { + if (downstreams.contains(intIface)) return true; + } + return false; + } + @NonNull private NetworkStats buildNetworkStats(@NonNull StatsType type, int ifIndex, @NonNull final ForwardedStats diff) { diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java index fa1d881e79..788673a5e7 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java @@ -1277,47 +1277,62 @@ public class BpfCoordinatorTest { try { final String intIface1 = "wlan1"; final String intIface2 = "rndis0"; - final String extIface = "rmnet_data0"; + final String extIface1 = "rmnet_data0"; + final String extIface2 = "v4-rmnet_data0"; final String virtualIface = "ipsec0"; final BpfUtils mockMarkerBpfUtils = staticMockMarker(BpfUtils.class); final BpfCoordinator coordinator = makeBpfCoordinator(); // [1] Add the forwarding pair . Expect that attach both wlan1 and // rmnet_data0. - coordinator.maybeAttachProgram(intIface1, extIface); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface, DOWNSTREAM)); + coordinator.maybeAttachProgram(intIface1, extIface1); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface1, DOWNSTREAM)); ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface1, UPSTREAM)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); // [2] Add the forwarding pair again. Expect no more action. - coordinator.maybeAttachProgram(intIface1, extIface); + coordinator.maybeAttachProgram(intIface1, extIface1); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); // [3] Add the forwarding pair . Expect that attach rndis0 only. - coordinator.maybeAttachProgram(intIface2, extIface); + coordinator.maybeAttachProgram(intIface2, extIface1); ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface2, UPSTREAM)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); - // [4] Remove the forwarding pair . Expect detach rndis0 only. - coordinator.maybeDetachProgram(intIface2, extIface); + // [4] Add the forwarding pair . Expect that attach + // v4-rmnet_data0 only. + coordinator.maybeAttachProgram(intIface2, extIface2); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface2, DOWNSTREAM)); + ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); + ExtendedMockito.clearInvocations(mockMarkerBpfUtils); + + // [5] Remove the forwarding pair . Expect detach + // v4-rmnet_data0 only. + coordinator.maybeDetachProgram(intIface2, extIface2); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface2)); + ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); + ExtendedMockito.clearInvocations(mockMarkerBpfUtils); + + // [6] Remove the forwarding pair . Expect detach rndis0 only. + coordinator.maybeDetachProgram(intIface2, extIface1); ExtendedMockito.verify(() -> BpfUtils.detachProgram(intIface2)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); - // [5] Remove the forwarding pair . Expect that detach both wlan1 + // [7] Remove the forwarding pair . Expect that detach both wlan1 // and rmnet_data0. - coordinator.maybeDetachProgram(intIface1, extIface); - ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface)); + coordinator.maybeDetachProgram(intIface1, extIface1); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface1)); ExtendedMockito.verify(() -> BpfUtils.detachProgram(intIface1)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); - // [6] Skip attaching if upstream is virtual interface. + // [8] Skip attaching if upstream is virtual interface. coordinator.maybeAttachProgram(intIface1, virtualIface); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface, DOWNSTREAM), never()); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface1, DOWNSTREAM), never()); ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface1, UPSTREAM), never()); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils);