From c8ad14b0e8b1be6595042298baaca361123efe38 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 21 Feb 2020 20:21:14 +0900 Subject: [PATCH 1/2] Clear IPv6 forwarding rules when losing upstream or stopping. Test: new unit test Change-Id: I8626932e43e0daa300dad5fe6a81f47a6d667030 --- .../src/android/net/ip/IpServer.java | 21 +++++++-- .../unit/src/android/net/ip/IpServerTest.java | 47 +++++++++++++++++++ 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/packages/Tethering/src/android/net/ip/IpServer.java b/packages/Tethering/src/android/net/ip/IpServer.java index 0d369dcff8789..89670292922f6 100644 --- a/packages/Tethering/src/android/net/ip/IpServer.java +++ b/packages/Tethering/src/android/net/ip/IpServer.java @@ -805,7 +805,7 @@ public class IpServer extends StateMachine { rule.dstMac.toByteArray()); mIpv6ForwardingRules.put(rule.address, rule); } catch (RemoteException | ServiceSpecificException e) { - Log.e(TAG, "Could not add IPv6 downstream rule: " + e); + mLog.e("Could not add IPv6 downstream rule: ", e); } } @@ -816,10 +816,17 @@ public class IpServer extends StateMachine { mIpv6ForwardingRules.remove(rule.address); } } catch (RemoteException | ServiceSpecificException e) { - Log.e(TAG, "Could not remove IPv6 downstream rule: " + e); + mLog.e("Could not remove IPv6 downstream rule: ", e); } } + private void clearIpv6ForwardingRules() { + for (Ipv6ForwardingRule rule : mIpv6ForwardingRules.values()) { + removeIpv6ForwardingRule(rule, false /*removeFromMap*/); + } + mIpv6ForwardingRules.clear(); + } + // Convenience method to replace a rule with the same rule on a new upstream interface. // Allows replacing the rules in one iteration pass without ConcurrentModificationExceptions. // Relies on the fact that rules are in a map indexed by IP address. @@ -832,6 +839,12 @@ public class IpServer extends StateMachine { // changes or if a neighbor event is received. private void updateIpv6ForwardingRules(int prevUpstreamIfindex, int upstreamIfindex, NeighborEvent e) { + // If we no longer have an upstream, clear forwarding rules and do nothing else. + if (upstreamIfindex == 0) { + clearIpv6ForwardingRules(); + return; + } + // If the upstream interface has changed, remove all rules and re-add them with the new // upstream interface. if (prevUpstreamIfindex != upstreamIfindex) { @@ -841,13 +854,14 @@ public class IpServer extends StateMachine { } // If we're here to process a NeighborEvent, do so now. + // mInterfaceParams must be non-null or the event would not have arrived. if (e == null) return; if (!(e.ip instanceof Inet6Address) || e.ip.isMulticastAddress() || e.ip.isLoopbackAddress() || e.ip.isLinkLocalAddress()) { return; } - Ipv6ForwardingRule rule = new Ipv6ForwardingRule(mLastIPv6UpstreamIfindex, + Ipv6ForwardingRule rule = new Ipv6ForwardingRule(upstreamIfindex, mInterfaceParams.index, (Inet6Address) e.ip, mInterfaceParams.macAddr, e.macAddr); if (e.isValid()) { @@ -1090,6 +1104,7 @@ public class IpServer extends StateMachine { for (String ifname : mUpstreamIfaceSet.ifnames) cleanupUpstreamInterface(ifname); mUpstreamIfaceSet = null; + clearIpv6ForwardingRules(); } private void cleanupUpstreamInterface(String upstreamIface) { diff --git a/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index 39742f5bd4f65..5b4cdb42cbb5a 100644 --- a/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -43,6 +43,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doAnswer; @@ -578,6 +579,52 @@ public class IpServerTest { eq(neighB.getAddress()), eq(myMac.toByteArray()), eq(macB.toByteArray())); inOrder.verify(mNetd).tetherRuleRemoveDownstreamIpv6(eq(UPSTREAM_IFINDEX), eq(neighB.getAddress())); + reset(mNetd); + + // When the upstream is lost, rules are removed. + dispatchTetherConnectionChanged(null, null); + verify(mNetd).tetherRuleRemoveDownstreamIpv6(eq(UPSTREAM_IFINDEX2), + eq(neighA.getAddress())); + verify(mNetd).tetherRuleRemoveDownstreamIpv6(eq(UPSTREAM_IFINDEX2), + eq(neighB.getAddress())); + reset(mNetd); + + // If the upstream is IPv4-only, no rules are added. + dispatchTetherConnectionChanged(UPSTREAM_IFACE); + reset(mNetd); + recvNewNeigh(myIfindex, neighA, NUD_REACHABLE, macA); + verifyNoMoreInteractions(mNetd); + + // Rules can be added again once upstream IPv6 connectivity is available. + lp.setInterfaceName(UPSTREAM_IFACE); + dispatchTetherConnectionChanged(UPSTREAM_IFACE, lp); + recvNewNeigh(myIfindex, neighB, NUD_REACHABLE, macB); + verify(mNetd).tetherRuleAddDownstreamIpv6(eq(myIfindex), eq(UPSTREAM_IFINDEX), + eq(neighB.getAddress()), eq(myMac.toByteArray()), eq(macB.toByteArray())); + verify(mNetd, never()).tetherRuleAddDownstreamIpv6(anyInt(), anyInt(), + eq(neighA.getAddress()), any(), any()); + + // If upstream IPv6 connectivity is lost, rules are removed. + reset(mNetd); + dispatchTetherConnectionChanged(UPSTREAM_IFACE, null); + verify(mNetd).tetherRuleRemoveDownstreamIpv6(eq(UPSTREAM_IFINDEX), eq(neighB.getAddress())); + + // When the interface goes down, rules are removed. + lp.setInterfaceName(UPSTREAM_IFACE); + dispatchTetherConnectionChanged(UPSTREAM_IFACE, lp); + recvNewNeigh(myIfindex, neighA, NUD_REACHABLE, macA); + recvNewNeigh(myIfindex, neighB, NUD_REACHABLE, macB); + verify(mNetd).tetherRuleAddDownstreamIpv6(eq(myIfindex), eq(UPSTREAM_IFINDEX), + eq(neighA.getAddress()), eq(myMac.toByteArray()), eq(macA.toByteArray())); + verify(mNetd).tetherRuleAddDownstreamIpv6(eq(myIfindex), eq(UPSTREAM_IFINDEX), + eq(neighB.getAddress()), eq(myMac.toByteArray()), eq(macB.toByteArray())); + reset(mNetd); + + mIpServer.stop(); + mLooper.dispatchAll(); + verify(mNetd).tetherRuleRemoveDownstreamIpv6(eq(UPSTREAM_IFINDEX), eq(neighA.getAddress())); + verify(mNetd).tetherRuleRemoveDownstreamIpv6(eq(UPSTREAM_IFINDEX), eq(neighB.getAddress())); + reset(mNetd); } private void assertDhcpStarted(IpPrefix expectedPrefix) throws Exception { From 580440d9c1abf612c103eeb9f0212f0254739bc5 Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Sun, 23 Feb 2020 19:21:49 +0900 Subject: [PATCH 2/2] Fix a logic error in IpServerTest#addRemoveipv6ForwardingRules When checking that link-local and multicast neighbours are ignored, make sure the test neighbours are added on the correct interface. Otherwise, they might be ignored because events on the wrong interface are ignored, and not necessarily because link-local and multicast neighbours are ignored. Test: atest TetheringTests Change-Id: I4a624ea4ce9ee9a9352afccbc7bf866587d4cdfa --- .../Tethering/tests/unit/src/android/net/ip/IpServerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java index 5b4cdb42cbb5a..948266d3cf502 100644 --- a/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java +++ b/packages/Tethering/tests/unit/src/android/net/ip/IpServerTest.java @@ -547,9 +547,9 @@ public class IpServerTest { reset(mNetd); // Link-local and multicast neighbors are ignored. - recvNewNeigh(notMyIfindex, neighLL, NUD_REACHABLE, macA); + recvNewNeigh(myIfindex, neighLL, NUD_REACHABLE, macA); verifyNoMoreInteractions(mNetd); - recvNewNeigh(notMyIfindex, neighMC, NUD_REACHABLE, macA); + recvNewNeigh(myIfindex, neighMC, NUD_REACHABLE, macA); verifyNoMoreInteractions(mNetd); // A neighbor that is no longer valid causes the rule to be removed.