All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Guilherme G. Piccoli" <gpiccoli@igalia.com>
To: linux-hyperv@vger.kernel.org
Cc: kexec@lists.infradead.org, gpiccoli@igalia.com,
	kernel@gpiccoli.net, mikelley@microsoft.com,
	Tianyu.Lan@microsoft.com, kys@microsoft.com,
	haiyangz@microsoft.com, sthemmin@microsoft.com,
	wei.liu@kernel.org, decui@microsoft.com
Subject: [PATCH] Drivers: hv: vmbus: Fix potential crash on module unload
Date: Tue, 15 Mar 2022 17:35:35 -0300	[thread overview]
Message-ID: <20220315203535.682306-1-gpiccoli@igalia.com> (raw)

The vmbus driver relies on the panic notifier infrastructure to perform
some operations when a panic event is detected. Since vmbus can be built
as module, it is required that the driver handles both registering and
unregistering such panic notifier callback.

After commit 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic callback")
though, the panic notifier registration is done unconditionally in the module
initialization routine whereas the unregistering procedure is conditionally
guarded and executes only if HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE capability
is set.

This patch fixes that by unconditionally unregistering the panic notifier
in the module's exit routine as well.

Fixes: 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic callback")
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


Hi folks, thanks in advance for any reviews! This was build-tested
with Debian config, on 5.17-rc7.

This patch is a result of code analysis - I didn't experience this
issue but seems a valid/feasible case.

Also, this is part of an ongoing effort of clearing/refactoring the panic
notifiers, more will be done soon, but I prefer to send the simple bug
fixes quickly, or else it might take a while since the next steps are more
complex and subject to many iterations I expect.

Cheers,

Guilherme


 drivers/hv/vmbus_drv.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 12a2b37e87f3..12585324cc4a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2780,10 +2780,15 @@ static void __exit vmbus_exit(void)
 	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
 		kmsg_dump_unregister(&hv_kmsg_dumper);
 		unregister_die_notifier(&hyperv_die_block);
-		atomic_notifier_chain_unregister(&panic_notifier_list,
-						 &hyperv_panic_block);
 	}
 
+	/*
+	 * The panic notifier is always registered, hence we should
+	 * also unconditionally unregister it here as well.
+	 */
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &hyperv_panic_block);
+
 	free_page((unsigned long)hv_panic_page);
 	unregister_sysctl_table(hv_ctl_table_hdr);
 	hv_ctl_table_hdr = NULL;
-- 
2.35.1


WARNING: multiple messages have this Message-ID (diff)
From: Guilherme G. Piccoli <gpiccoli@igalia.com>
To: kexec@lists.infradead.org
Subject: [PATCH] Drivers: hv: vmbus: Fix potential crash on module unload
Date: Tue, 15 Mar 2022 17:35:35 -0300	[thread overview]
Message-ID: <20220315203535.682306-1-gpiccoli@igalia.com> (raw)

The vmbus driver relies on the panic notifier infrastructure to perform
some operations when a panic event is detected. Since vmbus can be built
as module, it is required that the driver handles both registering and
unregistering such panic notifier callback.

After commit 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic callback")
though, the panic notifier registration is done unconditionally in the module
initialization routine whereas the unregistering procedure is conditionally
guarded and executes only if HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE capability
is set.

This patch fixes that by unconditionally unregistering the panic notifier
in the module's exit routine as well.

Fixes: 74347a99e73a ("x86/Hyper-V: Unload vmbus channel in hv panic callback")
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


Hi folks, thanks in advance for any reviews! This was build-tested
with Debian config, on 5.17-rc7.

This patch is a result of code analysis - I didn't experience this
issue but seems a valid/feasible case.

Also, this is part of an ongoing effort of clearing/refactoring the panic
notifiers, more will be done soon, but I prefer to send the simple bug
fixes quickly, or else it might take a while since the next steps are more
complex and subject to many iterations I expect.

Cheers,

Guilherme


 drivers/hv/vmbus_drv.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 12a2b37e87f3..12585324cc4a 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -2780,10 +2780,15 @@ static void __exit vmbus_exit(void)
 	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
 		kmsg_dump_unregister(&hv_kmsg_dumper);
 		unregister_die_notifier(&hyperv_die_block);
-		atomic_notifier_chain_unregister(&panic_notifier_list,
-						 &hyperv_panic_block);
 	}
 
+	/*
+	 * The panic notifier is always registered, hence we should
+	 * also unconditionally unregister it here as well.
+	 */
+	atomic_notifier_chain_unregister(&panic_notifier_list,
+					 &hyperv_panic_block);
+
 	free_page((unsigned long)hv_panic_page);
 	unregister_sysctl_table(hv_ctl_table_hdr);
 	hv_ctl_table_hdr = NULL;
-- 
2.35.1



             reply	other threads:[~2022-03-15 21:17 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-15 20:35 Guilherme G. Piccoli [this message]
2022-03-15 20:35 ` [PATCH] Drivers: hv: vmbus: Fix potential crash on module unload Guilherme G. Piccoli
2022-03-19 15:30 ` Michael Kelley (LINUX)
2022-03-19 15:30   ` Michael Kelley
2022-03-29 12:05   ` Wei Liu
2022-03-29 12:05     ` Wei Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220315203535.682306-1-gpiccoli@igalia.com \
    --to=gpiccoli@igalia.com \
    --cc=Tianyu.Lan@microsoft.com \
    --cc=decui@microsoft.com \
    --cc=haiyangz@microsoft.com \
    --cc=kernel@gpiccoli.net \
    --cc=kexec@lists.infradead.org \
    --cc=kys@microsoft.com \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=mikelley@microsoft.com \
    --cc=sthemmin@microsoft.com \
    --cc=wei.liu@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.