Allow clients on different tethered interfaces to communicate

Previously, the routing rules were too strict. We should probably also deprecate TrafficRecord.upstream sometime.

mobile/src/main/java/be/mygod/vpnhotspot/net/Routing.kt

@@ -4,9 +4,14 @@ import android.os.Build
 import be.mygod.vpnhotspot.App.Companion.app
 import be.mygod.vpnhotspot.R
 import be.mygod.vpnhotspot.net.monitor.FallbackUpstreamMonitor
+import be.mygod.vpnhotspot.net.monitor.IpNeighbourMonitor
 import be.mygod.vpnhotspot.net.monitor.TrafficRecorder
 import be.mygod.vpnhotspot.net.monitor.UpstreamMonitor
+import be.mygod.vpnhotspot.room.AppDatabase
+import be.mygod.vpnhotspot.room.lookup
+import be.mygod.vpnhotspot.room.macToLong
 import be.mygod.vpnhotspot.util.RootSession
+import be.mygod.vpnhotspot.util.computeIfAbsentCompat
 import be.mygod.vpnhotspot.widget.SmartSnackbar
 import timber.log.Timber
 import java.lang.RuntimeException
@@ -18,7 +23,7 @@ import java.util.concurrent.atomic.AtomicLong
  *
  * Once revert is called, this object no longer serves any purpose.
  */
-class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
+class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) : IpNeighbourMonitor.Callback {
     companion object {
         /**
          * Since Android 5.0, RULE_PRIORITY_TETHERING = 18000.
@@ -55,9 +60,9 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
             }
         }
 
-        fun RootSession.Transaction.iptablesAdd(content: String, table: String = "filter") =
+        private fun RootSession.Transaction.iptablesAdd(content: String, table: String = "filter") =
                 exec("$IPTABLES -t $table -A $content", "$IPTABLES -t $table -D $content", true)
-        fun RootSession.Transaction.iptablesInsert(content: String, table: String = "filter") =
+        private fun RootSession.Transaction.iptablesInsert(content: String, table: String = "filter") =
                 exec("$IPTABLES -t $table -I $content", "$IPTABLES -t $table -D $content", true)
     }
 
@@ -74,6 +79,24 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
 
     private val upstreams = HashSet<String>()
     private open inner class Upstream(val priority: Int) : UpstreamMonitor.Callback {
+        /**
+         * The only case when upstream is null is on API 23- and we are using system default rules.
+         */
+        inner class Subrouting(priority: Int, val upstream: String? = null) {
+            val transaction = RootSession.beginTransaction().safeguard {
+                if (upstream != null) {
+                    exec("ip rule add from all iif $downstream lookup $upstream priority $priority",
+                            // by the time stopScript is called, table entry for upstream may already get removed
+                            "ip rule del from all iif $downstream priority $priority")
+                }
+                // note: specifying -i wouldn't work for POSTROUTING
+                if (hasMasquerade) {
+                    iptablesAdd(if (upstream == null) "vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE" else
+                        "vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE", "nat")
+                }
+            }
+        }
+
         var subrouting: Subrouting? = null
         var dns: List<InetAddress> = emptyList()
 
@@ -83,7 +106,7 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
                 subrouting != null -> check(subrouting.upstream == ifname)
                 !upstreams.add(ifname) -> return
                 else -> this.subrouting = try {
-                    Subrouting(this@Routing, priority, ifname)
+                    Subrouting(priority, ifname)
                 } catch (e: Exception) {
                     SmartSnackbar.make(e).show()
                     Timber.w(e)
@@ -98,9 +121,7 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
             val subrouting = subrouting ?: return
             // we could be removing fallback subrouting which no collision could ever happen, check before removing
             if (subrouting.upstream != null) check(upstreams.remove(subrouting.upstream))
-            subrouting.close()
+            subrouting.transaction.revert()
-            TrafficRecorder.update()    // record stats before removing rules to prevent stats losing
-            subrouting.revert()
             this.subrouting = null
             dns = emptyList()
             updateDnsRoute()
@@ -110,7 +131,7 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
         override fun onFallback() = synchronized(this@Routing) {
             check(subrouting == null)
             subrouting = try {
-                Subrouting(this@Routing, priority)
+                Subrouting(priority)
             } catch (e: Exception) {
                 SmartSnackbar.make(e).show()
                 Timber.w(e)
@@ -121,6 +142,47 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
     }
     private val upstream = Upstream(RULE_PRIORITY_UPSTREAM)
 
+    private inner class Client(private val ip: Inet4Address, mac: String) : AutoCloseable {
+        private val transaction = RootSession.beginTransaction().safeguard {
+            val address = ip.hostAddress
+            iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -j ACCEPT")
+            iptablesInsert("vpnhotspot_fwd -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
+        }
+
+        init {
+            try {
+                TrafficRecorder.register(ip, downstream, mac)
+            } catch (e: Exception) {
+                close()
+                throw e
+            }
+        }
+
+        override fun close() {
+            TrafficRecorder.unregister(ip, downstream)
+            transaction.revert()
+        }
+    }
+    private val clients = HashMap<InetAddress, Client>()
+    override fun onIpNeighbourAvailable(neighbours: List<IpNeighbour>) = synchronized(this) {
+        val toRemove = HashSet(clients.keys)
+        for (neighbour in neighbours) {
+            if (neighbour.dev != downstream || neighbour.ip !is Inet4Address ||
+                    AppDatabase.instance.clientRecordDao.lookup(neighbour.lladdr.macToLong()).blocked) continue
+            toRemove.remove(neighbour.ip)
+            try {
+                clients.computeIfAbsentCompat(neighbour.ip) { Client(neighbour.ip, neighbour.lladdr) }
+            } catch (e: Exception) {
+                Timber.w(e)
+                SmartSnackbar.make(e).show()
+            }
+        }
+        if (toRemove.isNotEmpty()) {
+            TrafficRecorder.update()    // record stats before removing rules to prevent stats losing
+            for (address in toRemove) clients.remove(address)!!.close()
+        }
+    }
+
     fun ipForward() = transaction.exec("echo 1 >/proc/sys/net/ipv4/ip_forward")
 
     fun disableIpv6() = transaction.exec("echo 1 >/proc/sys/net/ipv6/conf/$downstream/disable_ipv6",
@@ -181,22 +243,23 @@ class Routing(val downstream: String, ownerAddress: InterfaceAddress? = null) {
     }
 
     fun stop() {
+        IpNeighbourMonitor.unregisterCallback(this)
         FallbackUpstreamMonitor.unregisterCallback(fallbackUpstream)
-        fallbackUpstream.subrouting?.close()
         UpstreamMonitor.unregisterCallback(upstream)
-        upstream.subrouting?.close()
     }
 
     fun commit() {
         transaction.commit()
         FallbackUpstreamMonitor.registerCallback(fallbackUpstream)
         UpstreamMonitor.registerCallback(upstream)
+        IpNeighbourMonitor.registerCallback(this)
     }
     fun revert() {
         stop()
         TrafficRecorder.update()    // record stats before exiting to prevent stats losing
-        fallbackUpstream.subrouting?.revert()
+        clients.forEach { (_, subroute) -> subroute.close() }
-        upstream.subrouting?.revert()
+        fallbackUpstream.subrouting?.transaction?.revert()
+        upstream.subrouting?.transaction?.revert()
         transaction.revert()
     }
 }

mobile/src/main/java/be/mygod/vpnhotspot/net/Subrouting.kt

@@ -1,110 +0,0 @@
-package be.mygod.vpnhotspot.net
-
-import be.mygod.vpnhotspot.net.Routing.Companion.iptablesAdd
-import be.mygod.vpnhotspot.net.Routing.Companion.iptablesInsert
-import be.mygod.vpnhotspot.net.monitor.IpNeighbourMonitor
-import be.mygod.vpnhotspot.net.monitor.TrafficRecorder
-import be.mygod.vpnhotspot.room.AppDatabase
-import be.mygod.vpnhotspot.room.lookup
-import be.mygod.vpnhotspot.room.macToLong
-import be.mygod.vpnhotspot.util.RootSession
-import be.mygod.vpnhotspot.util.computeIfAbsentCompat
-import be.mygod.vpnhotspot.widget.SmartSnackbar
-import timber.log.Timber
-import java.net.Inet4Address
-import java.net.InetAddress
-
-/**
- * The only case when upstream is null is on API 23- and we are using system default rules.
- */
-class Subrouting(private val parent: Routing, priority: Int, val upstream: String? = null) :
-        IpNeighbourMonitor.Callback, AutoCloseable {
-    private inner class Subroute(private val ip: Inet4Address, mac: String) : AutoCloseable {
-        private val transaction = RootSession.beginTransaction().safeguard {
-            val downstream = parent.downstream
-            val address = ip.hostAddress
-            if (upstream == null) {
-                // otw allow downstream packets to be redirected to anywhere
-                // because we don't wanna keep track of default network changes
-                iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -j ACCEPT")
-                iptablesInsert("vpnhotspot_fwd -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
-            } else {
-                iptablesInsert("vpnhotspot_fwd -i $downstream -s $address -o $upstream -j ACCEPT")
-                iptablesInsert("vpnhotspot_fwd -i $upstream -o $downstream -d $address -m state --state ESTABLISHED,RELATED -j ACCEPT")
-            }
-        }
-
-        init {
-            try {
-                TrafficRecorder.register(ip, upstream, parent.downstream, mac)
-            } catch (e: Exception) {
-                close()
-                throw e
-            }
-        }
-
-        override fun close() {
-            TrafficRecorder.unregister(ip, upstream, parent.downstream)
-            transaction.revert()
-        }
-    }
-
-    private val transaction = RootSession.beginTransaction().safeguard {
-        if (upstream != null) {
-            val downstream = parent.downstream
-            exec("ip rule add from all iif $downstream lookup $upstream priority $priority",
-                    // by the time stopScript is called, table entry for upstream may already get removed
-                    "ip rule del from all iif $downstream priority $priority")
-        }
-        // note: specifying -i wouldn't work for POSTROUTING
-        if (parent.hasMasquerade) {
-            val hostSubnet = parent.hostSubnet
-            iptablesAdd(if (upstream == null) "vpnhotspot_masquerade -s $hostSubnet -j MASQUERADE" else
-                "vpnhotspot_masquerade -s $hostSubnet -o $upstream -j MASQUERADE", "nat")
-        }
-    }
-    private val subroutes = HashMap<InetAddress, Subroute>()
-
-    init {
-        Timber.d("Subrouting initialized from %s to %s", parent.downstream, upstream)
-        try {
-            IpNeighbourMonitor.registerCallback(this)
-        } catch (e: Exception) {
-            close()
-            revert()
-            throw e
-        }
-    }
-
-    /**
-     * Unregister client listener. This should be always called even after clean.
-     */
-    override fun close() {
-        IpNeighbourMonitor.unregisterCallback(this)
-        Timber.d("Subrouting closed from %s to %s", parent.downstream, upstream)
-    }
-
-    override fun onIpNeighbourAvailable(neighbours: List<IpNeighbour>) = synchronized(parent) {
-        val toRemove = HashSet(subroutes.keys)
-        for (neighbour in neighbours) {
-            if (neighbour.dev != parent.downstream || neighbour.ip !is Inet4Address ||
-                    AppDatabase.instance.clientRecordDao.lookup(neighbour.lladdr.macToLong()).blocked) continue
-            toRemove.remove(neighbour.ip)
-            try {
-                subroutes.computeIfAbsentCompat(neighbour.ip) { Subroute(neighbour.ip, neighbour.lladdr) }
-            } catch (e: Exception) {
-                Timber.w(e)
-                SmartSnackbar.make(e).show()
-            }
-        }
-        if (toRemove.isNotEmpty()) {
-            TrafficRecorder.update()    // record stats before removing rules to prevent stats losing
-            for (address in toRemove) subroutes.remove(address)!!.close()
-        }
-    }
-
-    fun revert() {
-        subroutes.forEach { (_, subroute) -> subroute.close() }
-        transaction.revert()
-    }
-}

mobile/src/main/java/be/mygod/vpnhotspot/net/monitor/TrafficRecorder.kt

@@ -22,27 +22,25 @@ object TrafficRecorder {
 
     private var scheduled = false
     private var lastUpdate = 0L
-    private val records = HashMap<Triple<InetAddress, String?, String>, TrafficRecord>()
+    private val records = HashMap<Pair<InetAddress, String>, TrafficRecord>()
     val foregroundListeners = Event2<Collection<TrafficRecord>, LongSparseArray<TrafficRecord>>()
 
-    fun register(ip: InetAddress, upstream: String?, downstream: String, mac: String) {
+    fun register(ip: InetAddress, downstream: String, mac: String) {
         val record = TrafficRecord(
                 mac = mac.macToLong(),
                 ip = ip,
-                upstream = upstream,
                 downstream = downstream)
         AppDatabase.instance.trafficRecordDao.insert(record)
         synchronized(this) {
-            DebugHelper.log(TAG, "Registering ($ip, $upstream, $downstream)")
+            DebugHelper.log(TAG, "Registering $ip%$downstream")
-            check(records.put(Triple(ip, upstream, downstream), record) == null)
+            check(records.put(Pair(ip, downstream), record) == null)
             scheduleUpdateLocked()
         }
     }
-    fun unregister(ip: InetAddress, upstream: String?, downstream: String) = synchronized(this) {
+    fun unregister(ip: InetAddress, downstream: String) = synchronized(this) {
         update()    // flush stats before removing
-        DebugHelper.log(TAG, "Unregistering ($ip, $upstream, $downstream)")
+        DebugHelper.log(TAG, "Unregistering $ip%$downstream")
-        if (records.remove(Triple(ip, upstream, downstream)) == null) Timber.w(
+        if (records.remove(Pair(ip, downstream)) == null) Timber.w("Failed to find traffic record for $ip%$downstream.")
-                "Failed to find traffic record for ($ip, $downstream, $upstream).")
     }
 
     private fun unscheduleUpdateLocked() {
@@ -78,15 +76,12 @@ object TrafficRecorder {
                         check(isReceive != isSend)  // this check might fail when the user performed an upgrade from 1.x
                         val ip = parseNumericAddress(columns[if (isReceive) 8 else 7])
                         val downstream = columns[if (isReceive) 6 else 5]
-                        var upstream: String? = columns[if (isReceive) 5 else 6]
+                        val key = Pair(ip, downstream)
-                        if (upstream == "*") upstream = null
-                        val key = Triple(ip, upstream, downstream)
                         val oldRecord = records[key] ?: continue@loop   // assuming they're legacy old rules
                         val record = if (oldRecord.id == null) oldRecord else TrafficRecord(
                                 timestamp = timestamp,
                                 mac = oldRecord.mac,
                                 ip = ip,
-                                upstream = upstream,
                                 downstream = downstream,
                                 sentPackets = -1,
                                 sentBytes = -1,

mobile/src/main/java/be/mygod/vpnhotspot/room/TrafficRecord.kt

@@ -27,6 +27,7 @@ data class TrafficRecord(
          * For now only stats for IPv4 will be recorded. But I'm going to put the more general class here just in case.
          */
         val ip: InetAddress,
+        @Deprecated("This field is no longer used.")
         val upstream: String? = null,
         val downstream: String,
         var sentPackets: Long = 0,

mobile/src/main/res/values-zh-rCN/strings.xml

@@ -66,7 +66,7 @@
     <string name="clients_nickname_title">%s 的昵称</string>
     <string name="clients_stats_title">%s 的流量</string>
     <plurals name="clients_stats_message_1">
-        <item quantity="other">自 %2$s 以来重新路由了 %1$s 次</item>
+        <item quantity="other">自 %2$s 以来连接了 %1$s 次</item>
     </plurals>
     <plurals name="clients_stats_message_2">
         <item quantity="other">上传 %1$s 个包，%2$s</item>

mobile/src/main/res/values/strings.xml

@@ -70,8 +70,8 @@
     <string name="clients_nickname_title">Nickname for %s</string>
     <string name="clients_stats_title">Stats for %s</string>
     <plurals name="clients_stats_message_1">
-        <item quantity="one">Rerouted 1 time since %2$s</item>
+        <item quantity="one">Connected 1 time since %2$s</item>
-        <item quantity="other">Rerouted %1$s times since %2$s</item>
+        <item quantity="other">Connected %1$s times since %2$s</item>
     </plurals>
     <plurals name="clients_stats_message_2">
         <item quantity="one">Sent 1 packet, %2$s</item>