| Message ID | 65233605-9738-4B2C-8401-37AA5C43D797@marsching.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel] Bugfix: Set broadcast address on interface | expand |
Hi, On Sat, Sep 13, 2025 at 04:25:25PM +0200, Sebastian Marsching wrote: > 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). Should Linux not be able to auto-calculate that, based on IP address and netmask? No other OS needs explicit setting of broadcast address in 2025... Can you show the difference in "ip address show" or "ifconfig" output before/after your change? Is this on a client or server? gert
Hi Gert, > On Sat, Sep 13, 2025 at 04:25:25PM +0200, Sebastian Marsching wrote: >> 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). > > Should Linux not be able to auto-calculate that, based on IP address > and netmask? You would think so, but it does not… > No other OS needs explicit setting of broadcast address in 2025... > > Can you show the difference in "ip address show" or "ifconfig" output > before/after your change? Is this on a client or server? Output of ifconfig without the change (IPv6 address anonymized by me): tap0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 inet 172.22.56.178 netmask 255.255.255.0 broadcast 0.0.0.0 inet6 xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx prefixlen 64 scopeid 0x0<global> inet6 fe80::800b:93ff:fe07:a32a prefixlen 64 scopeid 0x20<link> ether f6:f6:91:dc:83:af txqueuelen 1000 (Ethernet) RX packets 108 bytes 32404 (32.4 KB) RX errors 0 dropped 0 overruns 0 frame 0 TX packets 14 bytes 1172 (1.1 KB) TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 With the change: tap0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 inet 172.22.56.178 netmask 255.255.255.0 broadcast 172.22.56.255 inet6 xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx prefixlen 64 scopeid 0x0<global> inet6 fe80::f0f2:e4ff:fe4f:bfff prefixlen 64 scopeid 0x20<link> ether f2:f2:e4:4f:bf:ff txqueuelen 1000 (Ethernet) RX packets 12 bytes 1200 (1.2 KB) RX errors 0 dropped 0 overruns 0 frame 0 TX packets 11 bytes 962 (962.0 B) TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0 When using ip address show instead, the difference is that without the change inet 172.22.56.178/24 scope global tap0 is shown, while with the change inet 172.22.56.178/24 brd 172.22.56.255 scope global tap0 is shown. In fact, systemd-networkd also does it. When configuring an IP address, 1. it calls the code that calculates the broadcast address at https://github.com/systemd/systemd/blob/5bf7438ff025ae05daf1b706f204f31373d5ab82/src/network/networkd-address.c#L1667, 2. it calculates the broadcast address (if not specified explicitly) at https://github.com/systemd/systemd/blob/5bf7438ff025ae05daf1b706f204f31373d5ab82/src/network/networkd-address.c#L408, 3. and finally it send the broadcast address to the kernel at https://github.com/systemd/systemd/blob/5bf7438ff025ae05daf1b706f204f31373d5ab82/src/network/networkd-address.c#L1555. It implements the way how the broadcast address is calculated slightly differently. For my patch, I took the inspiration from how iproute2 implements it instead. Best regards, Sebastian
Hi, On Sat, Sep 13, 2025 at 06:14:03PM +0200, Sebastian Marsching wrote: > > Should Linux not be able to auto-calculate that, based on IP address > > and netmask? > You would think so, but it does not??? It seems so. Thanks for the output examples. Weird. [..] > It implements the way how the broadcast address is calculated slightly differently. For my patch, I took the inspiration from how iproute2 implements it instead. It's a bit unfortunate that *this* piece of code does not have the netmask around anymore (having converted it carefully to a /netbits value...) I'll leave this to Antonio to ACK, as it's all his code. gert
Hi, On Sat, Sep 13, 2025 at 04:25:25PM +0200, Sebastian Marsching wrote: > @@ -803,6 +804,16 @@ 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) > + { > + broadcast = *local; > + for (i = 31; i >= prefixlen; i--) Ah, well, I guess the integer police will come after us... so in modern parts of OpenVPN we go for local C99 declarations, *and* we take extra care to make it all -Wsign-conversion safe, so I think this needs to be + for (unsigned int i = 31; i >= prefixlen; i--) (and no extra "int i" declaration on top) This said, I see we have a "netbits_to_netmask(bits)" function in route.h -> please use that, it should be something like + if (af_family == AF_INET && local && !remote && prefixlen <= 30) + { + inet_address_t broadcast = *local; + broadcast.ipv4 |= ~netbits_to_netmask(prefixlen); + ... (maybe with an htonl() wrapped around) gert
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..4294a7a5 100644 --- a/src/openvpn/networking_sitnl.c +++ b/src/openvpn/networking_sitnl.c @@ -760,7 +760,8 @@ sitnl_addr_set(int cmd, uint32_t flags, int ifindex, sa_family_t af_family, { struct sitnl_addr_req req; uint32_t size; - int ret = -EINVAL; + inet_address_t broadcast; + int ret = -EINVAL, i; CLEAR(req); @@ -803,6 +804,16 @@ 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) + { + broadcast = *local; + for (i = 31; i >= prefixlen; i--) + { + broadcast.ipv4 |= htonl(1<<(31-i)); + } + SITNL_ADDATTR(&req.n, sizeof(req), IFA_BROADCAST, &broadcast, size); + } + ret = sitnl_send(&req.n, 0, 0, NULL, NULL); if (ret == -EEXIST) {