| Message ID | 20211117175424.17195-1-maximilian.fillinger@foxcrypto.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Don't "undo" ifconfig when given --ifconfig-noexec | expand |
Hi, On Wed, Nov 17, 2021 at 06:54:24PM +0100, Max Fillinger wrote: > When running with --ifconfig-noexec on Linux, OpenVPN may still delete > the ip address from the tun interface on exit, because it tries to undo > the ifconfig that did not actually happen. > > This commit reintroduces the did_ifconfig member to struct tuntap so > that we can check if ifconfig was actually done before trying to undo > it. It's behind an #ifdef because it's only used on Linux, and that was > the reason why it was removed before. > > Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> I'm sure that this will work, but I wonder why we need to do this in such a complicated way - the code here is close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) ... if (tt->did_ifconfig_setup) { undo_ifconfig_ipv4(tt, ctx); } if (tt->did_ifconfig_ipv6_setup) { undo_ifconfig_ipv6(tt, ctx); } ... so why is "did_ifconfig_setup" true, if ifconfig wasn't done? Or, phrased differently, what is did_ifconfig_setup used for, across the code, and can we just "not set it to true" if ifconfig-noexec is in effect? Or does it have nasty side effects? gert
> ... so why is "did_ifconfig_setup" true, if ifconfig wasn't done? > > Or, phrased differently, what is did_ifconfig_setup used for, across the > code, and can we just "not set it to true" if ifconfig-noexec is in > effect? Or does it have nasty side effects? tt->did_ifconfig_setup is set in init_tun, which sets up the tuntap struct but doesn't do ifconfig yet. This is executed independently of --ifconfig-noexec. There are probably some things to refactor there. Maybe we could remove did_ifconfig_setup and instead use did_ifconfig on all platforms. But to be honest, I'm scared of touching it. Also, could we backport it to 2.5? If so, isn't it better to keep the fix small?
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 75d5eaf7..32e739fc 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -1601,6 +1601,10 @@ do_ifconfig(struct tuntap *tt, const char *ifname, int tun_mtu, do_ifconfig_ipv6(tt, ifname, tun_mtu, es, ctx); } +#ifdef TARGET_LINUX + tt->did_ifconfig = true; +#endif + /* release resources potentially allocated during interface setup */ net_ctx_free(ctx); } @@ -2190,7 +2194,7 @@ close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx) { ASSERT(tt); - if (tt->type != DEV_TYPE_NULL) + if (tt->type != DEV_TYPE_NULL && tt->did_ifconfig) { if (tt->did_ifconfig_setup) { diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h index aa1e47b5..1f579e34 100644 --- a/src/openvpn/tun.h +++ b/src/openvpn/tun.h @@ -162,6 +162,9 @@ struct tuntap bool did_ifconfig_setup; bool did_ifconfig_ipv6_setup; +#ifdef TARGET_LINUX + bool did_ifconfig; +#endif bool persistent_if; /* if existed before, keep on program end */
When running with --ifconfig-noexec on Linux, OpenVPN may still delete the ip address from the tun interface on exit, because it tries to undo the ifconfig that did not actually happen. This commit reintroduces the did_ifconfig member to struct tuntap so that we can check if ifconfig was actually done before trying to undo it. It's behind an #ifdef because it's only used on Linux, and that was the reason why it was removed before. Signed-off-by: Max Fillinger <maximilian.fillinger@foxcrypto.com> --- src/openvpn/tun.c | 6 +++++- src/openvpn/tun.h | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-)