From 1668b2da03da334d930c4c1d3a55b8d986846ae5 Mon Sep 17 00:00:00 2001 From: Hungming Chen Date: Mon, 8 Aug 2022 19:02:58 +0800 Subject: [PATCH] BpfCoordinator: avoid attach/deatach ipv6 program on 464xlat interface 464xlat interface v4-* doesn't have IPv6 address. IPv6 program should not be attached/deatached on v4-* interface. Bug: 241106456 Test: atest BpfCoordinatorTest Test: manual test Use IPv6-only wifi to test because there is no IPv6-only live cellular environment here. 1. Patch TetheringInterfaceUtils.allowIpv6Tethering to allow WIFI. 2. Connect to IPv6-only wifi Google-Guest. 3. Enable USB tethering. 4. Check that v4-wlan0 doesn't have IPv6 program. $ adb shell tc filter show dev wlan0 ingress; filter protocol ipv6 pref 2 bpf chain 0 filter protocol ipv6 pref 2 bpf chain 0 handle 0x1 prog_offload_schedcls_tether_downstream6_ether:[*fsobj] direct-action not_in_hw id 2 tag 7d9be9d02d02e8d8 filter protocol ip pref 3 bpf chain 0 filter protocol ip pref 3 bpf chain 0 handle 0x1 prog_offload_schedcls_tether_downstream4_ether:[*fsobj] direct-action not_in_hw id 8 tag 434b3ecac7968bb3 filter protocol ipv6 pref 4 bpf chain 0 filter protocol ipv6 pref 4 bpf chain 0 handle 0x1 prog_clatd_schedcls_ingress6_clat_ether:[*fsobj] direct-action not_in_hw id 24 tag 06763574e0e1f984 $ adb shell tc filter show dev v4-wlan0 ingress; filter protocol ip pref 3 bpf chain 0 filter protocol ip pref 3 bpf chain 0 handle 0x1 prog_offload_schedcls_tether_downstream4_rawip:[*fsobj] direct-action not_in_hw id 6 tag 9e6e587e0c6cf0bc $ adb shell tc filter show dev v4-wlan0 egress; filter protocol ip pref 4 bpf chain 0 filter protocol ip pref 4 bpf chain 0 handle 0x1 prog_clatd_schedcls_egress4_clat_rawip:[*fsobj] direct-action not_in_hw id 27 tag 9158397a2869247e $ adb shell tc filter show dev ncm0 ingress; filter protocol ipv6 pref 2 bpf chain 0 filter protocol ipv6 pref 2 bpf chain 0 handle 0x1 prog_offload_schedcls_tether_upstream6_ether:[*fsobj] direct-action not_in_hw id 3 tag 5f70a4f42dffe899 filter protocol ip pref 3 bpf chain 0 filter protocol ip pref 3 bpf chain 0 handle 0x1 prog_offload_schedcls_tether_upstream4_ether:[*fsobj] direct-action not_in_hw id 9 tag 26f324113d54c30a Change-Id: Ic1c1cfc7646b4d4004856850b66428e4651d42ab --- .../apishim/api30/BpfCoordinatorShimImpl.java | 4 +- .../apishim/api31/BpfCoordinatorShimImpl.java | 8 +-- .../apishim/common/BpfCoordinatorShim.java | 12 +++- .../tethering/BpfCoordinator.java | 30 ++++++-- .../networkstack/tethering/BpfUtils.java | 72 ++++++++++--------- .../tethering/BpfCoordinatorTest.java | 37 ++++++---- 6 files changed, 105 insertions(+), 58 deletions(-) diff --git a/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java b/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java index 18ef63171d..898b124e07 100644 --- a/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java +++ b/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java @@ -168,13 +168,13 @@ public class BpfCoordinatorShimImpl } @Override - public boolean attachProgram(String iface, boolean downstream) { + public boolean attachProgram(String iface, boolean downstream, boolean ipv4) { /* no op */ return true; } @Override - public boolean detachProgram(String iface) { + public boolean detachProgram(String iface, boolean ipv4) { /* no op */ return true; } diff --git a/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java b/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java index fd9dab5ddf..776832fcf2 100644 --- a/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java +++ b/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java @@ -425,11 +425,11 @@ public class BpfCoordinatorShimImpl } @Override - public boolean attachProgram(String iface, boolean downstream) { + public boolean attachProgram(String iface, boolean downstream, boolean ipv4) { if (!isInitialized()) return false; try { - BpfUtils.attachProgram(iface, downstream); + BpfUtils.attachProgram(iface, downstream, ipv4); } catch (IOException e) { mLog.e("Could not attach program: " + e); return false; @@ -438,11 +438,11 @@ public class BpfCoordinatorShimImpl } @Override - public boolean detachProgram(String iface) { + public boolean detachProgram(String iface, boolean ipv4) { if (!isInitialized()) return false; try { - BpfUtils.detachProgram(iface); + BpfUtils.detachProgram(iface, ipv4); } catch (IOException e) { mLog.e("Could not detach program: " + e); return false; diff --git a/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java b/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java index 69cbab5140..51cecfe889 100644 --- a/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java +++ b/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java @@ -172,16 +172,24 @@ public abstract class BpfCoordinatorShim { /** * Attach BPF program. * + * @param iface the interface name to attach program. + * @param downstream indicate the datapath. true if downstream, false if upstream. + * @param ipv4 indicate the protocol family. true if ipv4, false if ipv6. + * * TODO: consider using InterfaceParams to replace interface name. */ - public abstract boolean attachProgram(@NonNull String iface, boolean downstream); + public abstract boolean attachProgram(@NonNull String iface, boolean downstream, + boolean ipv4); /** * Detach BPF program. * + * @param iface the interface name to detach program. + * @param ipv4 indicate the protocol family. true if ipv4, false if ipv6. + * * TODO: consider using InterfaceParams to replace interface name. */ - public abstract boolean detachProgram(@NonNull String iface); + public abstract boolean detachProgram(@NonNull String iface, boolean ipv4); /** * Add interface index mapping. diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index f8169cbd6b..7e0a589ae2 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -895,6 +895,28 @@ public class BpfCoordinator { } } + private boolean is464XlatInterface(@NonNull String ifaceName) { + return ifaceName.startsWith("v4-"); + } + + private void maybeAttachProgramImpl(@NonNull String iface, boolean downstream) { + mBpfCoordinatorShim.attachProgram(iface, downstream, true /* ipv4 */); + + // Ignore 464xlat interface because it is IPv4 only. + if (!is464XlatInterface(iface)) { + mBpfCoordinatorShim.attachProgram(iface, downstream, false /* ipv4 */); + } + } + + private void maybeDetachProgramImpl(@NonNull String iface) { + mBpfCoordinatorShim.detachProgram(iface, true /* ipv4 */); + + // Ignore 464xlat interface because it is IPv4 only. + if (!is464XlatInterface(iface)) { + mBpfCoordinatorShim.detachProgram(iface, false /* ipv4 */); + } + } + /** * Attach BPF program * @@ -913,11 +935,11 @@ public class BpfCoordinator { // Ex: IPv6 only interface has two forwarding pair, iface and v4-iface, on the // same downstream. if (firstUpstreamForThisDownstream) { - mBpfCoordinatorShim.attachProgram(intIface, UPSTREAM); + maybeAttachProgramImpl(intIface, UPSTREAM); } // Attach if the upstream is the first time to be used in a forwarding pair. if (firstDownstreamForThisUpstream) { - mBpfCoordinatorShim.attachProgram(extIface, DOWNSTREAM); + maybeAttachProgramImpl(extIface, DOWNSTREAM); } } @@ -929,11 +951,11 @@ public class BpfCoordinator { // Detaching program may fail because the interface has been removed already. if (!isAnyForwardingPairOnDownstream(intIface)) { - mBpfCoordinatorShim.detachProgram(intIface); + maybeDetachProgramImpl(intIface); } // Detach if no more forwarding pair is using the upstream. if (!isAnyForwardingPairOnUpstream(extIface)) { - mBpfCoordinatorShim.detachProgram(extIface); + maybeDetachProgramImpl(extIface); } } diff --git a/Tethering/src/com/android/networkstack/tethering/BpfUtils.java b/Tethering/src/com/android/networkstack/tethering/BpfUtils.java index 3d2dfaa8db..12a0c96f0b 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfUtils.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfUtils.java @@ -74,7 +74,7 @@ public class BpfUtils { * * TODO: use interface index to replace interface name. */ - public static void attachProgram(@NonNull String iface, boolean downstream) + public static void attachProgram(@NonNull String iface, boolean downstream, boolean ipv4) throws IOException { final InterfaceParams params = InterfaceParams.getByName(iface); if (params == null) { @@ -88,24 +88,26 @@ public class BpfUtils { throw new IOException("isEthernet(" + params.index + "[" + iface + "]) failure: " + e); } - try { - // tc filter add dev .. ingress prio 1 protocol ipv6 bpf object-pinned /sys/fs/bpf/... - // direct-action - TcUtils.tcFilterAddDevBpf(params.index, INGRESS, PRIO_TETHER6, (short) ETH_P_IPV6, - makeProgPath(downstream, 6, ether)); - } catch (IOException e) { - throw new IOException("tc filter add dev (" + params.index + "[" + iface - + "]) ingress prio PRIO_TETHER6 protocol ipv6 failure: " + e); - } - - try { - // tc filter add dev .. ingress prio 2 protocol ip bpf object-pinned /sys/fs/bpf/... - // direct-action - TcUtils.tcFilterAddDevBpf(params.index, INGRESS, PRIO_TETHER4, (short) ETH_P_IP, - makeProgPath(downstream, 4, ether)); - } catch (IOException e) { - throw new IOException("tc filter add dev (" + params.index + "[" + iface - + "]) ingress prio PRIO_TETHER4 protocol ip failure: " + e); + if (ipv4) { + try { + // tc filter add dev .. ingress prio 2 protocol ip bpf object-pinned /sys/fs/bpf/... + // direct-action + TcUtils.tcFilterAddDevBpf(params.index, INGRESS, PRIO_TETHER4, (short) ETH_P_IP, + makeProgPath(downstream, 4, ether)); + } catch (IOException e) { + throw new IOException("tc filter add dev (" + params.index + "[" + iface + + "]) ingress prio PRIO_TETHER4 protocol ip failure: " + e); + } + } else { + try { + // tc filter add dev .. ingress prio 1 protocol ipv6 bpf object-pinned + // /sys/fs/bpf/... direct-action + TcUtils.tcFilterAddDevBpf(params.index, INGRESS, PRIO_TETHER6, (short) ETH_P_IPV6, + makeProgPath(downstream, 6, ether)); + } catch (IOException e) { + throw new IOException("tc filter add dev (" + params.index + "[" + iface + + "]) ingress prio PRIO_TETHER6 protocol ipv6 failure: " + e); + } } } @@ -114,26 +116,28 @@ public class BpfUtils { * * TODO: use interface index to replace interface name. */ - public static void detachProgram(@NonNull String iface) throws IOException { + public static void detachProgram(@NonNull String iface, boolean ipv4) throws IOException { final InterfaceParams params = InterfaceParams.getByName(iface); if (params == null) { throw new IOException("Fail to get interface params for interface " + iface); } - try { - // tc filter del dev .. ingress prio 1 protocol ipv6 - TcUtils.tcFilterDelDev(params.index, INGRESS, PRIO_TETHER6, (short) ETH_P_IPV6); - } catch (IOException e) { - throw new IOException("tc filter del dev (" + params.index + "[" + iface - + "]) ingress prio PRIO_TETHER6 protocol ipv6 failure: " + e); - } - - try { - // tc filter del dev .. ingress prio 2 protocol ip - TcUtils.tcFilterDelDev(params.index, INGRESS, PRIO_TETHER4, (short) ETH_P_IP); - } catch (IOException e) { - throw new IOException("tc filter del dev (" + params.index + "[" + iface - + "]) ingress prio PRIO_TETHER4 protocol ip failure: " + e); + if (ipv4) { + try { + // tc filter del dev .. ingress prio 2 protocol ip + TcUtils.tcFilterDelDev(params.index, INGRESS, PRIO_TETHER4, (short) ETH_P_IP); + } catch (IOException e) { + throw new IOException("tc filter del dev (" + params.index + "[" + iface + + "]) ingress prio PRIO_TETHER4 protocol ip failure: " + e); + } + } else { + try { + // tc filter del dev .. ingress prio 1 protocol ipv6 + TcUtils.tcFilterDelDev(params.index, INGRESS, PRIO_TETHER6, (short) ETH_P_IPV6); + } catch (IOException e) { + throw new IOException("tc filter del dev (" + params.index + "[" + iface + + "]) ingress prio PRIO_TETHER6 protocol ipv6 failure: " + e); + } } } } 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 788673a5e7..63bb7317da 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java @@ -59,6 +59,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyString; @@ -141,6 +142,9 @@ public class BpfCoordinatorTest { @Rule public final DevSdkIgnoreRule mIgnoreRule = new DevSdkIgnoreRule(); + private static final boolean IPV4 = true; + private static final boolean IPV6 = false; + private static final int TEST_NET_ID = 24; private static final int TEST_NET_ID2 = 25; @@ -1286,8 +1290,10 @@ public class BpfCoordinatorTest { // [1] Add the forwarding pair . Expect that attach both wlan1 and // rmnet_data0. coordinator.maybeAttachProgram(intIface1, extIface1); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface1, DOWNSTREAM)); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface1, UPSTREAM)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface1, DOWNSTREAM, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface1, DOWNSTREAM, IPV6)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface1, UPSTREAM, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface1, UPSTREAM, IPV6)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); @@ -1298,42 +1304,49 @@ public class BpfCoordinatorTest { // [3] Add the forwarding pair . Expect that attach rndis0 only. coordinator.maybeAttachProgram(intIface2, extIface1); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface2, UPSTREAM)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface2, UPSTREAM, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface2, UPSTREAM, IPV6)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); // [4] Add the forwarding pair . Expect that attach - // v4-rmnet_data0 only. + // v4-rmnet_data0 IPv4 program only. coordinator.maybeAttachProgram(intIface2, extIface2); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface2, DOWNSTREAM)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface2, DOWNSTREAM, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface2, DOWNSTREAM, IPV6), + never()); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); // [5] Remove the forwarding pair . Expect detach - // v4-rmnet_data0 only. + // v4-rmnet_data0 IPv4 program only. coordinator.maybeDetachProgram(intIface2, extIface2); - ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface2)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface2, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface2, IPV6), never()); 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.verify(() -> BpfUtils.detachProgram(intIface2, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(intIface2, IPV6)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); // [7] Remove the forwarding pair . Expect that detach both wlan1 // and rmnet_data0. coordinator.maybeDetachProgram(intIface1, extIface1); - ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface1)); - ExtendedMockito.verify(() -> BpfUtils.detachProgram(intIface1)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface1, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(extIface1, IPV6)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(intIface1, IPV4)); + ExtendedMockito.verify(() -> BpfUtils.detachProgram(intIface1, IPV6)); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils); // [8] Skip attaching if upstream is virtual interface. coordinator.maybeAttachProgram(intIface1, virtualIface); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(extIface1, DOWNSTREAM), never()); - ExtendedMockito.verify(() -> BpfUtils.attachProgram(intIface1, UPSTREAM), never()); + ExtendedMockito.verify(() -> + BpfUtils.attachProgram(anyString(), anyBoolean(), anyBoolean()), never()); ExtendedMockito.verifyNoMoreInteractions(mockMarkerBpfUtils); ExtendedMockito.clearInvocations(mockMarkerBpfUtils);