| Message ID | 20250915110507.20557-1-sebastian-git-2016@marsching.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v3] Bugfix: Set broadcast address on interface. | expand |
Hi, On 15/09/2025 13:05, Sebastian Marsching wrote: > This fixes a problem that was introduced in OpenVPN 2.5. Previously, > the ifconfig utility was used for adding the local address to an > interface. This utility automatically sets the correct broadcast address > based on the given unicast address and netmask. > > Due to switching to iproute and Netlink, this does not happen > automatically any longer, which means that applications that rely on > broadcasts do not work correctly. > iproute2 was already supported before 2.5 (but probably it needed to be explicitly activated at compile time), therefore I'd say this bug already existed :) I went through the kernel code and I can clearly see the difference in APIs: the old SIOCSIFADDR ioctl (used by net-tools/ifconfig) takes care of computing the broadcast address: 1230 if (!(dev->flags & IFF_POINTOPOINT)) { 1231 ifa->ifa_prefixlen = inet_abc_len(ifa->ifa_address); 1232 ifa->ifa_mask = inet_make_mask(ifa->ifa_prefixlen); 1233 if ((dev->flags & IFF_BROADCAST) && 1234 ifa->ifa_prefixlen < 31) 1235 ifa->ifa_broadcast = ifa->ifa_address | 1236 ~ifa->ifa_mask; 1237 } However, this logic does not exist in the more modern netlink code, therefore it was a somehow conscious decision. > This patch fixes this issue both when using iproute (by telling iproute > to set the broadcast address based on the local address and prefix) and > when using Netlink (by calculating the correct broadcast address and > setting it). I imagine this is the behaviour the average OpenVPN user expects (although probably only few of them needs a brd address), therefore thanks for fixing this. > > Signed-off-by: Sebastian Marsching <sebastian-git-2016@marsching.com> > --- > src/openvpn/networking_iproute2.c | 2 +- > src/openvpn/networking_sitnl.c | 8 ++++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c > index e9be3a45..773571d6 100644 > --- a/src/openvpn/networking_iproute2.c > +++ b/src/openvpn/networking_iproute2.c > @@ -150,7 +150,7 @@ net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface, const in_addr_t *addr > > const char *addr_str = print_in_addr_t(*addr, 0, &ctx->gc); > > - argv_printf(&argv, "%s addr add dev %s %s/%d", iproute_path, iface, addr_str, prefixlen); > + argv_printf(&argv, "%s addr add dev %s %s/%d broadcast +", iproute_path, iface, addr_str, prefixlen); > argv_msg(M_INFO, &argv); > openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed"); > > diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c > index 4210e92c..00d61067 100644 > --- a/src/openvpn/networking_sitnl.c > +++ b/src/openvpn/networking_sitnl.c > @@ -31,6 +31,7 @@ > #include "misc.h" > #include "networking.h" > #include "proto.h" > +#include "route.h" > > #include <errno.h> > #include <string.h> > @@ -803,6 +804,13 @@ sitnl_addr_set(int cmd, uint32_t flags, int ifindex, sa_family_t af_family, > SITNL_ADDATTR(&req.n, sizeof(req), IFA_LOCAL, local, size); > } > > + if (af_family == AF_INET && local && !remote && prefixlen <= 30) > + { > + inet_address_t broadcast = *local; > + broadcast.ipv4 |= htonl(~netbits_to_netmask(prefixlen)); > + SITNL_ADDATTR(&req.n, sizeof(req), IFA_BROADCAST, &broadcast, size); > + } > + The code looks good and makes sense to me (nice to see how concise a patch extending sitnl can be :)). Thanks Gert for complaining about the style earlier on. I haven't tested it, but we have a buildbot army which will tell us if some esoteric bug is hidden away. Acked-by: Antonio Quartulli <antonio@mandelbit.com> Regards, > ret = sitnl_send(&req.n, 0, 0, NULL, NULL); > if (ret == -EEXIST) > {
I have tested this on Linux "without DCO" and "with DCO", client
and server. The non-DCO server version isn't really helpful as it
puts all tun interface into p2p mode ("routing happens inside OpenVPN"),
but the TAP interfaces shows the expected "broadcast" setting - as does
the "tun / topology subnet" client. On the DCO "tun" servers the
handling of routes/iroutes is different, and the expected netmask + brd
shows up. And the tests still pass :-)
Your patch has been applied to the master and release/2.6 branch
(whether it's "a bugfix" stands to be debated - but it is not very
intrusive and has passed all my system tests, and if the lack of
broadcast did break people's working 2.5 setups they will well
consider it a "bug"... so, "bugfix")
commit 0df0edc49ce4acb96c8e16bdf0fcee1eedfd91f0 (master)
commit 4e46e727217cf58c7c671e36e8417e469c4e3b1d (release/2.6)
Author: Sebastian Marsching
Date: Mon Sep 15 13:05:07 2025 +0200
Bugfix: Set broadcast address on interface.
Signed-off-by: Sebastian Marsching <sebastian-git-2016@marsching.com>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20250915110507.20557-1-sebastian-git-2016@marsching.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg33131.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c index e9be3a45..773571d6 100644 --- a/src/openvpn/networking_iproute2.c +++ b/src/openvpn/networking_iproute2.c @@ -150,7 +150,7 @@ net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface, const in_addr_t *addr const char *addr_str = print_in_addr_t(*addr, 0, &ctx->gc); - argv_printf(&argv, "%s addr add dev %s %s/%d", iproute_path, iface, addr_str, prefixlen); + argv_printf(&argv, "%s addr add dev %s %s/%d broadcast +", iproute_path, iface, addr_str, prefixlen); argv_msg(M_INFO, &argv); openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed"); diff --git a/src/openvpn/networking_sitnl.c b/src/openvpn/networking_sitnl.c index 4210e92c..00d61067 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -31,6 +31,7 @@ #include "misc.h" #include "networking.h" #include "proto.h" +#include "route.h" #include <errno.h> #include <string.h> @@ -803,6 +804,13 @@ sitnl_addr_set(int cmd, uint32_t flags, int ifindex, sa_family_t af_family, SITNL_ADDATTR(&req.n, sizeof(req), IFA_LOCAL, local, size); } + if (af_family == AF_INET && local && !remote && prefixlen <= 30) + { + inet_address_t broadcast = *local; + broadcast.ipv4 |= htonl(~netbits_to_netmask(prefixlen)); + SITNL_ADDATTR(&req.n, sizeof(req), IFA_BROADCAST, &broadcast, size); + } + ret = sitnl_send(&req.n, 0, 0, NULL, NULL); if (ret == -EEXIST) {
This fixes a problem that was introduced in OpenVPN 2.5. Previously, the ifconfig utility was used for adding the local address to an interface. This utility automatically sets the correct broadcast address based on the given unicast address and netmask. Due to switching to iproute and Netlink, this does not happen automatically any longer, which means that applications that rely on broadcasts do not work correctly. This patch fixes this issue both when using iproute (by telling iproute to set the broadcast address based on the local address and prefix) and when using Netlink (by calculating the correct broadcast address and setting it). Signed-off-by: Sebastian Marsching <sebastian-git-2016@marsching.com> --- src/openvpn/networking_iproute2.c | 2 +- src/openvpn/networking_sitnl.c | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)