| Message ID | 20220607093619.23066-1-gert@greenie.muc.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Fix error message about extended errors for IPv4-only sockets. | expand |
> #if EXTENDED_SOCKET_ERROR_CAPABILITY > /* if the OS supports it, enable extended error passing on the socket */ > - set_sock_extended_error_passing(sock->sd); > + set_sock_extended_error_passing(sock->sd, sock->info.af); > #endif > } > I have to NACK this patch. sock->info.af is not the actual address familiy but the family we want to use and it might be 0 if there is no udp4/udp6 in the config file. For a client you can look at sock->info.lsa->actual.dest if that is defined. For a server we seem to actually override/set that info.af value according to my debugger but with a client I get 0 in that value. It looks that set_mtu_discover_type in the same function has the same problem, so it is easy to this mistake. Arne
Hi, On Sun, Jun 19, 2022 at 11:38:17AM +0200, Arne Schwabe wrote: > > #if EXTENDED_SOCKET_ERROR_CAPABILITY > > /* if the OS supports it, enable extended error passing on the socket */ > > - set_sock_extended_error_passing(sock->sd); > > + set_sock_extended_error_passing(sock->sd, sock->info.af); > > #endif > > } > > > > I have to NACK this patch. sock->info.af is not the actual address > familiy but the family we want to use and it might be 0 if there is no > udp4/udp6 in the config file. > > For a client you can look at sock->info.lsa->actual.dest if that is > defined. For a server we seem to actually override/set that info.af > value according to my debugger but with a client I get 0 in that value. > It looks that set_mtu_discover_type in the same function has the same > problem, so it is easy to this mistake. So how do we know if this is a v4 or v6 socket, in a client? Phrased differently - I could do a v2 which will just ignore this particular errno value. Maybe not "as clean" as not calling the "wrong" setsockopt() in the first place, but will keep the log free of non-useful error messages just fine... gert
Am 19.06.22 um 19:03 schrieb Gert Doering: > Hi, > > On Sun, Jun 19, 2022 at 11:38:17AM +0200, Arne Schwabe wrote: >>> #if EXTENDED_SOCKET_ERROR_CAPABILITY >>> /* if the OS supports it, enable extended error passing on the socket */ >>> - set_sock_extended_error_passing(sock->sd); >>> + set_sock_extended_error_passing(sock->sd, sock->info.af); >>> #endif >>> } >>> >> >> I have to NACK this patch. sock->info.af is not the actual address >> familiy but the family we want to use and it might be 0 if there is no >> udp4/udp6 in the config file. >> >> For a client you can look at sock->info.lsa->actual.dest if that is >> defined. For a server we seem to actually override/set that info.af >> value according to my debugger but with a client I get 0 in that value. >> It looks that set_mtu_discover_type in the same function has the same >> problem, so it is easy to this mistake. > > So how do we know if this is a v4 or v6 socket, in a client? > > Phrased differently - I could do a v2 which will just ignore this > particular errno value. Maybe not "as clean" as not calling the > "wrong" setsockopt() in the first place, but will keep the log free > of non-useful error messages just fine... Two ways. Either fix that info.af is 0 for client sockets. You already fixed that for MTU discovery and server sockets %) (commit ed5d0fe5097). For client socket, we should probably set the info.af in the openvpn_connect/socket_connect path. The other more hacky is to check info.af first and then check lsa->actual.dest and see if one of them is non-zero. Arne
diff --git a/src/openvpn/mtu.c b/src/openvpn/mtu.c index 59b91798..f60f4853 100644 --- a/src/openvpn/mtu.c +++ b/src/openvpn/mtu.c @@ -413,17 +413,22 @@ exit: } void -set_sock_extended_error_passing(int sd) +set_sock_extended_error_passing(int sd, sa_family_t proto_af) { int on = 1; - /* see "man 7 ip" (on Linux) */ + /* see "man 7 ip" (on Linux) + * this works on IPv4 and IPv6(-dual-stack) sockets (v4-mapped) + */ if (setsockopt(sd, SOL_IP, IP_RECVERR, (void *) &on, sizeof(on)) != 0) { msg(M_WARN | M_ERRNO, "Note: enable extended error passing on TCP/UDP socket failed (IP_RECVERR)"); } - /* see "man 7 ipv6" (on Linux) */ - if (setsockopt(sd, IPPROTO_IPV6, IPV6_RECVERR, (void *) &on, sizeof(on)) != 0) + /* see "man 7 ipv6" (on Linux) + * this only works on IPv6 sockets + */ + if (proto_af == AF_INET6 + && setsockopt(sd, IPPROTO_IPV6, IPV6_RECVERR, (void *) &on, sizeof(on)) != 0) { msg(M_WARN | M_ERRNO, "Note: enable extended error passing on TCP/UDP socket failed (IPV6_RECVERR)"); diff --git a/src/openvpn/mtu.h b/src/openvpn/mtu.h index 7f967e06..ac3268f7 100644 --- a/src/openvpn/mtu.h +++ b/src/openvpn/mtu.h @@ -278,7 +278,7 @@ void alloc_buf_sock_tun(struct buffer *buf, #if EXTENDED_SOCKET_ERROR_CAPABILITY -void set_sock_extended_error_passing(int sd); +void set_sock_extended_error_passing(int sd, sa_family_t proto_af); const char *format_extended_socket_error(int fd, int *mtu, struct gc_arena *gc); diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c index 4e4a3a2f..47f1ba27 100644 --- a/src/openvpn/socket.c +++ b/src/openvpn/socket.c @@ -1949,7 +1949,7 @@ phase2_set_socket_flags(struct link_socket *sock) #if EXTENDED_SOCKET_ERROR_CAPABILITY /* if the OS supports it, enable extended error passing on the socket */ - set_sock_extended_error_passing(sock->sd); + set_sock_extended_error_passing(sock->sd, sock->info.af); #endif }
The new code to enable IPv6 extended error reporting will cause an error ("Protocol not available (errno=92)") if trying to enable that setsockopt() option on an IPv4-only socket. Fix: pass sock->info.af to set_sock_extended_error_passing(), only apply to AF_INET6 sockets. Add comments to make explicit that the asymmetry here (IPv4 extended socket error reporting is enabled on all sockets) is intentional. Signed-off-by: Gert Doering <gert@greenie.muc.de> --- src/openvpn/mtu.c | 13 +++++++++---- src/openvpn/mtu.h | 2 +- src/openvpn/socket.c | 2 +- 3 files changed, 11 insertions(+), 6 deletions(-)