[Openvpn-devel] Fix error message about extended errors for IPv4-only sockets.

Message ID 20220607093619.23066-1-gert@greenie.muc.de
State New
Headers show
Series
  • [Openvpn-devel] Fix error message about extended errors for IPv4-only sockets.
Related show

Commit Message

Gert Doering June 7, 2022, 9:36 a.m.
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(-)

Comments

Arne Schwabe June 19, 2022, 9:38 a.m. | #1
>   #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
Gert Doering June 19, 2022, 5:03 p.m. | #2
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
Arne Schwabe June 20, 2022, 9:01 a.m. | #3
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

Patch

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
 }