Bug 209428 - if_vtnet(4) panics when doing kldunload if_vtnet
Summary: if_vtnet(4) panics when doing kldunload if_vtnet
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-virtualization (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-10 15:14 UTC by Roman Bogorodskiy
Modified: 2016-05-21 05:40 UTC (History)
5 users (show)

See Also:


Attachments
possible fix (666 bytes, patch)
2016-05-10 15:45 UTC, Roman Bogorodskiy
no flags Details | Diff
updated patch (1.28 KB, patch)
2016-05-11 16:47 UTC, Roman Bogorodskiy
no flags Details | Diff
patch v3 (1.31 KB, patch)
2016-05-12 13:58 UTC, Roman Bogorodskiy
no flags Details | Diff
patch v4 (1.08 KB, patch)
2016-05-12 14:47 UTC, Roman Bogorodskiy
no flags Details | Diff
patch v5 (912 bytes, patch)
2016-05-12 15:18 UTC, Roman Bogorodskiy
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-10 15:14:13 UTC
When using if_vtnet(4) as a module and unloading it using "kldunload if_vtnet" it immediately causes a panic:

Fatal trap 12: page fault while in kernel mode
cpuid = 0; apic id = 00
fault virtual address   = 0x80
fault code              = supervisor read data, page not present
instruction pointer     = 0x20:0xffffffff80cff56d
stack pointer           = 0x28:0xfffffe0096eb28a0
frame pointer           = 0x28:0xfffffe0096eb28d0
code segment            = base rx0, limit 0xfffff, type 0x1b
                        = DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags        = interrupt enabled, resume, IOPL = 0
current process         = 779 (kldunload)
[ thread pid 779 tid 100058 ]
Stopped at      uma_zone_get_cur+0xd:   movq    0x80(%r14),%rdi
db> bt
Tracing pid 779 tid 100058 td 0xfffff800064219a0
uma_zone_get_cur() at uma_zone_get_cur+0xd/frame 0xfffffe0096eb28d0
vtnet_modevent() at vtnet_modevent+0x51/frame 0xfffffe0096eb2900
module_unload() at module_unload+0x32/frame 0xfffffe0096eb2920
linker_file_unload() at linker_file_unload+0x24b/frame 0xfffffe0096eb2970
kern_kldunload() at kern_kldunload+0xbc/frame 0xfffffe0096eb29a0
amd64_syscall() at amd64_syscall+0x2db/frame 0xfffffe0096eb2ab0
Xfast_syscall() at Xfast_syscall+0xfb/frame 0xfffffe0096eb2ab0
--- syscall (444, FreeBSD ELF64, sys_kldunloadf), rip = 0x80086b15a, rsp = 0x7fffffffe348, rbp = 0x7fffffffebb0 ---
db>
Comment 1 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-10 15:45:32 UTC
Created attachment 170182 [details]
possible fix

Attaching a patch that fixes this issue for me.
Comment 2 Kristof Provost freebsd_committer freebsd_triage 2016-05-10 20:38:29 UTC
That fix looked odd to me, because I did not expect vtnet_modevent() to be called twice with MOD_UNLOAD.
That is the cause though, because there are two DRIVER_MODULE()s for vtnet.

I think this problem was introduced with r276367. Cc-ing Andrew so he can take a look too.
Comment 3 Kristof Provost freebsd_committer freebsd_triage 2016-05-10 22:08:38 UTC
It's just occurred to me that if vtnet_modevent() gets called twice when it's unloaded it probably also gets called twice when the module is loaded.
That means that it leaks the first vtnet_tx_header_zone allocated with uma_zcreate().
Comment 4 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-11 16:47:19 UTC
Created attachment 170217 [details]
updated patch

Yeah, you're right, vtnet_modevent() is called twice, so we don't need to duplicate uma_zcreate() calls indeed.
Comment 5 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-11 17:10:47 UTC
Oh, and actually it appears to work fine if I just do:

 DRIVER_MODULE(vtnet, virtio_mmio, vtnet_driver, vtnet_devclass,
     vtnet_modevent, 0);
 DRIVER_MODULE(vtnet, virtio_pci, vtnet_driver, vtnet_devclass,
-    vtnet_modevent, 0);
+    0, 0);
Comment 6 Kristof Provost freebsd_committer freebsd_triage 2016-05-11 17:16:13 UTC
(In reply to Roman Bogorodskiy from comment #5)
Yeah, but that's probably risky. I'm not sure the load order is guaranteed, and I'm also not sure that there's any guarantee that the probe function won't be executed until both are loaded.

There's a couple other drivers with a similar setup. For example sys/dev/cxgbe/t4_main.c

They've solved it with a simple counter in their mod_event function. I'd be inclined to  do something similar.
Comment 7 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-11 17:29:07 UTC
(In reply to Kristof Provost from comment #6)

Thanks for the pointer, I'll try to test the same approach for vtnet and upload a new patch if things go well.
Comment 8 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-12 13:58:02 UTC
Created attachment 170227 [details]
patch v3

Updated the patch to use an approach similar to the one used in cxgbe. Basic testing didn't reveal any issues.
Comment 9 Kristof Provost freebsd_committer freebsd_triage 2016-05-12 14:09:03 UTC
That looks correct to me, although I'm not sure if the mutex is actually required.
The code calling the module load/unload function seems to do so with Giant held.
Comment 10 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-12 14:32:21 UTC
(In reply to Kristof Provost from comment #9)

Ah, I think that's right:

https://svnweb.freebsd.org/base/head/sys/kern/kern_module.c?revision=289111&view=markup#l260

It's not obvious why cxgbe uses it then.
Comment 11 Kristof Provost freebsd_committer freebsd_triage 2016-05-12 14:36:27 UTC
I *think* cxgbe is wrong (or overly cautious). I'd propose doing it without the mutex in vtnet.

You're not going to make things any worse than the currently are by not adding it.
Comment 12 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-12 14:47:38 UTC
Created attachment 170229 [details]
patch v4

Uploaded v4 of the patch, without mutex. Again, basic testing didn't show any issues.
Comment 13 Kristof Provost freebsd_committer freebsd_triage 2016-05-12 14:51:06 UTC
Hmm, can you verify that uma_zdestroy() still gets called?
I think the order is MOD_LOAD on load, but an unload first gets MOD_QUIESCE and then MOD_UNLOAD.
That'd mean we do loaded++ twice, but --loaded four times.
Comment 14 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-12 15:18:30 UTC
Created attachment 170230 [details]
patch v5

Good point, I've moved out EBUSY check out of the --loader == 0 condition, as all the MOD_QUIESCE calls will be _before_ the last MOD_UNLOAD where we clean things up.
Comment 15 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-12 15:44:55 UTC
(In reply to Roman Bogorodskiy from comment #14)

With some debug logging it looks like this (printf's + dmesg):

load: loaded = 0

load: loaded = 1
vtnet0: <VirtIO Networking Adapter> on virtio_pci0
vtnet0: Ethernet address: 52:54:00:f5:03:43
vtnet0: link state changed to UP

quiesce: loaded = 2

quiesce: loaded = 2

quiesce: loaded = 2

quiesce: loaded = 2
vtnet0: default vnet
vtnet0: link state changed to DOWN
vtnet0: 
detached

unload: loaded = 1

unload: uma_zdestroy

unload: loaded = 0

("load" logs are printed _before_ changing value of 'loaded', "unload" are printed _after_).

There were a couple "Device busy" statues on unload before it succeeded.
Comment 16 commit-hook freebsd_committer freebsd_triage 2016-05-14 06:08:00 UTC
A commit references this bug:

Author: kp
Date: Sat May 14 06:07:15 UTC 2016
New revision: 299725
URL: https://svnweb.freebsd.org/changeset/base/299725

Log:
  vtnet: fix panic on unload

  Since r276367 added the virtio_mmio support vtnet_modevent() gets called twice.
  This resulted in a memory leak during load and a panic on unload.

  Count the loads so we only initialise once (just like cxgbe(4)), and only clean
  up in the final unload.

  PR:		209428
  Submitted by:	novel@FreeBSD.org
  MFC after:	1 week

Changes:
  head/sys/dev/virtio/network/if_vtnet.c
Comment 17 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-14 15:11:58 UTC
Kristof, thanks for reviewing and committing this!
Comment 18 Kristof Provost freebsd_committer freebsd_triage 2016-05-21 03:50:44 UTC
Thanks for the fix Roman.

I was looking to MFC this, but in stable/10 there's only one DRIVER_MODULE(), so there's no problem there.
Comment 19 Roman Bogorodskiy freebsd_committer freebsd_triage 2016-05-21 05:40:19 UTC
(In reply to Kristof Provost from comment #18)

Thanks for helping to create a *proper* fix. 

If you're interested, I've noticed one more issue with if_vtnet(4): https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=209427