| Message ID | 20220629170555.116087-1-maximilian.fillinger@foxcrypto.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel,v5] Don't "undo" ifconfig on exit if it wasn't done | expand |
Hi, On 29/06/2022 19:05, Max Fillinger wrote: > When running with --ifconfig-noexec, OpenVPN does not execute ifconfig, > but on exit, it still tries to "undo" the configuration it would have > done. This patch fixes it by extracting an undo_ifconfig() function from > close_tun(). The undo function is called before close_tun(), but only if > --ifconfig-noexec isn't set. This is symmetric to how open_tun() and > do_ifconfig() are used. > > v2: Fix tabs-vs-spaces. > v3: Fix another style mistake. > v4: Move undo_ifconfig{4,6}() out of #ifdef TARGET_LINUX. > v5: Keep ctx argument in close_tun(). > > Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> > --- > src/openvpn/init.c | 4 ++ > src/openvpn/tun.c | 161 +++++++++++++++++++++++---------------------- > src/openvpn/tun.h | 8 +++ > 3 files changed, 96 insertions(+), 77 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index c9d05c31..f5ca2be8 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -1873,6 +1873,10 @@ do_close_tun_simple(struct context *c) > msg(D_CLOSE, "Closing TUN/TAP interface"); > if (c->c1.tuntap) > { > + if (!c->options.ifconfig_noexec) > + { > + undo_ifconfig(c->c1.tuntap, &c->net_ctx); > + } > close_tun(c->c1.tuntap, &c->net_ctx); > c->c1.tuntap = NULL; > } > diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c > index 5e7b8c49..feee83ef 100644 > --- a/src/openvpn/tun.c > +++ b/src/openvpn/tun.c > @@ -1605,6 +1605,89 @@ do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, > net_ctx_free(ctx); > } > > +static void > +undo_ifconfig_ipv4(struct tuntap *tt, openvpn_net_ctx_t *ctx) > +{ > +#if defined(TARGET_LINUX) > + int netbits = netmask_to_netbits2(tt->remote_netmask); > + > + if (is_tun_p2p(tt)) > + { > + if (net_addr_ptp_v4_del(ctx, tt->actual_name, &tt->local, > + &tt->remote_netmask) < 0) > + { > + msg(M_WARN, "Linux can't del IP from iface %s", > + tt->actual_name); > + } > + } > + else > + { > + if (net_addr_v4_del(ctx, tt->actual_name, &tt->local, netbits) < 0) > + { > + msg(M_WARN, "Linux can't del IP from iface %s", > + tt->actual_name); > + } > + } > +#elif !defined(_WIN32) /* if !defined(TARGET_LINUX) && !defined(_WIN32) */ > + struct argv argv = argv_new(); > + > + argv_printf(&argv, "%s %s 0.0.0.0", IFCONFIG_PATH, tt->actual_name); > + > + argv_msg(M_INFO, &argv); > + openvpn_execve_check(&argv, NULL, 0, "Generic ip addr del failed"); > + > + argv_free(&argv); > +#endif /* ifdef TARGET_LINUX */ The comment should contain the same guard as the first if, i.e. "if defined(TARGET_LINUX)" > + /* Empty for _WIN32. */ Indentation of comment is wrong > +} > + > +static void > +undo_ifconfig_ipv6(struct tuntap *tt, openvpn_net_ctx_t *ctx) > +{ > +#if defined(TARGET_LINUX) > + if (net_addr_v6_del(ctx, tt->actual_name, &tt->local_ipv6, > + tt->netbits_ipv6) < 0) > + { > + msg(M_WARN, "Linux can't del IPv6 from iface %s", tt->actual_name); > + } > +#elif !defined(_WIN32) /* if !defined(TARGET_LINUX) && !defined(_WIN32) */ > + struct gc_arena gc = gc_new(); > + const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc); > + struct argv argv = argv_new(); > + > + argv_printf(&argv, "%s %s del %s/%d", IFCONFIG_PATH, tt->actual_name, > + ifconfig_ipv6_local, tt->netbits_ipv6); > + > + argv_msg(M_INFO, &argv); > + openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed"); Error message is wrong, because it should say "Generic" as above. > + > + argv_free(&argv); > + gc_free(&gc); > +#endif /* ifdef TARGET_LINUX */ as above > + /* Empty for _WIN32. */ as above > +} > + > +void > +undo_ifconfig(struct tuntap *tt, openvpn_net_ctx_t *ctx) > +{ > + if (tt->type != DEV_TYPE_NULL) > + { > + if (tt->did_ifconfig_setup) > + { > + undo_ifconfig_ipv4(tt, ctx); > + } > + > + if (tt->did_ifconfig_ipv6_setup) > + { > + undo_ifconfig_ipv6(tt, ctx); > + } > + > + /* release resources potentially allocated during undo */ > + net_ctx_reset(ctx); > + } > + spurious empty line > +} > + > static void > clear_tuntap(struct tuntap *tuntap) > { > @@ -1846,7 +1929,7 @@ open_tun_generic(const char *dev, const char *dev_type, const char *dev_node, > msg(M_INFO, "DCO device %s opened", tunname); > } > else > -#endif > +#endif /* if defined(TARGET_LINUX) */ > { > if (!dynamic_opened) > { > @@ -2176,87 +2259,11 @@ tuncfg(const char *dev, const char *dev_type, const char *dev_node, > > #endif /* ENABLE_FEATURE_TUN_PERSIST */ > > -static void > -undo_ifconfig_ipv4(struct tuntap *tt, openvpn_net_ctx_t *ctx) > -{ > -#if defined(TARGET_LINUX) > - int netbits = netmask_to_netbits2(tt->remote_netmask); > - > - if (is_tun_p2p(tt)) > - { > - if (net_addr_ptp_v4_del(ctx, tt->actual_name, &tt->local, > - &tt->remote_netmask) < 0) > - { > - msg(M_WARN, "Linux can't del IP from iface %s", > - tt->actual_name); > - } > - } > - else > - { > - if (net_addr_v4_del(ctx, tt->actual_name, &tt->local, netbits) < 0) > - { > - msg(M_WARN, "Linux can't del IP from iface %s", > - tt->actual_name); > - } > - } > -#else /* ifndef TARGET_LINUX */ > - struct argv argv = argv_new(); > - > - argv_printf(&argv, "%s %s 0.0.0.0", IFCONFIG_PATH, tt->actual_name); > - > - argv_msg(M_INFO, &argv); > - openvpn_execve_check(&argv, NULL, 0, "Generic ip addr del failed"); > - > - argv_free(&argv); > -#endif /* ifdef TARGET_LINUX */ > -} > - > -static void > -undo_ifconfig_ipv6(struct tuntap *tt, openvpn_net_ctx_t *ctx) > -{ > -#if defined(TARGET_LINUX) > - if (net_addr_v6_del(ctx, tt->actual_name, &tt->local_ipv6, > - tt->netbits_ipv6) < 0) > - { > - msg(M_WARN, "Linux can't del IPv6 from iface %s", tt->actual_name); > - } > -#else /* ifndef TARGET_LINUX */ > - struct gc_arena gc = gc_new(); > - const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc); > - struct argv argv = argv_new(); > - > - argv_printf(&argv, "%s %s del %s/%d", IFCONFIG_PATH, tt->actual_name, > - ifconfig_ipv6_local, tt->netbits_ipv6); > - > - argv_msg(M_INFO, &argv); > - openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed"); > - > - argv_free(&argv); > - gc_free(&gc); > -#endif /* ifdef TARGET_LINUX */ > -} > - > void > close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) > { > ASSERT(tt); > > - if (tt->type != DEV_TYPE_NULL) > - { > - if (tt->did_ifconfig_setup) > - { > - undo_ifconfig_ipv4(tt, ctx); > - } > - > - if (tt->did_ifconfig_ipv6_setup) > - { > - undo_ifconfig_ipv6(tt, ctx); > - } > - > - /* release resources potentially allocated during undo */ > - net_ctx_reset(ctx); > - } > - > #ifdef TARGET_LINUX > if (!tt->options.disable_dco) > { > diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h > index cf02bf43..6d4530f7 100644 > --- a/src/openvpn/tun.h > +++ b/src/openvpn/tun.h > @@ -300,6 +300,14 @@ void do_ifconfig_setenv(const struct tuntap *tt, > void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, > const struct env_set *es, openvpn_net_ctx_t *ctx); > > +/** > + * undo_ifconfig - undo configuration of the tunnel interface > + * > + * @param tt the tuntap interface context > + * @param ctx the networking API opaque context > + */ > +void undo_ifconfig(struct tuntap *tt, openvpn_net_ctx_t *ctx); > + > bool is_dev_type(const char *dev, const char *dev_type, const char *match_type); > > int dev_type_enum(const char *dev, const char *dev_type); Other than the nitpicks above, it looks good to me. On top of that, most of the issues I highlighted already exist in the current code (it is just being moved). But this could be the chance to fix those on the fly. Acked-by: Antonio Quartulli <a@unstable.cc> Note: this change still requires more testing with non-linux/non-windows platforms. Cheers,
HI, On Wed, Jun 29, 2022 at 07:05:55PM +0200, Max Fillinger wrote: > When running with --ifconfig-noexec, OpenVPN does not execute ifconfig, > but on exit, it still tries to "undo" the configuration it would have > done. This patch fixes it by extracting an undo_ifconfig() function from > close_tun(). The undo function is called before close_tun(), but only if > --ifconfig-noexec isn't set. This is symmetric to how open_tun() and > do_ifconfig() are used. > > v2: Fix tabs-vs-spaces. > v3: Fix another style mistake. > v4: Move undo_ifconfig{4,6}() out of #ifdef TARGET_LINUX. > v5: Keep ctx argument in close_tun(). I did what I promised too many weeks ago, and pushed this change towards the buildbot army... alas, they did what I was afraid they would do: complained. +-DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -Wall -g -O2 -std=c99 -MT +tun.o -MD -MP -MF .deps/tun.Tpo -c -o tun.o tun.c tun.c:1656:73: error: passing 'struct gc_arena' to parameter of incompatible +type 'struct gc_arena *'; take the address with & const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc); ^~ & ./socket.h:391:88: note: passing argument to parameter 'gc' here const char *print_in6_addr(struct in6_addr addr6, unsigned int flags, struct +gc_arena *gc); so the "non-linux" bits of undo_ifconfig_ipv6() have been broken since ever - but the patch now actually shows that to the compiler... > +#elif !defined(_WIN32) /* if !defined(TARGET_LINUX) && !defined(_WIN32) */ > + struct gc_arena gc = gc_new(); > + const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc); > + struct argv argv = argv_new(); ... and inded, this needs to be "&gc"... Can you please send a v6 that changes this, and also rebases against master? The context has changed a bit, and one hunk (#endif /* comment */) is no longer relevant, as that #endif never came into existance :-) We should then be able to proceed more quickly... thanks, gert
I had to apply a bit of pressure to get the patch to apply - it had
one hunk trying to fix an #endif which never existed :-) - and the context
of the last tun.c hunk needed adjustment (if (tun_dco_enabled(tt))) -
both fairly trivial, and no actual code change.
Pushed to the new and renovated buildbot army, to see if it would break
any platforms (very convenient, this ;-) ).
The change is as we have discussed, and I still think it's a good and
clean way forward. The "undo_ifconfig_ipv4() / _ipv6()" functions
are as they have been before (except that they now get compiled for
all platforms except WIN32 - the "linux only" part was a 2.5.0 oversight),
and the calls moved 1:1 from (TARGET_LINUX) close_tun() to a new
undo_ifconfig() function. tun.c is still a mess, but this helps a bit.
While staring at the diff, I kicked a spurious blank line from the
end of undo_ifconfig()... :-)
Your patch has been applied to the master branch.
commit 054f60fc5eaeacb90333e6b5e183459eb39a6d4f
Author: Max Fillinger
Date: Wed Jun 29 19:05:55 2022 +0200
Don't undo ifconfig on exit if it wasn't done
Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20220629170555.116087-1-maximilian.fillinger@foxcrypto.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24610.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
Hi, On Fri, Aug 19, 2022 at 07:04:42PM +0200, Gert Doering wrote: > I had to apply a bit of pressure to get the patch to apply - it had > one hunk trying to fix an #endif which never existed :-) - and the context > of the last tun.c hunk needed adjustment (if (tun_dco_enabled(tt))) - > both fairly trivial, and no actual code change. "This mail escaped". Of course I have only applied v6, but I had the mail for v5 still sitting around, and never sent it when I discovered that &gc was missing the "&"... > commit 054f60fc5eaeacb90333e6b5e183459eb39a6d4f This commit does not exist (outside of my tree). gert
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index c9d05c31..f5ca2be8 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -1873,6 +1873,10 @@ do_close_tun_simple(struct context *c) msg(D_CLOSE, "Closing TUN/TAP interface"); if (c->c1.tuntap) { + if (!c->options.ifconfig_noexec) + { + undo_ifconfig(c->c1.tuntap, &c->net_ctx); + } close_tun(c->c1.tuntap, &c->net_ctx); c->c1.tuntap = NULL; } diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 5e7b8c49..feee83ef 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -1605,6 +1605,89 @@ do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, net_ctx_free(ctx); } +static void +undo_ifconfig_ipv4(struct tuntap *tt, openvpn_net_ctx_t *ctx) +{ +#if defined(TARGET_LINUX) + int netbits = netmask_to_netbits2(tt->remote_netmask); + + if (is_tun_p2p(tt)) + { + if (net_addr_ptp_v4_del(ctx, tt->actual_name, &tt->local, + &tt->remote_netmask) < 0) + { + msg(M_WARN, "Linux can't del IP from iface %s", + tt->actual_name); + } + } + else + { + if (net_addr_v4_del(ctx, tt->actual_name, &tt->local, netbits) < 0) + { + msg(M_WARN, "Linux can't del IP from iface %s", + tt->actual_name); + } + } +#elif !defined(_WIN32) /* if !defined(TARGET_LINUX) && !defined(_WIN32) */ + struct argv argv = argv_new(); + + argv_printf(&argv, "%s %s 0.0.0.0", IFCONFIG_PATH, tt->actual_name); + + argv_msg(M_INFO, &argv); + openvpn_execve_check(&argv, NULL, 0, "Generic ip addr del failed"); + + argv_free(&argv); +#endif /* ifdef TARGET_LINUX */ + /* Empty for _WIN32. */ +} + +static void +undo_ifconfig_ipv6(struct tuntap *tt, openvpn_net_ctx_t *ctx) +{ +#if defined(TARGET_LINUX) + if (net_addr_v6_del(ctx, tt->actual_name, &tt->local_ipv6, + tt->netbits_ipv6) < 0) + { + msg(M_WARN, "Linux can't del IPv6 from iface %s", tt->actual_name); + } +#elif !defined(_WIN32) /* if !defined(TARGET_LINUX) && !defined(_WIN32) */ + struct gc_arena gc = gc_new(); + const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc); + struct argv argv = argv_new(); + + argv_printf(&argv, "%s %s del %s/%d", IFCONFIG_PATH, tt->actual_name, + ifconfig_ipv6_local, tt->netbits_ipv6); + + argv_msg(M_INFO, &argv); + openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed"); + + argv_free(&argv); + gc_free(&gc); +#endif /* ifdef TARGET_LINUX */ + /* Empty for _WIN32. */ +} + +void +undo_ifconfig(struct tuntap *tt, openvpn_net_ctx_t *ctx) +{ + if (tt->type != DEV_TYPE_NULL) + { + if (tt->did_ifconfig_setup) + { + undo_ifconfig_ipv4(tt, ctx); + } + + if (tt->did_ifconfig_ipv6_setup) + { + undo_ifconfig_ipv6(tt, ctx); + } + + /* release resources potentially allocated during undo */ + net_ctx_reset(ctx); + } + +} + static void clear_tuntap(struct tuntap *tuntap) { @@ -1846,7 +1929,7 @@ open_tun_generic(const char *dev, const char *dev_type, const char *dev_node, msg(M_INFO, "DCO device %s opened", tunname); } else -#endif +#endif /* if defined(TARGET_LINUX) */ { if (!dynamic_opened) { @@ -2176,87 +2259,11 @@ tuncfg(const char *dev, const char *dev_type, const char *dev_node, #endif /* ENABLE_FEATURE_TUN_PERSIST */ -static void -undo_ifconfig_ipv4(struct tuntap *tt, openvpn_net_ctx_t *ctx) -{ -#if defined(TARGET_LINUX) - int netbits = netmask_to_netbits2(tt->remote_netmask); - - if (is_tun_p2p(tt)) - { - if (net_addr_ptp_v4_del(ctx, tt->actual_name, &tt->local, - &tt->remote_netmask) < 0) - { - msg(M_WARN, "Linux can't del IP from iface %s", - tt->actual_name); - } - } - else - { - if (net_addr_v4_del(ctx, tt->actual_name, &tt->local, netbits) < 0) - { - msg(M_WARN, "Linux can't del IP from iface %s", - tt->actual_name); - } - } -#else /* ifndef TARGET_LINUX */ - struct argv argv = argv_new(); - - argv_printf(&argv, "%s %s 0.0.0.0", IFCONFIG_PATH, tt->actual_name); - - argv_msg(M_INFO, &argv); - openvpn_execve_check(&argv, NULL, 0, "Generic ip addr del failed"); - - argv_free(&argv); -#endif /* ifdef TARGET_LINUX */ -} - -static void -undo_ifconfig_ipv6(struct tuntap *tt, openvpn_net_ctx_t *ctx) -{ -#if defined(TARGET_LINUX) - if (net_addr_v6_del(ctx, tt->actual_name, &tt->local_ipv6, - tt->netbits_ipv6) < 0) - { - msg(M_WARN, "Linux can't del IPv6 from iface %s", tt->actual_name); - } -#else /* ifndef TARGET_LINUX */ - struct gc_arena gc = gc_new(); - const char *ifconfig_ipv6_local = print_in6_addr(tt->local_ipv6, 0, gc); - struct argv argv = argv_new(); - - argv_printf(&argv, "%s %s del %s/%d", IFCONFIG_PATH, tt->actual_name, - ifconfig_ipv6_local, tt->netbits_ipv6); - - argv_msg(M_INFO, &argv); - openvpn_execve_check(&argv, NULL, 0, "Linux ip -6 addr del failed"); - - argv_free(&argv); - gc_free(&gc); -#endif /* ifdef TARGET_LINUX */ -} - void close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) { ASSERT(tt); - if (tt->type != DEV_TYPE_NULL) - { - if (tt->did_ifconfig_setup) - { - undo_ifconfig_ipv4(tt, ctx); - } - - if (tt->did_ifconfig_ipv6_setup) - { - undo_ifconfig_ipv6(tt, ctx); - } - - /* release resources potentially allocated during undo */ - net_ctx_reset(ctx); - } - #ifdef TARGET_LINUX if (!tt->options.disable_dco) { diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index cf02bf43..6d4530f7 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -300,6 +300,14 @@ void do_ifconfig_setenv(const struct tuntap *tt, void do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, const struct env_set *es, openvpn_net_ctx_t *ctx); +/** + * undo_ifconfig - undo configuration of the tunnel interface + * + * @param tt the tuntap interface context + * @param ctx the networking API opaque context + */ +void undo_ifconfig(struct tuntap *tt, openvpn_net_ctx_t *ctx); + bool is_dev_type(const char *dev, const char *dev_type, const char *match_type); int dev_type_enum(const char *dev, const char *dev_type);
When running with --ifconfig-noexec, OpenVPN does not execute ifconfig, but on exit, it still tries to "undo" the configuration it would have done. This patch fixes it by extracting an undo_ifconfig() function from close_tun(). The undo function is called before close_tun(), but only if --ifconfig-noexec isn't set. This is symmetric to how open_tun() and do_ifconfig() are used. v2: Fix tabs-vs-spaces. v3: Fix another style mistake. v4: Move undo_ifconfig{4,6}() out of #ifdef TARGET_LINUX. v5: Keep ctx argument in close_tun(). Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> --- src/openvpn/init.c | 4 ++ src/openvpn/tun.c | 161 +++++++++++++++++++++++---------------------- src/openvpn/tun.h | 8 +++ 3 files changed, 96 insertions(+), 77 deletions(-)