| Message ID | 20250201062437.4059652-1-dqfext@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Openvpn-devel,v2] dco: fix source IP selection | expand |
Hi, On Sat, Feb 01, 2025 at 02:24:37PM +0800, Qingfang Deng wrote: > #endif > + if (sock->bind_local && sock->info.lsa->bind_local) { > + localaddr = sock->info.lsa->bind_local->ai_addr; > + } > + This is still not correct style - the opening "{" needs to go on its own line. Also see below on whether this is necessary at all. > int ret = dco_new_peer(&c->c1.tuntap->dco, multi->peer_id, > - c->c2.link_sockets[0]->sd, NULL, remoteaddr, NULL, NULL); > + c->c2.link_sockets[0]->sd, localaddr, remoteaddr, NULL, NULL); > if (ret < 0) > { > return ret; > @@ -550,7 +555,7 @@ dco_multi_get_localaddr(struct multi_context *m, struct multi_instance *mi, > { > struct sockaddr_in *sock_in4 = (struct sockaddr_in *)local; > #if defined(HAVE_IN_PKTINFO) && defined(HAVE_IPI_SPEC_DST) > - sock_in4->sin_addr = actual->pi.in4.ipi_addr; > + sock_in4->sin_addr = actual->pi.in4.ipi_spec_dst; > #elif defined(IP_RECVDSTADDR) > sock_in4->sin_addr = actual->pi.in4; Well spotted. > #else > @@ -616,10 +621,15 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) > vpn_addr6 = &c->c2.push_ifconfig_ipv6_local; > } > > + struct link_socket *ls = c->c2.link_sockets[0]; > if (dco_multi_get_localaddr(m, mi, &local)) > { > localaddr = (struct sockaddr *)&local; > } > + else if (ls->bind_local && ls->info.lsa->bind_local) > + { > + localaddr = ls->info.lsa->bind_local->ai_addr; > + } Not sure about that. If the socket is bound, the kernel knows about the binding (because we pass the socket to the kernel). You're sure this is needed? gert
Hi, Thanks a lot for bringing this problem to our attention. On 01/02/2025 08:54, Gert Doering wrote: >> @@ -616,10 +621,15 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) >> vpn_addr6 = &c->c2.push_ifconfig_ipv6_local; >> } >> >> + struct link_socket *ls = c->c2.link_sockets[0]; >> if (dco_multi_get_localaddr(m, mi, &local)) >> { >> localaddr = (struct sockaddr *)&local; >> } >> + else if (ls->bind_local && ls->info.lsa->bind_local) >> + { >> + localaddr = ls->info.lsa->bind_local->ai_addr; >> + } > > Not sure about that. If the socket is bound, the kernel knows about > the binding (because we pass the socket to the kernel). You're sure this > is needed? As already questioned by Gert, I can confirm that this is not expected, because DCO should automatically pick any source address that was bound to the socket. This said, DCO seems to not be doing this and therefore the bound source address is not respected. Therefore this is a bug that needs fixing in DCO and should not be worked-around by userspace. Hence, I'd suggest to drop the hunk about and rather fix the issue in DCO. while looking at this patch, I have already identified where DCO needs adjustments, therefore it's likely that the next DCO revision will have this fixed. Regards, > > gert > > > > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index b5a21369..eef49ade 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -493,6 +493,7 @@ dco_p2p_add_new_peer(struct context *c) ASSERT(sock->info.connection_established); struct sockaddr *remoteaddr = &sock->info.lsa->actual.dest.addr.sa; + struct sockaddr *localaddr = NULL; struct tls_multi *multi = c->c2.tls_multi; #ifdef TARGET_FREEBSD /* In Linux in P2P mode the kernel automatically removes an existing peer @@ -503,8 +504,12 @@ dco_p2p_add_new_peer(struct context *c) c->c2.tls_multi->dco_peer_id = -1; } #endif + if (sock->bind_local && sock->info.lsa->bind_local) { + localaddr = sock->info.lsa->bind_local->ai_addr; + } + int ret = dco_new_peer(&c->c1.tuntap->dco, multi->peer_id, - c->c2.link_sockets[0]->sd, NULL, remoteaddr, NULL, NULL); + c->c2.link_sockets[0]->sd, localaddr, remoteaddr, NULL, NULL); if (ret < 0) { return ret; @@ -550,7 +555,7 @@ dco_multi_get_localaddr(struct multi_context *m, struct multi_instance *mi, { struct sockaddr_in *sock_in4 = (struct sockaddr_in *)local; #if defined(HAVE_IN_PKTINFO) && defined(HAVE_IPI_SPEC_DST) - sock_in4->sin_addr = actual->pi.in4.ipi_addr; + sock_in4->sin_addr = actual->pi.in4.ipi_spec_dst; #elif defined(IP_RECVDSTADDR) sock_in4->sin_addr = actual->pi.in4; #else @@ -616,10 +621,15 @@ dco_multi_add_new_peer(struct multi_context *m, struct multi_instance *mi) vpn_addr6 = &c->c2.push_ifconfig_ipv6_local; } + struct link_socket *ls = c->c2.link_sockets[0]; if (dco_multi_get_localaddr(m, mi, &local)) { localaddr = (struct sockaddr *)&local; } + else if (ls->bind_local && ls->info.lsa->bind_local) + { + localaddr = ls->info.lsa->bind_local->ai_addr; + } int ret = dco_new_peer(&c->c1.tuntap->dco, peer_id, sd, localaddr, remoteaddr, vpn_addr4, vpn_addr6);
When multihome option is enabled, OpenVPN passes ipi_addr to DCO, which is always 0.0.0.0. It should use ipi_spec_dst instead. When local option is present, OpenVPN does not pass it to DCO. As a result, Linux may pick a different IP as the source IP, breaking the connection. Signed-off-by: Qingfang Deng <dqfext@gmail.com> --- v2: fix code style reported by CI Discussions: https://github.com/OpenVPN/openvpn/pull/668 src/openvpn/dco.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)