Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Gert Doering]

Hi,

On Mon, Nov 23, 2015 at 11:51:43AM +, Christian Pellegrin wrote:
> On Sat, Nov 21, 2015 at 8:06 PM, Gert Doering  wrote:
> > if (sock->info.af == AF_UNSPEC)
> > +   {
> >msg (M_WARN, "Could not determine IPv4/IPv6 protocol. 
> > Using %s",
> >  
> > addr_family_name(sock->info.lsa->bind_local->ai_family));
> > +  sock->info.af = sock->info.lsa->bind_local->ai_family;
> > +   }
> 
> Just tested, I ack it works perfectly. Thanks!

Thanks.  So I've committed the patch as appended (slightly different
format than the "usual" process as the original patch was no formal 
commit yet, but message-id and ack etc. preserved)

gert

From ed5d0fe5097a26206a6a7d4463622461a0987655 Mon Sep 17 00:00:00 2001
From: Gert Doering 
List-Post: openvpn-devel@lists.sourceforge.net
Date: Mon, 23 Nov 2015 20:47:42 +0100
Subject: [PATCH] Fix info.af == AF_UNSPEC case for server with --mtu-disc

Commit 2bed089d31a12c2 introduced "AF_UNSPEC" sockets when we do not know
the actual address family yet - for the "bind local" case, getaddrinfo()
will tell us what to do, but that information never made it into
sock->info.af - so, make it.  Otherwise, trying to call --mtu-disc
on an OpenVPN server will cause a M_FATAL error in set_mtu_discovery())

Signed-off-by: Gert Doering 
Acked-by: Christian Pellegrin 
Message-ID: <20151121200637.gd24...@greenie.muc.de>
URL: http://article.gmane.org/gmane.network.openvpn.devel/10547
---
 src/openvpn/socket.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index b24d9ad..8e6b4bc 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1887,8 +1887,11 @@ link_socket_init_phase2 (struct link_socket *sock,
  /* Warn if this is because neither v4 or v6 was specified
   * and we should not connect a remote */
  if (sock->info.af == AF_UNSPEC)
-   msg (M_WARN, "Could not determine IPv4/IPv6 protocol. Using %s",
+   {
+ msg (M_WARN, "Could not determine IPv4/IPv6 protocol. Using 
%s",
 addr_family_name(sock->info.lsa->bind_local->ai_family));
+ sock->info.af = sock->info.lsa->bind_local->ai_family;
+   }
 
  create_socket (sock, sock->info.lsa->bind_local);
}
-- 
2.4.9


-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Christian Pellegrin]

On Sat, Nov 21, 2015 at 8:06 PM, Gert Doering  wrote:
> if (sock->info.af == AF_UNSPEC)
> +   {
>msg (M_WARN, "Could not determine IPv4/IPv6 protocol. 
> Using %s",
>  addr_family_name(sock->info.lsa->bind_local->ai_family));
> +  sock->info.af = sock->info.lsa->bind_local->ai_family;
> +   }

Just tested, I ack it works perfectly. Thanks!

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[christian pellegrin]

On Sun, Nov 22, 2015 at 1:19 PM, Gert Doering  wrote:
> Could you test the patch suggestion I sent yesterday?  I think your log
> *should* (before the ASSERT) show the "Could not determine IPv4/IPv6
> protocol. Using..." message...


Ack, will do tomorrow and report.

-- 
Christian Pellegrin, see http://www.evolware.org/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Gert Doering]

Hi,

On Sun, Nov 22, 2015 at 01:09:48PM +, christian pellegrin wrote:
> Huh, sorry the segfault. 

Nothing to be sorry about - that code is complicated ("has grown over
10+ years, too many options and platforms") and refactoring it for
proper dual-stack support still sees some fallout...

> To solve this specific problem it's enough to
> test for AF_UNSPEC in
> https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/mtu.c#L163,
> I will prepare the patch tomorrow.

I'm not sure what the "right" fix would be at this place - all you can
do is either "do nothing" or "try IPv4 and IPv6 both", which both does
not ring right to me.  If we reach this spot, we should know which AF
our socket has...

> I don't think the configuration is too strange. It uses proto udp,
> mode server and sets local address because of LB/HA setup.
> mtu-discovery is set to maybe. What is really strange (and will check
> what is the code flow in this case) is that if I enable multihome and
> IPv6 I don't see the problem.

If you use "proto udp6", you're forcing AF_INET6.

Could you test the patch suggestion I sent yesterday?  I think your log
*should* (before the ASSERT) show the "Could not determine IPv4/IPv6
protocol. Using..." message...

thanks,

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[christian pellegrin]

Huh, sorry the segfault. To solve this specific problem it's enough to
test for AF_UNSPEC in
https://github.com/OpenVPN/openvpn/blob/master/src/openvpn/mtu.c#L163,
I will prepare the patch tomorrow.

I don't think the configuration is too strange. It uses proto udp,
mode server and sets local address because of LB/HA setup.
mtu-discovery is set to maybe. What is really strange (and will check
what is the code flow in this case) is that if I enable multihome and
IPv6 I don't see the problem.

On Sat, Nov 21, 2015 at 9:21 PM, Arne Schwabe  wrote:
>
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
>
>>> +++ b/src/openvpn/socket.c >> @@ -1670,7 +1670,7 @@ phase2_set_socket_flags 
>>> (struct link_socket*
> sock) >>  set_cloexec (sock->ctrl_sd); >>   >>/* set Path MTU
> discovery options on the socket */ >> -  set_mtu_discover_type
> (sock->sd, sock->mtu_discover_type, sock->info.af); >> +
> set_mtu_discover_type (sock->sd, sock->mtu_discover_type,
> sock->info.lsa->bind_local->ai_family); > > ... on a client with
> --nobind, this immediately crashes as lsa->bind_local > is not set... >
>> Sat Nov 21 21:01:48 2015 OpenVPN 2.3_git
> [git:master/48c23dc92006e1d8+] i686-pc-linux-gnu [SSL (OpenSSL)] [LZO]
> [LZ4] [EPOLL] [MH] [IPv6] built on Nov 21 2015 > Sat Nov 21 21:01:48
> 2015 library versions: OpenSSL 1.0.2d 9 Jul 2015, LZO 2.08 > Sat Nov 21
> 21:01:48 2015 TCP/UDP: Preserving recently used remote address:
> [AF_INET]199.102.77.82:51194 > Sat Nov 21 21:01:48 2015 Socket Buffers:
> R=[110592->110592] S=[110592->110592] > > Program received signal
> SIGSEGV, Segmentation fault. > 0x0809c165 in phase2_set_socket_flags
> (sock=0x80ee270) > at ../../../openvpn/src/openvpn/socket.c:1673 >
> 1673  set_mtu_discover_type (sock->sd, sock->mtu_discover_type,
> sock->info.lsa->bind_local->ai_family); > > (gdb) print
> sock->info.lsa->bind_local > $1 = (struct addrinfo *) 0x0 > > > So,
> overruling Arne's ACK and not applying it :-) > > > I wonder if the
> *right* fix for this case wouldn't be to extend this > code block
> (before calling phase2_set_socket_flags(), socket.c line 1880): >
>>   if (sock->bind_local  && !sock->remote_host &&
> sock->info.lsa->bind_local) > { >   /* Warn if
> this is because neither v4 or v6 was specified >* and we
> should not connect a remote */ >   if (sock->info.af ==
> AF_UNSPEC) > msg (M_WARN, "Could not determine IPv4/IPv6
> protocol. Using %s", >
> addr_family_name(sock->info.lsa->bind_local->ai_family)); >
>>   create_socket (sock, sock->info.lsa->bind_local);
>> } > > into... > >   if (sock->info.af ==
> AF_UNSPEC) > +{ >msg (M_WARN, "Could not
> determine IPv4/IPv6 protocol. Using %s", >
> addr_family_name(sock->info.lsa->bind_local->ai_family)); > +
> sock->info.af = sock->info.lsa->bind_local->ai_family; > +} > >
> which would catch your case, but also the "--nobind and we have a
> remote" > case (client). > > Arne, what do you think?
>
> For clients you never get into that codeblock because a remote either
> resolves (and then has an AF family or does not resolve). For a server
> we also bail out if we cannot resolve the bind address (with :: or
> 0.0.0.0 being the default). The only case you can into that is with a
> weird config like mtu-discovery.
>
> @Christian, how did you manage to trigger the bug?
>
> Arne
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v2.0.22 (MingW32)
>
> iEYEARECAAYFAlZQ4EsACgkQe8+cMNS4zRd+cACgxx/d2wY7GdyNkb+/YY93Vxoq
> TZUAoOWgpd0OgR0BtbWhwwpU/ZYzRq8O
> =4VJ/
> -END PGP SIGNATURE-
>
>
> --
> ___
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel



-- 
Christian Pellegrin, see http://www.evolware.org/
"Real Programmers don't play tennis, or any other sport which requires
you to change clothes. Mountain climbing is OK, and Real Programmers
wear their climbing boots to work in case a mountain should suddenly
spring up in the middle of the computer room."

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Arne Schwabe]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1


>> +++ b/src/openvpn/socket.c >> @@ -1670,7 +1670,7 @@ phase2_set_socket_flags 
>> (struct link_socket*
sock) >>  set_cloexec (sock->ctrl_sd); >>   >>/* set Path MTU
discovery options on the socket */ >> -  set_mtu_discover_type
(sock->sd, sock->mtu_discover_type, sock->info.af); >> + 
set_mtu_discover_type (sock->sd, sock->mtu_discover_type,
sock->info.lsa->bind_local->ai_family); > > ... on a client with
--nobind, this immediately crashes as lsa->bind_local > is not set... >
> Sat Nov 21 21:01:48 2015 OpenVPN 2.3_git
[git:master/48c23dc92006e1d8+] i686-pc-linux-gnu [SSL (OpenSSL)] [LZO]
[LZ4] [EPOLL] [MH] [IPv6] built on Nov 21 2015 > Sat Nov 21 21:01:48
2015 library versions: OpenSSL 1.0.2d 9 Jul 2015, LZO 2.08 > Sat Nov 21
21:01:48 2015 TCP/UDP: Preserving recently used remote address:
[AF_INET]199.102.77.82:51194 > Sat Nov 21 21:01:48 2015 Socket Buffers:
R=[110592->110592] S=[110592->110592] > > Program received signal
SIGSEGV, Segmentation fault. > 0x0809c165 in phase2_set_socket_flags
(sock=0x80ee270) > at ../../../openvpn/src/openvpn/socket.c:1673 >
1673  set_mtu_discover_type (sock->sd, sock->mtu_discover_type,
sock->info.lsa->bind_local->ai_family); > > (gdb) print
sock->info.lsa->bind_local > $1 = (struct addrinfo *) 0x0 > > > So,
overruling Arne's ACK and not applying it :-) > > > I wonder if the
*right* fix for this case wouldn't be to extend this > code block
(before calling phase2_set_socket_flags(), socket.c line 1880): >
>   if (sock->bind_local  && !sock->remote_host &&
sock->info.lsa->bind_local) > { >   /* Warn if
this is because neither v4 or v6 was specified >* and we
should not connect a remote */ >   if (sock->info.af ==
AF_UNSPEC) > msg (M_WARN, "Could not determine IPv4/IPv6
protocol. Using %s", > 
addr_family_name(sock->info.lsa->bind_local->ai_family)); >
>   create_socket (sock, sock->info.lsa->bind_local);
> } > > into... > >   if (sock->info.af ==
AF_UNSPEC) > +{ >msg (M_WARN, "Could not
determine IPv4/IPv6 protocol. Using %s", > 
addr_family_name(sock->info.lsa->bind_local->ai_family)); > +  
sock->info.af = sock->info.lsa->bind_local->ai_family; > +} > >
which would catch your case, but also the "--nobind and we have a
remote" > case (client). > > Arne, what do you think?

For clients you never get into that codeblock because a remote either
resolves (and then has an AF family or does not resolve). For a server
we also bail out if we cannot resolve the bind address (with :: or
0.0.0.0 being the default). The only case you can into that is with a
weird config like mtu-discovery.

@Christian, how did you manage to trigger the bug?

Arne
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (MingW32)

iEYEARECAAYFAlZQ4EsACgkQe8+cMNS4zRd+cACgxx/d2wY7GdyNkb+/YY93Vxoq
TZUAoOWgpd0OgR0BtbWhwwpU/ZYzRq8O
=4VJ/
-END PGP SIGNATURE-

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Gert Doering]

Hi,

On Tue, Nov 17, 2015 at 12:32:16PM +, Christian Pellegrin wrote:
> Looks better than just adding AF_UNSPEC case but I noted many other
> FIXMEs about AF_UNSPEC that could be solved this way. So let me know
> if there is any reason to not use the ai_family field (and just add
> the AF_UNSPEC case in set_mtu_discovery)

No, AF_UNSPEC in set_mtu_discovery is not the right thing (because it
would have to bei either a no-op, or try ipv4 *and* ipv6 to see which
one succeeds).  We really need to set info.af properly, as your patch
has a little side effect...

> +++ b/src/openvpn/socket.c
> @@ -1670,7 +1670,7 @@ phase2_set_socket_flags (struct link_socket* sock)
>  set_cloexec (sock->ctrl_sd);
>  
>/* set Path MTU discovery options on the socket */
> -  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, sock->info.af);
> +  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, 
> sock->info.lsa->bind_local->ai_family);

... on a client with --nobind, this immediately crashes as lsa->bind_local 
is not set...

Sat Nov 21 21:01:48 2015 OpenVPN 2.3_git [git:master/48c23dc92006e1d8+] 
i686-pc-linux-gnu [SSL (OpenSSL)] [LZO] [LZ4] [EPOLL] [MH] [IPv6] built on Nov 
21 2015
Sat Nov 21 21:01:48 2015 library versions: OpenSSL 1.0.2d 9 Jul 2015, LZO 2.08
Sat Nov 21 21:01:48 2015 TCP/UDP: Preserving recently used remote address: 
[AF_INET]199.102.77.82:51194
Sat Nov 21 21:01:48 2015 Socket Buffers: R=[110592->110592] S=[110592->110592]

Program received signal SIGSEGV, Segmentation fault.
0x0809c165 in phase2_set_socket_flags (sock=0x80ee270)
at ../../../openvpn/src/openvpn/socket.c:1673
1673  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, 
sock->info.lsa->bind_local->ai_family);

(gdb) print sock->info.lsa->bind_local
$1 = (struct addrinfo *) 0x0


So, overruling Arne's ACK and not applying it :-)


I wonder if the *right* fix for this case wouldn't be to extend this
code block (before calling phase2_set_socket_flags(), socket.c line 1880):

  if (sock->bind_local  && !sock->remote_host && 
sock->info.lsa->bind_local)
{
  /* Warn if this is because neither v4 or v6 was specified
   * and we should not connect a remote */
  if (sock->info.af == AF_UNSPEC)
msg (M_WARN, "Could not determine IPv4/IPv6 protocol. Using %s",
 addr_family_name(sock->info.lsa->bind_local->ai_family));

  create_socket (sock, sock->info.lsa->bind_local);
}

into...

  if (sock->info.af == AF_UNSPEC)
+   {
   msg (M_WARN, "Could not determine IPv4/IPv6 protocol. Using 
%s",
 addr_family_name(sock->info.lsa->bind_local->ai_family));
+  sock->info.af = sock->info.lsa->bind_local->ai_family;
+   }

which would catch your case, but also the "--nobind and we have a remote"
case (client).

Arne, what do you think?

gert

-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Gert Doering]

Hi,

On Tue, Nov 17, 2015 at 01:43:50PM +, Christian Pellegrin wrote:
> I also see: "Could not determine IPv4/IPv6 protocol. Using AF_INET" in
> the logs (which points in an quite explicative conditional in
> socket.c). The configuration entries that might affect this and that
> are present in my configuration are:
> 
> local [IPv4 address]
> lport 443
> mode server
> proto udp
> server [IPv4 subnet]
> topology subnet

I had the suspicion that this happens for the server side - "proto udp"
is the giveaway, as it will pass AF_UNSPEC to getaddrinfo() to get 
"what is needed for dual-stack"  (which does not always work, but that's
a different story).  So, starting out with AF_UNSPEC is good, but the
actual socket later on isn't (it is what getaddrinfo() returns)...

The client side should be sorted out quite nicely now, but seems the
server side could need some more polishing - so thanks for your patch,
and since it already got an ACK by our master of socket.c, I'll merge 
it ASAP.

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Christian Pellegrin]

On Tue, Nov 17, 2015 at 1:03 PM, Gert Doering  wrote:
> Why are you seeing AF_UNSPEC there?  This is actually more interesting
> to me right now than debating possible avenues to fix this :-)

I also see: "Could not determine IPv4/IPv6 protocol. Using AF_INET" in
the logs (which points in an quite explicative conditional in
socket.c). The configuration entries that might affect this and that
are present in my configuration are:

local [IPv4 address]
lport 443
mode server
proto udp
server [IPv4 subnet]
topology subnet

and this happens on a host where I configured just IPv4 (even if the
host itself is dual homed). Interestingly, if I configure full IPv6,
i.e. I add:

local [IPv6 address]
proto udp6
server [IPv6 netblock]

the problem (and of course above message) goes away. I mistakenly
assumed that seeing AF_UNSPEC is OK (from various comments when you
grep for AF_UNSPEC). If you think the root of all evil is this value,
I will dig further.

Thanks!

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Gert Doering]

Hi,

On Tue, Nov 17, 2015 at 12:32:16PM +, Christian Pellegrin wrote:
> Looks better than just adding AF_UNSPEC case but I noted many other
> FIXMEs about AF_UNSPEC that could be solved this way. So let me know
> if there is any reason to not use the ai_family field (and just add
> the AF_UNSPEC case in set_mtu_discovery)

Why are you seeing AF_UNSPEC there?  This is actually more interesting
to me right now than debating possible avenues to fix this :-)

gert
-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Arne Schwabe]

Am 17.11.15 um 13:32 schrieb Christian Pellegrin:
> Looks better than just adding AF_UNSPEC case but I noted many other
> FIXMEs about AF_UNSPEC that could be solved this way. So let me know
> if there is any reason to not use the ai_family field (and just add
> the AF_UNSPEC case in set_mtu_discovery)
> 

Still not very pretty but this is better than failing so ACK from me.

Arne

[Openvpn-devel] [PATCH] Fix info.af == AF_UNSPEC case after commit 2bed089d31a12c2d0277e36a64964ebab6640f75

[Christian Pellegrin]

Looks better than just adding AF_UNSPEC case but I noted many other
FIXMEs about AF_UNSPEC that could be solved this way. So let me know
if there is any reason to not use the ai_family field (and just add
the AF_UNSPEC case in set_mtu_discovery)

Thanks!
From 7044e2731eeafcc7f877c7d8b77676fb5d4dbd67 Mon Sep 17 00:00:00 2001
From: Christian Pellegrin 
Date: Tue, 17 Nov 2015 12:12:10 +
Subject: [PATCH] Fix info.af == AF_UNSPEC case after commit
 2bed089d31a12c2d0277e36a64964ebab6640f75

Signed-off-by: Christian Pellegrin 
---
 src/openvpn/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 3ef7279..c7885fd 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -1670,7 +1670,7 @@ phase2_set_socket_flags (struct link_socket* sock)
 set_cloexec (sock->ctrl_sd);
 
   /* set Path MTU discovery options on the socket */
-  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, sock->info.af);
+  set_mtu_discover_type (sock->sd, sock->mtu_discover_type, sock->info.lsa->bind_local->ai_family);
 
 #if EXTENDED_SOCKET_ERROR_CAPABILITY
   /* if the OS supports it, enable extended error passing on the socket */
-- 
2.6.0.rc2.230.g3dd15c0