Re: [PATCH] Drivers: hv: vmbus: Fix potential crash on module unload

From: Wei Liu <wei.liu@kernel.org>
To: "Michael Kelley (LINUX)" <mikelley@microsoft.com>
Cc: "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"kexec@lists.infradead.org" <kexec@lists.infradead.org>,
	"kernel@gpiccoli.net" <kernel@gpiccoli.net>,
	Tianyu Lan <Tianyu.Lan@microsoft.com>,
	KY Srinivasan <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"wei.liu@kernel.org" <wei.liu@kernel.org>,
	Dexuan Cui <decui@microsoft.com>
Subject: Re: [PATCH] Drivers: hv: vmbus: Fix potential crash on module unload
Date: Tue, 29 Mar 2022 12:05:33 +0000	[thread overview]
Message-ID: <20220329120533.pbmzmq7oqybrtlfd@liuwe-devbox-debian-v2> (raw)
In-Reply-To: <PH0PR21MB302540E78316B06885EBE72AD7149@PH0PR21MB3025.namprd21.prod.outlook.com>

On Sat, Mar 19, 2022 at 03:30:16PM +0000, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli <gpiccoli@igalia.com> Sent: Tuesday, March 15, 2022 1:36 PM
> > 
> > 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
> 
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> 

Applied to hyperv-fixes. Thanks.