allowed_ips move semantics?

[Jason A. Donenfeld]

Hi Ivan,

You make a lot of compelling points there, and I'm inclined to follow your
advice. The (untested) patch to change this to the behavior you'd like is
actually this trivial two line change:

diff --git a/src/routingtable.c b/src/routingtable.c
index f9c3eff..0ad4d3c 100644
--- a/src/routingtable.c
+++ b/src/routingtable.c
@@ -194,6 +194,8 @@ static int add(struct routing_table_node __rcu **trie,
u8 bits, const u8 *key, u
  return 0;
  }
  if (node_placement(*trie, key, cidr, bits, &node, lock)) {
+ if (node->peer)
+ return -EEXIST;
  node->peer = peer;
  return 0;
  }

I'm especially compelled by your point that this would mirror the semantics
of the routing table.

However, I was a bit confused by your mention of a race condition, because
I believe that my current solution _avoids_ a race condition. The routing
table has a concept of metrics, which allows for avoiding a race condition
when gracefully handing over routes between different interfaces, like this:

# ip route add 10.0.0.0/24 dev eth0 metric 1000
# ip route add 10.0.0.0/24 dev wlan0 metric 1001
# ip route del 10.0.0.0/24 dev eth0 metric 1000

(It also has `ip route change`, but I don't know if this is actually
implemented in a race free way in the kernel or how it works.)

If I added the -EEXIST semantic to WireGuard's allowed-ips, if you wanted
to change traffic from one peer to the other, you could not do so without
dropping a bunch of packets during the interim. Thus, a race condition.
This was the original reason for designing it as I did -- I liked the idea
of race-free handovers.

Do you think maintaining race-free handover is important enough to warrant
not using -EEXIST? I sort of think it is important. Of course, I could add
an explicit "change" flag, but that makes things a bit complicated. And, in
the case of your hypothetical fat-fingered sysadmin, the error condition is
that this "fails closed", rather than "fails open", which probably makes it
not such a big deal.

What do you think of that argumentation?

Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/wireguard/attachments/20170518/831ce500/attachment.html>