Re: [Openvpn-devel] RFC: tun/tap cleanup at program end

[Eric Crist]

On Aug 15, 2012, at 05:53:40, Gert Doering  wrote:

> Hi,
> 
> On Wed, Aug 15, 2012 at 12:00:12PM +0200, Gert Doering wrote:
>>  3 - check for the existance of "--dev tap3" and remember, not cleaning
>>  if it existed previously, doing this with RT_NETLINK which should
>>  be sufficiently portable across all BSDs.  Same advantage as "2",
>>  hopefully much nicer implementation.
> 
> Here we go.  I discovered if_nametoindex(), which is really handy for
> this :-)
> 
> Eric, please test whether this solves your issues.
> 
> David, please do *not yet* commit, even if someone ACKs - I need to test
> this on OpenBSD, NetBSD and FreeBSD 9 as well.  So far, only tested on 
> FreeBSD 7.4 (and works).

I'm comfortable giving my ACK to this patch.

-
Eric F Crist

Re: [Openvpn-devel] RFC: tun/tap cleanup at program end

[Gert Doering]

Hi,

On Wed, Aug 15, 2012 at 12:00:12PM +0200, Gert Doering wrote:
>   3 - check for the existance of "--dev tap3" and remember, not cleaning
>   if it existed previously, doing this with RT_NETLINK which should
>   be sufficiently portable across all BSDs.  Same advantage as "2",
>   hopefully much nicer implementation.

Here we go.  I discovered if_nametoindex(), which is really handy for
this :-)

Eric, please test whether this solves your issues.

David, please do *not yet* commit, even if someone ACKs - I need to test
this on OpenBSD, NetBSD and FreeBSD 9 as well.  So far, only tested on 
FreeBSD 7.4 (and works).

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
From 57c8532499a487f41ecabbcf9cb23c64ac82f32d Mon Sep 17 00:00:00 2001
From: Gert Doering 
List-Post: openvpn-devel@lists.sourceforge.net
Date: Wed, 15 Aug 2012 13:51:30 +0300
Subject: [PATCH] Keep pre-existing tun/tap devices around on *BSD

This amends commit 62c613d46dc49 to check whether a named tun/tap
device ("--dev tunX" instead of "--dev tun") exists before OpenVPN
started - if yes, keep around at program end.  If no, destroy.

Tested on FreeBSD 7.4.

Also has a spelling fix, and change clear_tuntap() to be "static"
(only ever called from within tun.c).

Signed-off-by: Gert Doering 
---
 src/openvpn/tun.c |   22 +-
 src/openvpn/tun.h |4 +++-
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 6218b73..ed67261 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -890,7 +890,7 @@ do_ifconfig (struct tuntap *tt,
 #elif defined(TARGET_OPENBSD)
 
   /*
-   * On OpenBSD, tun interfaces are persistant if created with
+   * On OpenBSD, tun interfaces are persistent if created with
* "ifconfig tunX create", and auto-destroyed if created by
* opening "/dev/tunX" (so we just use the /dev/tunX)
*/
@@ -1235,7 +1235,7 @@ do_ifconfig (struct tuntap *tt,
   gc_free ();
 }
 
-void
+static void
 clear_tuntap (struct tuntap *tuntap)
 {
   CLEAR (*tuntap);
@@ -1344,6 +1344,13 @@ open_tun_generic (const char *dev, const char *dev_type, 
const char *dev_node,
 
   if (!dynamic_opened)
{
+ /* has named device existed before? if so, don't destroy at end */
+ if ( if_nametoindex( dev ) > 0 )
+   {
+ msg (M_INFO, "TUN/TAP device %s exists previously, keep at 
program end", dev );
+ tt->persistent_if = true;
+   }
+
  if ((tt->fd = open (tunname, O_RDWR)) < 0)
msg (M_ERR, "Cannot open TUN/TAP dev %s", tunname);
}
@@ -2030,7 +2037,7 @@ close_tun (struct tuntap* tt)
 {
   /* only *TAP* devices need destroying, tun devices auto-self-destruct
*/
-  if (tt && tt->type == DEV_TYPE_TUN )
+  if (tt && (tt->type == DEV_TYPE_TUN || tt->persistent_if ) )
 {
   close_tun_generic (tt);
   free(tt);
@@ -2165,7 +2172,7 @@ close_tun (struct tuntap *tt)
 {
   /* only tun devices need destroying, tap devices auto-self-destruct
*/
-  if (tt && tt->type != DEV_TYPE_TUN )
+  if (tt && ( tt->type != DEV_TYPE_TUN || tt->persistent_if ) )
 {
   close_tun_generic (tt);
   free(tt);
@@ -2303,7 +2310,12 @@ open_tun (const char *dev, const char *dev_type, const 
char *dev_node, struct tu
 void
 close_tun (struct tuntap *tt)
 {
-  if (tt)
+  if (tt && tt->persistent_if )/* keep pre-existing if around 
*/
+{
+  close_tun_generic (tt);
+  free (tt);
+}
+  else if (tt) /* close and destroy */
 {
   struct gc_arena gc = gc_new ();
   struct argv argv;
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index 9bd990f..8622bf8 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -137,6 +137,8 @@ struct tuntap
 
   bool ipv6;
 
+  bool persistent_if;  /* if existed before, keep on program end */
+
   struct tuntap_options options; /* options set on command line */
 
   char *actual_name; /* actual name of TUN/TAP dev, usually including unit 
number */
@@ -201,7 +203,7 @@ tuntap_defined (const struct tuntap *tt)
  * Function prototypes
  */
 
-void clear_tuntap (struct tuntap *tuntap);
+static void clear_tuntap (struct tuntap *tuntap);
 
 void open_tun (const char *dev, const char *dev_type, const char *dev_node,
   struct tuntap *tt);
-- 
1.7.3.5



pgpmkcnfyI47n.pgp
Description: PGP signature

Re: [Openvpn-devel] RFC: tun/tap cleanup at program end

[Gert Doering]

Hi,

On Tue, Aug 14, 2012 at 09:47:36PM +0200, Gert Doering wrote:
> I can see two options how to solve this:
> 
> 1 - set a flag if a device has been opened using a dynamic number 
> ("--dev tap", which hunts for the first-free tapN device), and 
> if yes, destroy the device on tun_close().  
> 
> This would leave devices lingering around if you call openvpn with 
> "--dev tap3" and there has not been a "tap3" before - but this could 
> be documented as "intended behaviour", and it would be fairly 
> straightforward to implement.
> 
> 2 - actually check for "--dev tap3" whether it exists before (by calling
> "ifconfig tap3" and checking the error return code - accessing /dev/tap3
> will not work, as that *always* springs the device to life), and 
> destroy devices that we newly created, while leaving alone devices that
> have been there before.
> 
> This will have the most nice user experience ("always leave the system
> as it was before"), but the implementation would mess up tun.c somewhat
> more.

  3 - check for the existance of "--dev tap3" and remember, not cleaning
  if it existed previously, doing this with RT_NETLINK which should
  be sufficiently portable across all BSDs.  Same advantage as "2",
  hopefully much nicer implementation.

I'll see whether I can get "3" done in sufficiently nice code...

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


pgpdxNGarcoXw.pgp
Description: PGP signature

[Openvpn-devel] RFC: tun/tap cleanup at program end

[Gert Doering]

Hi,

background: I spent quite some time in the 2.3 development cycle in
ensuring that OpenVPN quits nicely, leaving nothing behind - specifically,
this meant "make sure that all tun/tap interfaces we have created are
destroyed as well" - which is now breaking some people's setups :-/

On FreeBSD, the interaction between /dev/tap device nodes and the
"ifconfig" devices is as follows (same for tunX):

Case A: no tapX in the system yet, --dev tapX

 - we open the /dev/tunX device, get a file descriptor
 - the operating system auto-instanciates tunX
 - we call ifconfig tunX
 - at the end of the program, we close the file descriptor
 - to remove the tunX device again, we call "ifconfig tunX destroy"

Case B: there is an pre-existing tapX device, bound to a bridge

 - we do everything as in case A
 - at the end of the program, the tapX device is destroyed
 - on the next start of openvpn, tapX is recreated, and no longer part of
   the bridge

(the other BSDs are slightly different, of course, but the underlying
issue is the same)


In OpenVPN 2.2, "case A" used to leak devices - that is, the tapX device 
stuck behind after OpenVPN exited, which I consider bad behaviour.  OTOH, 
"case B" worked (somewhat by accident, as *OpenBSD* used to have the 
same issue of lingering devices, and OpenVPN worked around it by always 
calling "ifconfig tapX destroy" on startup...)

In OpenVPN 2.3, "case A" is fixed up, but now "case B" is broken, and I'm 
not exactly sure how to fix it - obviously, fixed it must be, as starting
and stopping should not permanently alter the system state unless it's
asked for (--mktun / --rmtun).


I can see two options how to solve this:

1 - set a flag if a device has been opened using a dynamic number 
("--dev tap", which hunts for the first-free tapN device), and 
if yes, destroy the device on tun_close().  

This would leave devices lingering around if you call openvpn with 
"--dev tap3" and there has not been a "tap3" before - but this could 
be documented as "intended behaviour", and it would be fairly 
straightforward to implement.

2 - actually check for "--dev tap3" whether it exists before (by calling
"ifconfig tap3" and checking the error return code - accessing /dev/tap3
will not work, as that *always* springs the device to life), and 
destroy devices that we newly created, while leaving alone devices that
have been there before.

This will have the most nice user experience ("always leave the system
as it was before"), but the implementation would mess up tun.c somewhat
more.

So I'd opt for variant 1...

(As this needs to go into 2.3, "rewrite all of tun.c" or "use RT_NETLINK"
is out)

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


pgpucpnaLap1h.pgp
Description: PGP signature