| Message ID | 20191103053148.18059-1-tom.ty89@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel] tun/iproute2: support 31-bit ipv4 prefix (RFC 3021) | expand |
Hi, On Sun, Nov 03, 2019 at 01:31:48PM +0800, Tom Yan wrote: > src/openvpn/tun.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) I tend to NAK this, on a number of reasons - we support arbitrary point-to-point links "since ever" if you do "topology p2p" (can be out of the same /31, or just arbitrary addresses on both ends), so I do not see why doing this in "topology subnet" would be beneficial. OTOH, *if* you do it, it needs to be consistently, not just "linux/iproute" (on linux, "sitnl" is the new gold standard), and the commit message should explain a bit more about the "why do it this way" and "how do other platforms deal with it" (windows, *BSD, MacOS, etc.) gert
Hi, On Sun, Nov 03, 2019 at 07:24:43PM +0100, Gert Doering wrote: > I tend to NAK this, on a number of reasons - we support arbitrary > point-to-point links "since ever" if you do "topology p2p" (can be > out of the same /31, or just arbitrary addresses on both ends), so > I do not see why doing this in "topology subnet" would be beneficial. I do see where you're coming from - the man page talks about depreciating p2p, in which case you need to make sure topology subnet does /31s. It might certainly be a useful excercise to investigate our current ifconfig (etc) calls - in the 2.5 branch - and possibly get rid of all the "broadcast" settings, across all platforms that do not need them. Personally I've never understood why people are so keen on explicitly configuring broadcast addresses everywhere (like in the network config files, etc.) - the standard address can be computed and "just works" (and on a tun interface, there are no link-layer broadcasts anyway, even if we pretend it were differently). The code was that way when David and I inherited the project, so I can't explain *why* it is - but this might be the opportunity to kick out a bit of needless garbage. Out of curiosity: does the sitnl code path handle /31s? gert
While the commit message says "support" 31-bit prefix, this patch is a bug fix by nature. Whether one can actually uses a /31 subnet for *anything* (i.e. not just OpenVPN) pretty much depends entirely on the platform itself. This patch is needed simply because broadcast address does not "apply" in a /31 subnet, and having it set *prevents* it from working. In fact, if openvpn tries to set the broadcast address with `+` instead of an explicit address calculated by itself, `ip` can handle it well. I do it with a prefix length check because I am not sure if there's a reason that `broadcast +` wasn't used instead. Yeah the removal of the p2p topology was one of the reasons. It's actually the fact that ics-openvpn doesn't really parse point-to-point ifconfig that made me aware of this. I don't know anything about sitnl. Is it available only in 2.5/master? While I have also sent the equivalent fix for that, it's merely a "forwardport". I haven't actually used 2.5/master at all (unless ics-openvpn counts, while I don't see broadcast being set for any case on Android). On Mon, 4 Nov 2019 at 02:33, Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Sun, Nov 03, 2019 at 07:24:43PM +0100, Gert Doering wrote: > > I tend to NAK this, on a number of reasons - we support arbitrary > > point-to-point links "since ever" if you do "topology p2p" (can be > > out of the same /31, or just arbitrary addresses on both ends), so > > I do not see why doing this in "topology subnet" would be beneficial. > > I do see where you're coming from - the man page talks about depreciating > p2p, in which case you need to make sure topology subnet does /31s. > > It might certainly be a useful excercise to investigate our current > ifconfig (etc) calls - in the 2.5 branch - and possibly get rid of all > the "broadcast" settings, across all platforms that do not need them. > > Personally I've never understood why people are so keen on explicitly > configuring broadcast addresses everywhere (like in the network config > files, etc.) - the standard address can be computed and "just works" > (and on a tun interface, there are no link-layer broadcasts anyway, even > if we pretend it were differently). > > The code was that way when David and I inherited the project, so I can't > explain *why* it is - but this might be the opportunity to kick out a bit > of needless garbage. > > > Out of curiosity: does the sitnl code path handle /31s? > > gert > > > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de
Hi, On Mon, Nov 04, 2019 at 03:54:49AM +0800, Tom Yan wrote: > While the commit message says "support" 31-bit prefix, this patch is a > bug fix by nature. Whether one can actually uses a /31 subnet for > *anything* (i.e. not just OpenVPN) pretty much depends entirely on the > platform itself. This patch is needed simply because broadcast address > does not "apply" in a /31 subnet, and having it set *prevents* it from > working. Understood (thought it would be more helpful if the commit message said so :-) ). > In fact, if openvpn tries to set the broadcast address with > `+` instead of an explicit address calculated by itself, `ip` can > handle it well. I do it with a prefix length check because I am not > sure if there's a reason that `broadcast +` wasn't used instead. I wasn't aware that there is a "broadcast +" setting, but I'm way more tempted to just get rid of setting broadcast at all. This is a computer, it can do the math itself. > Yeah the removal of the p2p topology was one of the reasons. It's > actually the fact that ics-openvpn doesn't really parse point-to-point > ifconfig that made me aware of this. I wasn't aware of that, but since Arne is reading here, maybe he can comment on it. It's possible that the Android VPN API just does not permit p2p mode, but wants a subnet. Arne, any idea if /31 works on Android? > I don't know anything about sitnl. Is it available only in 2.5/master? > While I have also sent the equivalent fix for that, it's merely a > "forwardport". I haven't actually used 2.5/master at all (unless > ics-openvpn counts, while I don't see broadcast being set for any case > on Android). Yeah, master. New functionality always has to go into master, and 2.4 backporting is only done under certain conditions (undisputed bugfixes, long-term compatibility changes). Adding conditionals is not a bugfix. Removing the "broadcast" part from "ip add" could be considered as such... it needs testing on somewhat ancient Linux systems (I think we aim to support all RHEL/CentOS releases that are still supported, which will definitely cover "OLD!!"). But seriously I expect this to be totally unnecesary historic cruft unless told otherwise. gert
So in master, when net_addr_v4_add is called in tun.c, which net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is actually used depends on how openvpn is built, right? I think the `if (broadcast)` condition in the one of networking_sitnl.c needs to be changed to `if (broadcast && prefixlen < 31)` as well. Also it seems that net_addr_v4_add is called wrongly in tun.c with `&tt->remote_netmask` instead of `&tt->broadcast` as the last argument. I don't really mind it being fixed with any of the approaches: no broadcast, `broadcast +`, or prefix length check. Don't know if it counts as a reference to you but I can even see in the code of systemd-networkd similar prefix length check. As I have no idea whether under any circumstances the broadcast address matters, the prefix length check seems to be a more conservative approach to me and that's why I chose it. (Also, the "conditional" added merely causes "remove-broadcast-the-bugfix" to apply only on where it is strictly necessary...) As for `broadcast +`, I am not sure if there's an equivalence for sitnl, and I wonder if it raises version requirement on iproute2. For the record, /31 subnet works fine on Linux and Android as long as the patch is applied (for Linux). (And again, from OpenVPN point of view, we don't need to care if /31 subnet works on any platform, as we never prevented users from setting that up. Well, other than the silly code in the server directive, which should be fixed in a follow-up; but it's not like users have to use that anyway.) On Mon, 4 Nov 2019 at 04:46, Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Mon, Nov 04, 2019 at 03:54:49AM +0800, Tom Yan wrote: > > While the commit message says "support" 31-bit prefix, this patch is a > > bug fix by nature. Whether one can actually uses a /31 subnet for > > *anything* (i.e. not just OpenVPN) pretty much depends entirely on the > > platform itself. This patch is needed simply because broadcast address > > does not "apply" in a /31 subnet, and having it set *prevents* it from > > working. > > Understood (thought it would be more helpful if the commit message > said so :-) ). > > > In fact, if openvpn tries to set the broadcast address with > > `+` instead of an explicit address calculated by itself, `ip` can > > handle it well. I do it with a prefix length check because I am not > > sure if there's a reason that `broadcast +` wasn't used instead. > > I wasn't aware that there is a "broadcast +" setting, but I'm way > more tempted to just get rid of setting broadcast at all. This is a > computer, it can do the math itself. > > > Yeah the removal of the p2p topology was one of the reasons. It's > > actually the fact that ics-openvpn doesn't really parse point-to-point > > ifconfig that made me aware of this. > > I wasn't aware of that, but since Arne is reading here, maybe he can > comment on it. It's possible that the Android VPN API just does not > permit p2p mode, but wants a subnet. > > Arne, any idea if /31 works on Android? > > > > I don't know anything about sitnl. Is it available only in 2.5/master? > > While I have also sent the equivalent fix for that, it's merely a > > "forwardport". I haven't actually used 2.5/master at all (unless > > ics-openvpn counts, while I don't see broadcast being set for any case > > on Android). > > Yeah, master. New functionality always has to go into master, and > 2.4 backporting is only done under certain conditions (undisputed > bugfixes, long-term compatibility changes). > > Adding conditionals is not a bugfix. Removing the "broadcast" part > from "ip add" could be considered as such... it needs testing on > somewhat ancient Linux systems (I think we aim to support all > RHEL/CentOS releases that are still supported, which will definitely > cover "OLD!!"). But seriously I expect this to be totally unnecesary > historic cruft unless told otherwise. > > gert > > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de
>> Yeah the removal of the p2p topology was one of the reasons. It's >> actually the fact that ics-openvpn doesn't really parse point-to-point >> ifconfig that made me aware of this. > > I wasn't aware of that, but since Arne is reading here, maybe he can > comment on it. It's possible that the Android VPN API just does not > permit p2p mode, but wants a subnet. > > Arne, any idea if /31 works on Android? Yes. But Android is special here anyway. It always wants a subnet mask but the mask can be also be 255.255.255.255. And you also don't have a gateway address and Android also does not set a route to the tun interface (On Android 4.4+). So basically you always operate in p2p mode and emulate other modes by adding routes to the tun interface. Arne
Yeah I was merely referring to the trivial warning of the ifconfig mismatch. It's not exactly a problem but it just sort of made me aware of such thing as /31 subnet and noticed the bug on Linux. That's why I never opened an issue on ics-openvpn.
Hmm btw, not sure you are talking about the same thing, but at least my device the "proto kernel scope link" subnet route is added for the tun. While I can ping the "peer" out-of-the-box with /31 subnet and the subnet topology, with p2p topology and 255.255.255.255 mask, I need to add route in ics-openvpn for its peer before I can ping it. (The server is configured with topology and ifconfig corresponds to the client's in both tests.) On Tue, 5 Nov 2019 at 01:01, Arne Schwabe <arne@rfc2549.org> wrote: > > >> Yeah the removal of the p2p topology was one of the reasons. It's > >> actually the fact that ics-openvpn doesn't really parse point-to-point > >> ifconfig that made me aware of this. > > > > I wasn't aware of that, but since Arne is reading here, maybe he can > > comment on it. It's possible that the Android VPN API just does not > > permit p2p mode, but wants a subnet. > > > > Arne, any idea if /31 works on Android? > > Yes. But Android is special here anyway. It always wants a subnet mask > but the mask can be also be 255.255.255.255. And you also don't have a > gateway address and Android also does not set a route to the tun > interface (On Android 4.4+). So basically you always operate in p2p mode > and emulate other modes by adding routes to the tun interface. > > Arne > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Hi, On Mon, Nov 04, 2019 at 05:10:12AM +0800, Tom Yan wrote: > So in master, when net_addr_v4_add is called in tun.c, which > net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is > actually used depends on how openvpn is built, right? Right. It's either "default options" (-> sitnl.c) or "--with-iproute2" (->networking_iproute2.c). 2.4 has "ifconfig or iproute2" 2.5 has "direct interface to netlink" (sitnl) or "iproute2" > I think the `if (broadcast)` condition in the one of > networking_sitnl.c needs to be changed to `if (broadcast && prefixlen > < 31)` as well. I think this is more something the caller needs to decide "do I want to set a broadcast address or not" (so, if /31, set broadcast = NULL). OTOH, I'm still waiting for someone to tell me why we'd want to set a broadcast address in the first place :-) - Antonio, you know the kernel side better. Why would anyone explicitly configure the standard broadcast address ("all-ones in the host part of the relevant subnet")? > Also it seems that net_addr_v4_add is called wrongly > in tun.c with `&tt->remote_netmask` instead of `&tt->broadcast` as the > last argument. I tend to agree with you here. Antonio, yours :-) (though I ACKed it) - tun.c, line 1092. We do mix up "remote" and "netmask" in the shared "remote_netmask" variable, but "broadcast" is independent of that... and OpenVPN actually tells us so, in the "topology subnet" case... net_addr_v4_add: 10.194.6.2/24 brd 255.255.255.0 dev tun1 Linux seems to accept this, but it has no further consequences ("all my tests pass")... $ ip addr show dev tun1 520: tun1: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UNKNOWN group default qlen 100 link/none inet 10.194.6.2/24 brd 255.255.255.0 scope global tun1 valid_lft forever preferred_lft forever ... but it's still not correct. gert
Hi, On Mon, Nov 04, 2019 at 05:10:12AM +0800, Tom Yan wrote: > So in master, when net_addr_v4_add is called in tun.c, which > net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is > actually used depends on how openvpn is built, right? Can you have a look at the current "master" branch? Antonio has ripped out *all* broadcast setting - so this should now do /31s just fine? More radical than your approach, but "less old code" is better than "yet another special condition added" :-) gert
Yes both iproute2 and sitnl works perfectly as expected. Will the fix be backported to the 2.4 branch? P.S. For the record, if the client is ics-openvpn with openvpn3, somehow you need push route (either host or subnet will do) anyway. This is not necessary with ics-openvpn with openvpn2 or OpenVPN Connect (AFAIK its core is openvpn3 as well, albeit different snapshot). The problem/behaviour is not specific to /31 subnet though, apparently. I guess the main route table is ignored/masked. On Mon, 11 Nov 2019 at 02:07, Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Mon, Nov 04, 2019 at 05:10:12AM +0800, Tom Yan wrote: > > So in master, when net_addr_v4_add is called in tun.c, which > > net_addr_v4_add (networking_iproute2.c or networking_sitnl.c) is > > actually used depends on how openvpn is built, right? > > Can you have a look at the current "master" branch? Antonio has ripped > out *all* broadcast setting - so this should now do /31s just fine? > > More radical than your approach, but "less old code" is better than > "yet another special condition added" :-) > > gert > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de
Am 11.11.19 um 16:12 schrieb Tom Yan: > Yes both iproute2 and sitnl works perfectly as expected. Will the fix > be backported to the 2.4 branch? > > P.S. For the record, if the client is ics-openvpn with openvpn3, > somehow you need push route (either host or subnet will do) anyway. I am not aware of this issue. Could you go more into detail? > This is not necessary with ics-openvpn with openvpn2 or OpenVPN > Connect (AFAIK its core is openvpn3 as well, albeit different > snapshot). The problem/behaviour is not specific to /31 subnet though, > apparently. I guess the main route table is ignored/masked. ics-openvpn is a) OpenVPN master b) uses a completely different logic to handle network config (in OpenVPNService.java). So this patch does not affect ics-openvpn in any way. Arne
Details like...? If you configure/push an IP with a subnet (i.e. non /32 prefix or 255.255.255.255 subnet mask) to a client, a subnet route will be added to main route table (as show by `ip r`) by the kernel just like on Desktop Linux (at least that's what would happen on my device). When I use ics-openvpn without openvpn3 box checked or OpenVPN Connect, the route would be in effect so I don't need to push/configure one explicitly to the client for it to be able to reach the server (or vice versa) via the tunnel. But if I use ics-openvpn with the openvpn3 box checked, apparently the route is not in effect (but still added), and therefore for it to be able to reach the server, I would need to push/configure a route explicitly. On Tue, 12 Nov 2019 at 02:02, Arne Schwabe <arne@rfc2549.org> wrote: > > Am 11.11.19 um 16:12 schrieb Tom Yan: > > Yes both iproute2 and sitnl works perfectly as expected. Will the fix > > be backported to the 2.4 branch? > > > > P.S. For the record, if the client is ics-openvpn with openvpn3, > > somehow you need push route (either host or subnet will do) anyway. > > I am not aware of this issue. Could you go more into detail? > > > This is not necessary with ics-openvpn with openvpn2 or OpenVPN > > Connect (AFAIK its core is openvpn3 as well, albeit different > > snapshot). The problem/behaviour is not specific to /31 subnet though, > > apparently. I guess the main route table is ignored/masked. > > ics-openvpn is a) OpenVPN master b) uses a completely different logic to > handle network config (in OpenVPNService.java). So this patch does not > affect ics-openvpn in any way. > > Arne > > >
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 80eaa2c4..f5823516 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -965,13 +965,14 @@ do_ifconfig(struct tuntap *tt, else { argv_printf(&argv, - "%s addr add dev %s %s/%d broadcast %s", + "%s addr add dev %s %s/%d", iproute_path, actual, ifconfig_local, - netmask_to_netbits2(tt->remote_netmask), - ifconfig_broadcast + netmask_to_netbits2(tt->remote_netmask) ); + if (netmask_to_netbits2(tt->remote_netmask) < 31) + argv_printf_cat(&argv, " broadcast %s", ifconfig_broadcast); argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, es, S_FATAL, "Linux ip addr add failed"); }