Created attachment 201965 [details] diff to /usr/src If a tun(4) interface is created on a VNET jail and renamed to the name other than "tunX", the interface cannot be destroyed thereafter. If it is renamed back to "tunX", it becomes destroyable again. This issue only occurs on VNET jails and doesn't seem to happen on a host or a/non-VNET environment. How to Reproduce: 1. Boot a VIMAGE-enabled FreeBSD system. (I used 12.0-RELEASE-p3 with GENERIC kernel. See attached log.txt) 2. Create a VNET jail. 3. Create a tun(4) interface on the VNET jail, then rename it to the name other than "tunX". 4. Try to destroy the tun interface on the VNET jail. Expected Result: Step 4 removes the interface successfully. Actual Result: Step 4 fails with "SIOCIFDESTROY: Invalid argument". Minimal Set of Commands to Reproduce: (Assume the second command creates 'tun0') jail -c -n test vnet persist jexec test ifconfig tun create jexec test ifconfig tun0 name wg0 jexec test ifconfig wg0 destroy Workaround: The problem can be worked around by renaming the undestroyable tun interface to "tunX" (X can be any number which is not being used by other tun interfaces on the system). jexec test ifconfig wg0 name tun0 jexec test ifconfig tun0 destroy Possible Fix: See attached patch for if_clone_destroy() in sys/net/if_clone.c. I wrote and tested it on releng/12.0 and confirmed it solved this particular problem, but because of my lack of knowledge and experience (this is my first kernel debugging, report and patch submission ever) I'm not sure it's reasonable and doesn't break any other stuff. Background: I found this issue when I was experimenting with the WireGuard VPN (net/wireguard package) on VNET jails. On FreeBSD, WireGuard uses a userspace program (net/wireguard-go) to create its tunnel interface. The program first generates a tun interface by opening /dev/tun, then renames it to the one specified on the command line. When I used 'wg0' as the WireGuard interface name on a VNET jail, I noticed that it couldn't be destroyed as 'service wireguard stop' got stuck waiting the interface to be destroyed. Further investigation shows me that it's not specific to WireGuard. Thus I create this report.
Created attachment 201966 [details] Reproduction test log
Excellent bug report. Bjoern knows this code a bit better than me and will hopefully be able to review your patch.
First, thank you so much for looking at my report. What follows is my humble analysis on the issue and my patch. Although this may be redundant or even inappropriate to include in bug reports, as a newbie, please let me summarize my thoughts. "ifconfig name destroy" issues a SIOCIFDESTROY ioctl on the interface "name", which finally calls if_clone_destroy(name) in sys/net/if_clone.c. https://svnweb.freebsd.org/base/releng/12.0/sys/net/if_clone.c?revision=340470&view=markup#l247 This function has two LIST_FOREACH loops which search for the 'cloner' used to create the interface "name". The first loop iterates the curvnet's cloner list and looks for a cloner whose name matches the driver name of the interface (ifp->if_dname. "tun" in this case). Because tun(4) seems to have only a single global cloner and doesn't seem to have per-VNET ones for some reason (related to the legacy devfs cloning functionality?), the cloner "tun" is not in the list and not found in the first loop. Thus the second loop is executed to look for the cloner "tun" in the global vnet0's cloner list. In the second loop, however, matching code uses the interface name (the function's argument "name". "wg0" in this case) instead of the driver name. As the interface name can be changed as seen in this report, there are cases where the second loop cannot find the cloner "tun" which is actually there. In those cases, the function exits with EINVAL, which matches the Actual Result of this report. On the other hand, a tun(4) interface on a host or non-VNET environment can be destroyed because the curvnet that the first loop iterates is the global vnet0's cloner list, which does contain the cloner "tun" and comparison is done against the driver name "tun" (not the interface name which can be changed to something like "wg0"). With those, I thought it's better for the second loop to use the same logic (comparing ifc->ifc_name with ifp->if_dname) as the first loop because the interface name (name) is changeable while the driver name (ifp->if_dname) seems not. I also noticed that with this change, it's no longer necessary to have separate matching codes depending on cloner types (SIMPLE or ADVANCED) because they are for matching full interface name including a unit number (and ADVANCED cloner even has a custom match function to handle non-starndard interface names such as epairXa/b or vlanX.Y) but now it only requires a simple, perfect match to perform lookups by driver name.
I've proposed a slightly different patch in https://reviews.freebsd.org/D19248
Thank you for working on this issue. I've briefly tested the patch and confirmed the reported issue was solved with it. I also confirmed that it changed the way unit numbers are assigned to if_tun. While if_tun's unit numbers are assigned sequentially across a host and all jails on it before the patch, it's now assigned independently so that vnet jails and the host can have interfaces with the same name. But now I began to wonder how the interfaces with the same name, say tun0, are related to /dev/tun0 on the system. When I did a quick testing with the following commands, the fourth command removed tun0 interface from the host and also deleted /dev/tun0 while tun0 interface on the vnet jail "test" is still there. Then the fifth command caused a kernel panic at destroy_devl in sys/kern/kern_conf.c. # Assume there's no if_tun on the system. jail -c -n test vnet persist ifconfig tun create jexec test ifconfig tun create ifconfig tun0 destroy jexec test ifconfig tun0 destroy
Thanks. You're right, the unit numbers are used to find the device node (or vice versa), so they need to be system-unique. I'll take a look at what we can do about that.
I've updated the patch on https://reviews.freebsd.org/D19248 to fix the unit number issue.
(In reply to Kristof Provost from comment #7) The updated patch fixed the issue. I think it's also a good example of virtualizing an interface.
Sorry to bother you again and again, but let me ask one more thing. Because some interface types (tap, vxlan etc.) still don't have virtualized cloners, I think similar problems could occur to them. (I know this is truly a corner case, though.) if_clone_destroy() in sys/net/if_clone.c has the following code to find the cloner for an interface to destroy. What I was wondering is why the line 263-267 (the first loop) and the line 271-278 (the second loop) use different code. Can't they be the same? (My original patch came from this question.) 261 /* Find the cloner for this interface */ 262 IF_CLONERS_LOCK(); 263 LIST_FOREACH(ifc, &V_if_cloners, ifc_list) { -> 264 if (strcmp(ifc->ifc_name, ifp->if_dname) == 0) { 265 break; 266 } 267 } 268 #ifdef VIMAGE 269 if (ifc == NULL && !IS_DEFAULT_VNET(curvnet)) { 270 CURVNET_SET_QUIET(vnet0); 271 LIST_FOREACH(ifc, &V_if_cloners, ifc_list) 272 if (ifc->ifc_type == SIMPLE) { -> 273 if (ifc_simple_match(ifc, name)) 274 break; 275 } else { -> 276 if (ifc->ifc_match(ifc, name)) 277 break; 278 } 279 CURVNET_RESTORE(); 280 } 281 #endif
(In reply to genneko217 from comment #9) That's a very good question that I don't have a great answer for. Looking at the commit that introduced it I'm inclined to think that might be a mistake. I've cc'd Gleb because he might know more.
A commit references this bug: Author: kp Date: Tue Mar 5 13:21:08 UTC 2019 New revision: 344794 URL: https://svnweb.freebsd.org/changeset/base/344794 Log: tun: VIMAGE fix for if_tun cloner The if_tun cloner is not virtualised, but if_clone_attach() does use a virtualised list of cloners. The result is that we can't find the if_tun cloner when we try to remove a renamed tun interface. Virtualise the cloner, and move the final cleanup into a sysuninit so that we're sure this happens after all of the vnet_sysuninits Note that we need unit numbers to be system-unique (rather than unique per vnet, as is done by if_clone_simple()). The unit number is used to create the corresponding /dev/tunX device node, and this node must match with the interface. Switch to if_clone_advanced() so that we have control over the unit numbers. Reproduction scenario: jail -c -n foo persist vnet jexec test ifconfig tun create jexec test ifconfig tun0 name wg0 jexec test ifconfig wg0 destroy PR: 235704 Reviewed by: bz, hrs, hselasky MFC after: 2 weeks Differential Revision: https://reviews.freebsd.org/D19248 Changes: head/sys/net/if_tun.c
A commit references this bug: Author: kp Date: Tue Mar 5 15:49:31 UTC 2019 New revision: 344797 URL: https://svnweb.freebsd.org/changeset/base/344797 Log: tun tests: Test renaming and destroying a tun interface in a vnet jail There was a problem destroying renamed tun interfaces in vnet jails. This was fixed in r344794. Test the previously failing scenario. PR: 235704 MFC after: 2 weeks Changes: head/tests/sys/net/Makefile head/tests/sys/net/if_tun_test.sh
A commit references this bug: Author: kp Date: Tue Mar 19 00:27:46 UTC 2019 New revision: 345285 URL: https://svnweb.freebsd.org/changeset/base/345285 Log: MFC r344794: tun: VIMAGE fix for if_tun cloner The if_tun cloner is not virtualised, but if_clone_attach() does use a virtualised list of cloners. The result is that we can't find the if_tun cloner when we try to remove a renamed tun interface. Virtualise the cloner, and move the final cleanup into a sysuninit so that we're sure this happens after all of the vnet_sysuninits Note that we need unit numbers to be system-unique (rather than unique per vnet, as is done by if_clone_simple()). The unit number is used to create the corresponding /dev/tunX device node, and this node must match with the interface. Switch to if_clone_advanced() so that we have control over the unit numbers. Reproduction scenario: jail -c -n foo persist vnet jexec test ifconfig tun create jexec test ifconfig tun0 name wg0 jexec test ifconfig wg0 destroy PR: 235704 Reviewed by: bz, hrs, hselasky Differential Revision: https://reviews.freebsd.org/D19248 Changes: _U stable/12/ stable/12/sys/net/if_tun.c
A commit references this bug: Author: kp Date: Tue Mar 19 00:27:48 UTC 2019 New revision: 345286 URL: https://svnweb.freebsd.org/changeset/base/345286 Log: MFC r344794: tun: VIMAGE fix for if_tun cloner The if_tun cloner is not virtualised, but if_clone_attach() does use a virtualised list of cloners. The result is that we can't find the if_tun cloner when we try to remove a renamed tun interface. Virtualise the cloner, and move the final cleanup into a sysuninit so that we're sure this happens after all of the vnet_sysuninits Note that we need unit numbers to be system-unique (rather than unique per vnet, as is done by if_clone_simple()). The unit number is used to create the corresponding /dev/tunX device node, and this node must match with the interface. Switch to if_clone_advanced() so that we have control over the unit numbers. Reproduction scenario: jail -c -n foo persist vnet jexec test ifconfig tun create jexec test ifconfig tun0 name wg0 jexec test ifconfig wg0 destroy PR: 235704 Reviewed by: bz, hrs, hselasky Differential Revision: https://reviews.freebsd.org/D19248 Changes: _U stable/11/ stable/11/sys/net/if_tun.c
A commit references this bug: Author: kp Date: Tue Mar 19 00:29:18 UTC 2019 New revision: 345287 URL: https://svnweb.freebsd.org/changeset/base/345287 Log: MFC r344797: tun tests: Test renaming and destroying a tun interface in a vnet jail There was a problem destroying renamed tun interfaces in vnet jails. This was fixed in r344794. Test the previously failing scenario. PR: 235704 Changes: _U stable/12/ stable/12/tests/sys/net/Makefile stable/12/tests/sys/net/if_tun_test.sh