Skip to contentSkip to conversation

tun cleanups

PATCH In review Alon Bar-Lev 2 participants 8 messages May 12, 2012 – Jun 18, 2012 1 message not in the archive

lore
Share & tools
lore
#0 lore raw

cover tun cleanups

Platform independent interface for tun provider.

Split the long tun.c into platform specific files using tun_engine interface.

Functionality is the same.

Maintenance will be much easier! new options, like stacking several interfaces and delegate partial control to plugin.

There is more to be done in order to clean up the code, however this is a good starting point.

Some more minor cleanups.

Tested on Linux, FreeBSD, Windows.

Branch should be merge after syshead[1].
Most probably patch exceeds list limitation, review is available[2].

This is for 2.4, please review quickly this so won't need to rebase.

Special thanks to Eric Crist for his FreeBSD help.

[1] https://github.com/alonbl/openvpn/compare/build...syshead
[2] https://github.com/alonbl/openvpn/compare/syshead...tun

Alon Bar-Lev (4):
cleanup: remove check_subnet_conflict
cleanup: move warn_on_use_of_common_subnets to init.c
cleanup: remove tun_adjust_frame_parameters
cleanup: tun: tun_engine interface

 configure.ac                             |    6 +
 src/openvpn/Makefile.am                  |   12 +
 src/openvpn/error.c                      |    2 +-
 src/openvpn/forward.c                    |   28 +-
 src/openvpn/helper.c                     |    2 +-
 src/openvpn/init.c                       |   61 +-
 src/openvpn/multi.c                      |    2 +-
 src/openvpn/options.c                    |   20 +-
 src/openvpn/options.h                    |    5 +-
 src/openvpn/route.c                      |    2 +-
 src/openvpn/sig.c                        |    4 +-
 src/openvpn/syshead.h                    |   56 -
 src/openvpn/tun-engine-common-bsd.c      |  103 +
 src/openvpn/tun-engine-common-bsd.h      |   34 +
 src/openvpn/tun-engine-common.c          |  518 +++
 src/openvpn/tun-engine-common.h          |   85 +
 src/openvpn/tun-engine-darwin.c          |  226 ++
 src/openvpn/tun-engine-dragonfly.c       |  193 ++
 src/openvpn/tun-engine-freebsd.c         |  229 ++
 src/openvpn/tun-engine-generic.c         |   97 +
 src/openvpn/tun-engine-linux-options.h   |   37 +
 src/openvpn/tun-engine-linux.c           |  515 +++
 src/openvpn/tun-engine-netbsd.c          |  318 ++
 src/openvpn/tun-engine-openbsd.c         |  259 ++
 src/openvpn/tun-engine-options.h         |   37 +
 src/openvpn/tun-engine-solaris.c         |  491 +++
 src/openvpn/tun-engine-windows-options.h |   92 +
 src/openvpn/tun-engine-windows-util.h    |   83 +
 src/openvpn/tun-engine-windows.c         | 2757 ++++++++++++++++
 src/openvpn/tun-engine.h                 |   95 +
 src/openvpn/tun.c                        | 5053 ++----------------------------
 src/openvpn/tun.h                        |  469 +---
 32 files changed, 6548 insertions(+), 5343 deletions(-)
 create mode 100644 src/openvpn/tun-engine-common-bsd.c
 create mode 100644 src/openvpn/tun-engine-common-bsd.h
 create mode 100644 src/openvpn/tun-engine-common.c
 create mode 100644 src/openvpn/tun-engine-common.h
 create mode 100644 src/openvpn/tun-engine-darwin.c
 create mode 100644 src/openvpn/tun-engine-dragonfly.c
 create mode 100644 src/openvpn/tun-engine-freebsd.c
 create mode 100644 src/openvpn/tun-engine-generic.c
 create mode 100644 src/openvpn/tun-engine-linux-options.h
 create mode 100644 src/openvpn/tun-engine-linux.c
 create mode 100644 src/openvpn/tun-engine-netbsd.c
 create mode 100644 src/openvpn/tun-engine-openbsd.c
 create mode 100644 src/openvpn/tun-engine-options.h
 create mode 100644 src/openvpn/tun-engine-solaris.c
 create mode 100644 src/openvpn/tun-engine-windows-options.h
 create mode 100644 src/openvpn/tun-engine-windows-util.h
 create mode 100644 src/openvpn/tun-engine-windows.c
 create mode 100644 src/openvpn/tun-engine.h
Signature · 1 line
1.7.3.4
#1 lore raw

PATCH 1/4 cleanup: remove check_subnet_conflict

Commeted out as "too many false positives".

---
 src/openvpn/route.c |    1 -
 src/openvpn/tun.c   |   43 -------------------------------------------
 src/openvpn/tun.h   |    4 ----
 3 files changed, 0 insertions(+), 48 deletions(-)
src/openvpn/route.c +0 -1
index 7c25c77..f36c324 100644
@@ -1030,7 +1030,6 @@ add_routes (struct route_list *rl, struct route_ipv6_list *rl6, const struct tun
       for (i = 0; i < rl->n; ++i)
 	{
 	  struct route *r = &rl->routes[i];
-	  check_subnet_conflict (r->network, r->netmask, "route");
 	  if (flags & ROUTE_DELETE_FIRST)
 	    delete_route (r, tt, flags, &rl->rgi, es);
 	  add_route (r, tt, flags, &rl->rgi, es);
src/openvpn/tun.c +0 -43

55 lines not on this page · raw message

src/openvpn/tun.h +0 -4
index 9bd990f..bea1554 100644
@@ -251,10 +251,6 @@ const char *ifconfig_options_string (const struct tuntap* tt, bool remote, bool
 
 bool is_tun_p2p (const struct tuntap *tt);
 
-void check_subnet_conflict (const in_addr_t ip,
-			    const in_addr_t netmask,
-			    const char *prefix);
-
 void warn_on_use_of_common_subnets (void);
 
 /*
Signature · 1 line
1.7.3.4
#2 lore raw

PATCH 2/4 cleanup: move warn_on_use_of_common_subnets to init.c

In reply to Alon Bar-Lev

No dependencies and init.c is the only user.

---
 src/openvpn/init.c |   18 ++++++++++++++++++
 src/openvpn/tun.c  |   17 -----------------
 src/openvpn/tun.h  |    2 --
 3 files changed, 18 insertions(+), 19 deletions(-)
src/openvpn/init.c +18 -0
index 61ced5d..eb4c5df 100644
@@ -59,6 +59,24 @@ static struct context *static_context; /* GLOBAL */
 
 static void do_init_first_time (struct context *c);
 
+static
+void
+warn_on_use_of_common_subnets (void)
+{
+  struct gc_arena gc = gc_new ();
+  struct route_gateway_info rgi;
+  const int needed = (RGI_ADDR_DEFINED|RGI_NETMASK_DEFINED);
+
+  get_default_gateway (&rgi);
+  if ((rgi.flags & needed) == needed)
+    {
+      const in_addr_t lan_network = rgi.gateway.addr & rgi.gateway.netmask;
+      if (lan_network == 0xC0A80000 || lan_network == 0xC0A80100)
+	msg (M_WARN, "NOTE: your local LAN uses the extremely common subnet address 192.168.0.x or 192.168.1.x.  Be aware that this might create routing conflicts if you connect to the VPN server from public locations such as internet cafes that use the same subnet.");
+    }
+  gc_free (&gc);
+}
+
 void
 context_clear (struct context *c)
 {
src/openvpn/tun.c +0 -17
index 033c1e2..31684f4 100644
@@ -242,23 +242,6 @@ check_addr_clash (const char *name,
   gc_free (&gc);
 }
 
-void
-warn_on_use_of_common_subnets (void)
-{
-  struct gc_arena gc = gc_new ();
-  struct route_gateway_info rgi;
-  const int needed = (RGI_ADDR_DEFINED|RGI_NETMASK_DEFINED);
-
-  get_default_gateway (&rgi);
-  if ((rgi.flags & needed) == needed)
-    {
-      const in_addr_t lan_network = rgi.gateway.addr & rgi.gateway.netmask;
-      if (lan_network == 0xC0A80000 || lan_network == 0xC0A80100)
-	msg (M_WARN, "NOTE: your local LAN uses the extremely common subnet address 192.168.0.x or 192.168.1.x.  Be aware that this might create routing conflicts if you connect to the VPN server from public locations such as internet cafes that use the same subnet.");
-    }
-  gc_free (&gc);
-}
-
 /*
  * Complain if --dev tap and --ifconfig is used on an OS for which
  * we don't have a custom tap ifconfig template below.
src/openvpn/tun.h +0 -2
index bea1554..7a8e769 100644
@@ -251,8 +251,6 @@ const char *ifconfig_options_string (const struct tuntap* tt, bool remote, bool
 
 bool is_tun_p2p (const struct tuntap *tt);
 
-void warn_on_use_of_common_subnets (void);
-
 /*
  * Inline functions
  */
Signature · 1 line
1.7.3.4
#3 lore raw

PATCH 3/4 cleanup: remove tun_adjust_frame_parameters

In reply to Alon Bar-Lev

It is a wrapper of frame_add_to_extra_tun, no sense to keep both around.

---
 src/openvpn/init.c |    2 +-
 src/openvpn/tun.h  |    6 ------
 2 files changed, 1 insertions(+), 7 deletions(-)
src/openvpn/init.c +1 -1
index eb4c5df..0bd3f75 100644
@@ -2437,7 +2437,7 @@ do_init_frame (struct context *c)
    * Adjust frame size based on the --tun-mtu-extra parameter.
    */
   if (c->options.ce.tun_mtu_extra_defined)
-    tun_adjust_frame_parameters (&c->c2.frame, c->options.ce.tun_mtu_extra);
+    frame_add_to_extra_tun (&c->c2.frame, c->options.ce.tun_mtu_extra);
 
   /*
    * Adjust frame size based on link socket parameters.
src/openvpn/tun.h +0 -6
index 7a8e769..f96bb50 100644
@@ -255,12 +255,6 @@ bool is_tun_p2p (const struct tuntap *tt);
  * Inline functions
  */
 
-static inline void
-tun_adjust_frame_parameters (struct frame* frame, int size)
-{
-  frame_add_to_extra_tun (frame, size);
-}
-
 /*
  * Should ifconfig be called before or after
  * tun dev open?
Signature · 1 line
1.7.3.4

A message that is not in the archive (sent off-list or not yet received): [email protected]...

#5 lore raw

Re: [Openvpn-devel] [V2.4 4/4] cleanup: tun: tun_engine interface

Quoted 53 lines · Alon Bar-Lev

Platform independent interface for tun provider.

Split the long tun.c into platform specific files using tun_engine interface.

There is more to be done in order to clean up the code, however this is a good starting point.

---
 configure.ac                             |    6 + src/openvpn/Makefile.am                  |   12 + src/openvpn/error.c                      |    2 +- src/openvpn/forward.c                    |   28 +- src/openvpn/helper.c                     |    2 +- src/openvpn/init.c                       |   41 +- src/openvpn/multi.c                      |    2 +- src/openvpn/options.c                    |   20 +- src/openvpn/options.h                    |    5 +- src/openvpn/route.c                      |    1 + src/openvpn/sig.c                        |    4 +- src/openvpn/syshead.h                    |   56 - src/openvpn/tun-engine-common-bsd.c      |  103 + src/openvpn/tun-engine-common-bsd.h      |   34 +

 src/openvpn/tun-engine-common.c          |  518 +++
 src/openvpn/tun-engine-common.h          |   85 +
 src/openvpn/tun-engine-darwin.c          |  226 ++

 src/openvpn/tun-engine-dragonfly.c       |  193 ++ src/openvpn/tun-engine-freebsd.c         |  229 ++ src/openvpn/tun-engine-generic.c         |   97 + src/openvpn/tun-engine-linux-options.h   |   37 + src/openvpn/tun-engine-linux.c           |  515 +++ src/openvpn/tun-engine-netbsd.c          |  318 ++

 src/openvpn/tun-engine-openbsd.c         |  259 ++
 src/openvpn/tun-engine-options.h         |   37 +
 src/openvpn/tun-engine-solaris.c         |  491 +++

 src/openvpn/tun-engine-windows-options.h |   92 + src/openvpn/tun-engine-windows-util.h    |   83 + src/openvpn/tun-engine-windows.c         | 2757 ++++++++++++++++ src/openvpn/tun-engine.h                 |   95 + src/openvpn/tun.c                        | 5019 ++---------------------------- src/openvpn/tun.h                        |  457 +---

 32 files changed, 6542 insertions(+), 5282 deletions(-)

 create mode 100644 src/openvpn/tun-engine-common-bsd.c create mode 100644 src/openvpn/tun-engine-common-bsd.h create mode 100644 src/openvpn/tun-engine-common.c create mode 100644 src/openvpn/tun-engine-common.h create mode 100644 src/openvpn/tun-engine-darwin.c create mode 100644 src/openvpn/tun-engine-dragonfly.c create mode 100644 src/openvpn/tun-engine-freebsd.c create mode 100644 src/openvpn/tun-engine-generic.c create mode 100644 src/openvpn/tun-engine-linux-options.h create mode 100644 src/openvpn/tun-engine-linux.c

Quoted 9 lines

 create mode 100644 src/openvpn/tun-engine-netbsd.c create mode 100644 src/openvpn/tun-engine-openbsd.c create mode 100644 src/openvpn/tun-engine-options.h create mode 100644 src/openvpn/tun-engine-solaris.c create mode 100644 src/openvpn/tun-engine-windows-options.h create mode 100644 src/openvpn/tun-engine-windows-util.h create mode 100644 src/openvpn/tun-engine-windows.c create mode 100644 src/openvpn/tun-engine.h

Too large for mailing list.
Please review at[1][2].

Alon.

[1] https://github.com/alonbl/openvpn/commit/c0c64bfa5369fb4abe7045a995fae2d33a34570f
[2] https://github.com/alonbl/openvpn/commit/c0c64bfa5369fb4abe7045a995fae2d33a34570f.patch

#6 lore raw
In reply to Alon Bar-Lev

Am 12.05.12 21:31, schrieb Alon Bar-Lev:

Quoted 10 lines

Platform independent interface for tun provider.

Split the long tun.c into platform specific files using tun_engine interface.

Functionality is the same.

Maintenance will be much easier! new options, like stacking several interfaces and delegate partial control to plugin.

I have looked at your patchset. It going in the right direction I think.

For "my platform" I would like to have/implement a persisent tun which works as follows:

- proivde tun configuration including routes and ifconfig
- open tun with the information
- keep tun open
- on reconect: compare new configuration with old configuration.
- if nothing has changed keep the old tun
- otherwise first open the new tun then close the old tun

If we redesign the tun interface I would like to support this as well. Having this interface would also make implementing platform specific redirect-gateway and so easier. Also the redirect variant used by Android (binding a socket to an interface) could be implemented as a alternative.

Arne

#7 lore raw
Quoted 6 lines · Arne Schwabe

Am 12.05.12 21:31, schrieb Alon Bar-Lev:

Quoted 5 lines

Platform independent interface for tun provider.

Split the long tun.c into platform specific files using tun_engine interface.

Quoted 6 lines
Quoted 5 lines

Functionality is the same.

Maintenance will be much easier! new options, like stacking several interfaces and delegate partial control to plugin.

I have looked at your patchset. It going in the right direction I think.

Thanks!
But as time goes by it becomes harder to rebase... and the 2.4 branch is not created... so I don't know what is the future of this patchset. Although I invested a lot of time in it and it has huge potential of eliminating duplicate code from the code base. I think this is simple enough to be in 2.3, but we had this discussion already.

Quoted 9 lines

For "my platform" I would like to have/implement a persisent tun which works as follows:

- proivde tun configuration including routes and ifconfig
- open tun with the information
- keep tun open
- on reconect: compare new configuration with old configuration.
 - if nothing has changed keep the old tun
 - otherwise first open the new tun then close the old tun

From the Android discussion I thought that the best will be to write a plugin "platform", delegate the ip configuration to the openvpn plugin interface.

Then you can implement whatever logic you need, if I understand correctly, delegate ip management to the UI.

Quoted 5 lines

If we redesign the tun interface I would like to support this as well. Having this interface would also make implementing platform specific redirect-gateway and so easier. Also the redirect variant used by Android (binding a socket to an interface) could be implemented as a alternative.

I think this should also be done in the plugin, using an inherited unix domain socket to pass the fd.
I explain this more at the Android thread if I remember correctly.

Thanks!
Alon.

#8 lore raw

Am 18.06.12 20:42, schrieb Alon Bar-Lev:

Quoted 29 lines · Arne Schwabe
Quoted 6 lines

Am 12.05.12 21:31, schrieb Alon Bar-Lev:

Quoted 5 lines

Platform independent interface for tun provider.

Split the long tun.c into platform specific files using tun_engine interface.

Quoted 6 lines
Quoted 5 lines

Functionality is the same.

Maintenance will be much easier! new options, like stacking several interfaces and delegate partial control to plugin.

I have looked at your patchset. It going in the right direction I think.

Thanks!
But as time goes by it becomes harder to rebase... and the 2.4 branch is not created... so I don't know what is the future of this patchset. Although I invested a lot of time in it and it has huge potential of eliminating duplicate code from the code base. I think this is simple enough to be in 2.3, but we had this discussion already.

Quoted 9 lines

For "my platform" I would like to have/implement a persisent tun which works as follows:

- proivde tun configuration including routes and ifconfig
- open tun with the information
- keep tun open
- on reconect: compare new configuration with old configuration.
- if nothing has changed keep the old tun
- otherwise first open the new tun then close the old tun

Quoted 7 lines

From the Android discussion I thought that the best will be to write a plugin "platform", delegate the ip configuration to the openvpn plugin interface.

Then you can implement whatever logic you need, if I understand correctly, delegate ip management to the UI.

Yes. I wanted to outline what I want to able to do so the tun framework can support what I am trying to do.

Arne