* [PATCH v2 1/5] Drivers: hv: vmbus: avoid double kfree for device_obj
2015-01-27 16:44 [PATCH v2 0/5] Drivers: hv: vmbus: fix crashes on hv_vmbus load/unload path Vitaly Kuznetsov
@ 2015-01-27 16:44 ` Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 2/5] Drivers: hv: vmbus: teardown hv_vmbus_con workqueue and vmbus_connection pages on shutdown Vitaly Kuznetsov
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-01-27 16:44 UTC (permalink / raw)
To: K. Y. Srinivasan, devel
Cc: Thomas Gleixner, Haiyang Zhang, linux-kernel, Dexuan Cui
On driver shutdown device_obj is being freed twice:
1) In vmbus_free_channels()
2) vmbus_device_release() (which is being triggered by device_unregister() in
vmbus_device_unregister().
This double kfree leads to the following sporadic crash on driver unload:
[ 23.469876] general protection fault: 0000 [#1] SMP
[ 23.470036] Modules linked in: hv_vmbus(-)
[ 23.470036] CPU: 2 PID: 213 Comm: rmmod Not tainted 3.19.0-rc5_bug923184+ #488
[ 23.470036] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090006 05/23/2012
[ 23.470036] task: ffff880036ef1cb0 ti: ffff880036ce8000 task.ti: ffff880036ce8000
[ 23.470036] RIP: 0010:[<ffffffff811d2e1b>] [<ffffffff811d2e1b>] __kmalloc_node_track_caller+0xdb/0x1e0
[ 23.470036] RSP: 0018:ffff880036cebcc8 EFLAGS: 00010246
...
When this crash does not happen on driver unload the similar one is expected if
we try to load hv_vmbus again.
Remove kfree from vmbus_free_channels() as freeing it from
vmbus_device_release() seems right.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/channel_mgmt.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 1d7df25..ac357b8 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -259,7 +259,6 @@ void vmbus_free_channels(void)
list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
vmbus_device_unregister(channel->device_obj);
- kfree(channel->device_obj);
free_channel(channel);
}
}
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/5] Drivers: hv: vmbus: teardown hv_vmbus_con workqueue and vmbus_connection pages on shutdown
2015-01-27 16:44 [PATCH v2 0/5] Drivers: hv: vmbus: fix crashes on hv_vmbus load/unload path Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 1/5] Drivers: hv: vmbus: avoid double kfree for device_obj Vitaly Kuznetsov
@ 2015-01-27 16:44 ` Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 3/5] drivers: hv: Teardown synthetic interrupt controllers on module unload Vitaly Kuznetsov
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-01-27 16:44 UTC (permalink / raw)
To: K. Y. Srinivasan, devel
Cc: Thomas Gleixner, Haiyang Zhang, linux-kernel, Dexuan Cui
We need to destroy hv_vmbus_con on module shutdown, otherwise the following
crash is sometimes observed:
[ 76.569845] hv_vmbus: Hyper-V Host Build:9600-6.3-17-0.17039; Vmbus version:3.0
[ 82.598859] BUG: unable to handle kernel paging request at ffffffffa0003480
[ 82.599287] IP: [<ffffffffa0003480>] 0xffffffffa0003480
[ 82.599287] PGD 1f34067 PUD 1f35063 PMD 3f72d067 PTE 0
[ 82.599287] Oops: 0010 [#1] SMP
[ 82.599287] Modules linked in: [last unloaded: hv_vmbus]
[ 82.599287] CPU: 0 PID: 26 Comm: kworker/0:1 Not tainted 3.19.0-rc5_bug923184+ #488
[ 82.599287] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v1.0 11/26/2012
[ 82.599287] Workqueue: hv_vmbus_con 0xffffffffa0003480
[ 82.599287] task: ffff88007b6ddfa0 ti: ffff88007f8f8000 task.ti: ffff88007f8f8000
[ 82.599287] RIP: 0010:[<ffffffffa0003480>] [<ffffffffa0003480>] 0xffffffffa0003480
[ 82.599287] RSP: 0018:ffff88007f8fbe00 EFLAGS: 00010202
...
To avoid memory leaks we need to free monitor_pages and int_page for
vmbus_connection. Implement vmbus_disconnect() function by separating cleanup
path from vmbus_connect().
As we use hv_vmbus_con to release channels (see free_channel() in channel_mgmt.c)
we need to make sure the work was done before we remove the queue, do that with
drain_workqueue(). We also need to avoid handling messages which can (potentially)
create new channels, so set vmbus_connection.conn_state = DISCONNECTED at the very
beginning of vmbus_exit() and check for that in vmbus_onmessage_work().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/connection.c | 17 ++++++++++++-----
drivers/hv/hyperv_vmbus.h | 1 +
drivers/hv/vmbus_drv.c | 6 ++++++
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index a63a795..c4acd1c 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -216,10 +216,21 @@ int vmbus_connect(void)
cleanup:
pr_err("Unable to connect to host\n");
+
vmbus_connection.conn_state = DISCONNECTED;
+ vmbus_disconnect();
+
+ kfree(msginfo);
+
+ return ret;
+}
- if (vmbus_connection.work_queue)
+void vmbus_disconnect(void)
+{
+ if (vmbus_connection.work_queue) {
+ drain_workqueue(vmbus_connection.work_queue);
destroy_workqueue(vmbus_connection.work_queue);
+ }
if (vmbus_connection.int_page) {
free_pages((unsigned long)vmbus_connection.int_page, 0);
@@ -230,10 +241,6 @@ cleanup:
free_pages((unsigned long)vmbus_connection.monitor_pages[1], 0);
vmbus_connection.monitor_pages[0] = NULL;
vmbus_connection.monitor_pages[1] = NULL;
-
- kfree(msginfo);
-
- return ret;
}
/*
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 44b1c94..6cf2de9 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -692,6 +692,7 @@ void vmbus_free_channels(void);
/* Connection interface */
int vmbus_connect(void);
+void vmbus_disconnect(void);
int vmbus_post_msg(void *buffer, size_t buflen);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 7488111..d5cd2fb 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -573,6 +573,10 @@ static void vmbus_onmessage_work(struct work_struct *work)
{
struct onmessage_work_context *ctx;
+ /* Do not process messages if we're in DISCONNECTED state */
+ if (vmbus_connection.conn_state == DISCONNECTED)
+ return;
+
ctx = container_of(work, struct onmessage_work_context,
work);
vmbus_onmessage(&ctx->msg);
@@ -992,11 +996,13 @@ cleanup:
static void __exit vmbus_exit(void)
{
+ vmbus_connection.conn_state = DISCONNECTED;
hv_remove_vmbus_irq();
vmbus_free_channels();
bus_unregister(&hv_bus);
hv_cleanup();
acpi_bus_unregister_driver(&vmbus_acpi_driver);
+ vmbus_disconnect();
}
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 3/5] drivers: hv: Teardown synthetic interrupt controllers on module unload
2015-01-27 16:44 [PATCH v2 0/5] Drivers: hv: vmbus: fix crashes on hv_vmbus load/unload path Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 1/5] Drivers: hv: vmbus: avoid double kfree for device_obj Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 2/5] Drivers: hv: vmbus: teardown hv_vmbus_con workqueue and vmbus_connection pages on shutdown Vitaly Kuznetsov
@ 2015-01-27 16:44 ` Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 4/5] clockevents: export clockevents_unbind_device instead of clockevents_unbind Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 5/5] Drivers: hv: vmbus: Teardown clockevent devices on module unload Vitaly Kuznetsov
4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-01-27 16:44 UTC (permalink / raw)
To: K. Y. Srinivasan, devel
Cc: Thomas Gleixner, Haiyang Zhang, linux-kernel, Dexuan Cui
SynIC has to be switched off when we unload the module, otherwise registered
memory pages can get corrupted after (as Hyper-V host still writes there) and
we see the following crashes for random processes:
[ 89.116774] BUG: Bad page map in process sh pte:4989c716 pmd:36f81067
[ 89.159454] addr:0000000000437000 vm_flags:00000875 anon_vma: (null) mapping:ffff88007bba55a0 index:37
[ 89.226146] vma->vm_ops->fault: filemap_fault+0x0/0x410
[ 89.257776] vma->vm_file->f_op->mmap: generic_file_mmap+0x0/0x60
[ 89.297570] CPU: 0 PID: 215 Comm: sh Tainted: G B 3.19.0-rc5_bug923184+ #488
[ 89.353738] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS 090006 05/23/2012
[ 89.409138] 0000000000000000 000000004e083d7b ffff880036e9fa18 ffffffff81a68d31
[ 89.468724] 0000000000000000 0000000000437000 ffff880036e9fa68 ffffffff811a1e3a
[ 89.519233] 000000004989c716 0000000000000037 ffffea0001edc340 0000000000437000
[ 89.575751] Call Trace:
[ 89.591060] [<ffffffff81a68d31>] dump_stack+0x45/0x57
[ 89.625164] [<ffffffff811a1e3a>] print_bad_pte+0x1aa/0x250
[ 89.667234] [<ffffffff811a2c95>] vm_normal_page+0x55/0xa0
[ 89.703818] [<ffffffff811a3105>] unmap_page_range+0x425/0x8a0
[ 89.737982] [<ffffffff811a3601>] unmap_single_vma+0x81/0xf0
[ 89.780385] [<ffffffff81184320>] ? lru_deactivate_fn+0x190/0x190
[ 89.820130] [<ffffffff811a4131>] unmap_vmas+0x51/0xa0
[ 89.860168] [<ffffffff811ad12c>] exit_mmap+0xac/0x1a0
[ 89.890588] [<ffffffff810763c3>] mmput+0x63/0x100
[ 89.919205] [<ffffffff811eba48>] flush_old_exec+0x3f8/0x8b0
[ 89.962135] [<ffffffff8123b5bb>] load_elf_binary+0x32b/0x1260
[ 89.998581] [<ffffffff811a14f2>] ? get_user_pages+0x52/0x60
hv_synic_cleanup() function exists but noone calls it now. Do the following:
- call hv_synic_cleanup() on each cpu from vmbus_exit();
- write global disable bit through MSR;
- use hv_synic_free_cpu() to avoid memory leask and code duplication.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/hv.c | 9 +++++++--
drivers/hv/vmbus_drv.c | 4 ++++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 50e51a5..39531dc 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -477,6 +477,7 @@ void hv_synic_cleanup(void *arg)
union hv_synic_sint shared_sint;
union hv_synic_simp simp;
union hv_synic_siefp siefp;
+ union hv_synic_scontrol sctrl;
int cpu = smp_processor_id();
if (!hv_context.synic_initialized)
@@ -502,6 +503,10 @@ void hv_synic_cleanup(void *arg)
wrmsrl(HV_X64_MSR_SIEFP, siefp.as_uint64);
- free_page((unsigned long)hv_context.synic_message_page[cpu]);
- free_page((unsigned long)hv_context.synic_event_page[cpu]);
+ /* Disable the global synic bit */
+ rdmsrl(HV_X64_MSR_SCONTROL, sctrl.as_uint64);
+ sctrl.enable = 0;
+ wrmsrl(HV_X64_MSR_SCONTROL, sctrl.as_uint64);
+
+ hv_synic_free_cpu(cpu);
}
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index d5cd2fb..f7e2c9e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -996,11 +996,15 @@ cleanup:
static void __exit vmbus_exit(void)
{
+ int cpu;
+
vmbus_connection.conn_state = DISCONNECTED;
hv_remove_vmbus_irq();
vmbus_free_channels();
bus_unregister(&hv_bus);
hv_cleanup();
+ for_each_online_cpu(cpu)
+ smp_call_function_single(cpu, hv_synic_cleanup, NULL, 1);
acpi_bus_unregister_driver(&vmbus_acpi_driver);
vmbus_disconnect();
}
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 4/5] clockevents: export clockevents_unbind_device instead of clockevents_unbind
2015-01-27 16:44 [PATCH v2 0/5] Drivers: hv: vmbus: fix crashes on hv_vmbus load/unload path Vitaly Kuznetsov
` (2 preceding siblings ...)
2015-01-27 16:44 ` [PATCH v2 3/5] drivers: hv: Teardown synthetic interrupt controllers on module unload Vitaly Kuznetsov
@ 2015-01-27 16:44 ` Vitaly Kuznetsov
2015-01-27 16:44 ` [PATCH v2 5/5] Drivers: hv: vmbus: Teardown clockevent devices on module unload Vitaly Kuznetsov
4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-01-27 16:44 UTC (permalink / raw)
To: K. Y. Srinivasan, devel
Cc: Thomas Gleixner, Haiyang Zhang, linux-kernel, Dexuan Cui
It looks like clockevents_unbind is being exported by mistake as:
- it is static;
- it is not listed in include/linux/clockchips.h;
- EXPORT_SYMBOL_GPL(clockevents_unbind) follows clockevents_unbind_device()
implementation.
I think clockevents_unbind_device should be exported instead. This is going to
be used to teardown Hyper-V clockevent devices on module unload.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
kernel/time/clockevents.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 5544990..888ecc1 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -371,7 +371,7 @@ int clockevents_unbind_device(struct clock_event_device *ced, int cpu)
mutex_unlock(&clockevents_mutex);
return ret;
}
-EXPORT_SYMBOL_GPL(clockevents_unbind);
+EXPORT_SYMBOL_GPL(clockevents_unbind_device);
/**
* clockevents_register_device - register a clock event device
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 5/5] Drivers: hv: vmbus: Teardown clockevent devices on module unload
2015-01-27 16:44 [PATCH v2 0/5] Drivers: hv: vmbus: fix crashes on hv_vmbus load/unload path Vitaly Kuznetsov
` (3 preceding siblings ...)
2015-01-27 16:44 ` [PATCH v2 4/5] clockevents: export clockevents_unbind_device instead of clockevents_unbind Vitaly Kuznetsov
@ 2015-01-27 16:44 ` Vitaly Kuznetsov
4 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2015-01-27 16:44 UTC (permalink / raw)
To: K. Y. Srinivasan, devel
Cc: Thomas Gleixner, Haiyang Zhang, linux-kernel, Dexuan Cui
Newly introduced clockevent devices made it impossible to unload hv_vmbus
module as clockevents_config_and_register() takes additional reverence to
the module. To make it possible again we do the following:
- avoid setting dev->owner for clockevent devices;
- implement hv_synic_clockevents_cleanup() doing clockevents_unbind_device();
- call it from vmbus_exit().
In theory hv_synic_clockevents_cleanup() can be merged with hv_synic_cleanup(),
however, we call hv_synic_cleanup() from smp_call_function_single() and this
doesn't work for clockevents_unbind_device() as it does such call on its own. I
opted for a separate function.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/hv/hv.c | 25 ++++++++++++++++++++++++-
drivers/hv/hyperv_vmbus.h | 2 ++
drivers/hv/vmbus_drv.c | 1 +
3 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 39531dc..d3943bc 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -312,7 +312,11 @@ static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
dev->features = CLOCK_EVT_FEAT_ONESHOT;
dev->cpumask = cpumask_of(cpu);
dev->rating = 1000;
- dev->owner = THIS_MODULE;
+ /*
+ * Avoid settint dev->owner = THIS_MODULE deliberately as doing so will
+ * result in clockevents_config_and_register() taking additional
+ * references to the hv_vmbus module making it impossible to unload.
+ */
dev->set_mode = hv_ce_setmode;
dev->set_next_event = hv_ce_set_next_event;
@@ -470,6 +474,20 @@ void hv_synic_init(void *arg)
}
/*
+ * hv_synic_clockevents_cleanup - Cleanup clockevent devices
+ */
+void hv_synic_clockevents_cleanup(void)
+{
+ int cpu;
+
+ if (!(ms_hyperv.features & HV_X64_MSR_SYNTIMER_AVAILABLE))
+ return;
+
+ for_each_online_cpu(cpu)
+ clockevents_unbind_device(hv_context.clk_evt[cpu], cpu);
+}
+
+/*
* hv_synic_cleanup - Cleanup routine for hv_synic_init().
*/
void hv_synic_cleanup(void *arg)
@@ -483,6 +501,11 @@ void hv_synic_cleanup(void *arg)
if (!hv_context.synic_initialized)
return;
+ /* Turn off clockevent device */
+ if (ms_hyperv.features & HV_X64_MSR_SYNTIMER_AVAILABLE)
+ hv_ce_setmode(CLOCK_EVT_MODE_SHUTDOWN,
+ hv_context.clk_evt[cpu]);
+
rdmsrl(HV_X64_MSR_SINT0 + VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
shared_sint.masked = 1;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index 6cf2de9..b055e53 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -572,6 +572,8 @@ extern void hv_synic_init(void *irqarg);
extern void hv_synic_cleanup(void *arg);
+extern void hv_synic_clockevents_cleanup(void);
+
/*
* Host version information.
*/
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index f7e2c9e..fe6d35e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -999,6 +999,7 @@ static void __exit vmbus_exit(void)
int cpu;
vmbus_connection.conn_state = DISCONNECTED;
+ hv_synic_clockevents_cleanup();
hv_remove_vmbus_irq();
vmbus_free_channels();
bus_unregister(&hv_bus);
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread