if_ovpn: fix memory leak in VNET

[p.mousavizadeh_protonmail.com]

Summary

if_ovpn: fix memory leak in VNET
I noticed that when I unload the if_ovpn kernel module
on my server, it panics. after further investigation
I found that it doesn't detach its interfaces in VNETs
other than vnet0. Here is the patch.

Test Plan

You can use the openvpn application
to see it panicing, however, you can see the memory
leak warning by following commands below:

[root@ftsr1] # kldload if_ovpn
[root@ftsr1] # jail -c persist name=vnet1 mount.devfs devfs_ruleset=5 vnet
path=/
[root@ftsr1] [~] # jexec -l 1
[root@] [/] # ifconfig ovpn create
ovpn0
[root@] [/] # ifconfig ovpn0
ovpn0: flags=8010<POINTOPOINT,MULTICAST> metric 0 mtu 1428
options=80000<LINKSTATE>
groups: openvpn
nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
[root@] [/] # ifconfig ovpn0 up
[root@] [/] # exit
[root@ftsr1] [~] # kldunload if_ovpn
Warning: memory type ovpn leaked memory on destroy (2 allocations, 768 bytes leaked).

[glebius]

sys/net/if_ovpn.c

2864

Since this was created with VNET_SYSINIT(), it correct way to detach would be VNET_SYSUNINIT().

[glebius]

sys/net/if_ovpn.c

2864

Normally MOD_LOAD/MOD_UNLOAD is used to initialize function pointers, etc. The VNET creation/teardown should be with VNET_SYSINIT()/VNET_SYSUNINIT(). The framework already has VNET_FOREACH() iterator in it.

[kp]

I believe that was accidentally broken by 96b29c7f0cffd377a757ad8ccc0cdd8fcb96d0dd, which fixed the issue of jails being unable to go away while they still had ovpn interfaces in them. It fixed that, but also removed the VNET_SYSUNINIT that prevented this leak.

To be clear, there are two scenarios that we need to cover: the first is that the module goes away while jails (potentially with if_ovpn interfaces in them) still exist. That's the scenario this patch considers. The other is that the module remains, but jails are being destroyed. In that scenario we need the ovpn_prison_remove() callback to ensure the ovpn interaces are cleaned up and don't prevent the jail from being cleaned up). That scenario was considered in 96b29c7f0cffd377a757ad8ccc0cdd8fcb96d0dd.

I think this patch is correct as-is, but it'd be cleaner to restore the VNET_SYSUNINIT(vnet_ovpn_uninit) rather than handling it in MOD_UNLOAD.

[p.mousavizadeh_protonmail.com]

Ok, I will try to fix it using VNET_SYSUNINIT().

[markj]

.

sys/net/if_ovpn.c

2864

The problem with VNET_SYSUNINIT is that it doesn't run if an interface is holding onto a prison reference. SYSUNINITs run only after all jail references are gone. That's why we use the prison REMOVE method here: a user tries to destroy the jail; if_ovpn destroys its interfaces, releasing jail references; the jail teardown can continue and eventually run SYSUNINITs.

So, if we want MOD_UNLOAD to autodestruct all if_ovpn interfaces, I think the current approach makes sense. Probably this should share code with ovpn_prison_remove().

[glebius]

In D54175#1237688, @markj wrote:

So, if we want MOD_UNLOAD to autodestruct all if_ovpn interfaces, I think the current approach makes sense. Probably this should share code with ovpn_prison_remove().

IMHO, we shall not want that :) The approach taken by other systems is that MOD_UNLOAD returns EBUSY if the module is being in use. User is supposed to stop using the module first, then unload it. IMHO, we should not overcomplicate the code for the sake of automated destruction on unload. Unloading a module is something that is very very rarely exercised in production.

[markj]

In D54175#1237693, @glebius wrote:

In D54175#1237688, @markj wrote:

So, if we want MOD_UNLOAD to autodestruct all if_ovpn interfaces, I think the current approach makes sense. Probably this should share code with ovpn_prison_remove().

IMHO, we shall not want that :) The approach taken by other systems is that MOD_UNLOAD returns EBUSY if the module is being in use. User is supposed to stop using the module first, then unload it. IMHO, we should not overcomplicate the code for the sake of automated destruction on unload. Unloading a module is something that is very very rarely exercised in production.

That's what I thought, but other software ifnet implementations I looked at (gif, wg, bridge, epair) all seem to behave like this.

[glebius]

In D54175#1237696, @markj wrote:

That's what I thought, but other software ifnet implementations I looked at (gif, wg, bridge, epair) all seem to behave like this.

IMHO, it is a continuous copy-and-paste from one driver to the other and this chain needs to broken, rather than continued.