linux-kernel - RE: [PATCH 1/1] Drivers: HV: Send one page worth of kmsg dump over Hyper-V during panic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Jul 2018 01:04:36 +0000
From:   "Michael Kelley (EOSG)" <Michael.H.Kelley@...rosoft.com>
To:     KY Srinivasan <kys@...rosoft.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devel@...uxdriverproject.org" <devel@...uxdriverproject.org>,
        "olaf@...fle.de" <olaf@...fle.de>,
        "apw@...onical.com" <apw@...onical.com>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "vkuznets@...hat.com" <vkuznets@...hat.com>
CC:     Sunil Muthuswamy <sunilmut@...rosoft.com>
Subject: RE: [PATCH 1/1] Drivers: HV: Send one page worth of kmsg dump over
 Hyper-V during panic

>From kys@...uxonhyperv.com <kys@...uxonhyperv.com>  Sent: Saturday, July 7, 2018 7:57 PM
> 
> From: Sunil Muthuswamy <sunilmut@...rosoft.com>
> 
> In the VM mode on Hyper-V, currently, when the kernel panics, an error
> code and few register values are populated in an MSR and the Hypervisor
> notified. This information is collected on the host. The amount of
> information currently collected is found to be limited and not very
> actionable. To gather more actionable data, such as stack trace, the
> proposal is to write one page worth of kmsg data on an allocated page
> and the Hypervisor notified of the page address through the MSR.
> 
> - Sysctl option to control the behavior, with ON by default.
> 
> Cc: K. Y. Srinivasan <kys@...rosoft.com>
> Cc: Stephen Hemminger <sthemmin@...rosoft.com>
> Signed-off-by: Sunil Muthuswamy <sunilmut@...rosoft.com>
> Signed-off-by: K. Y. Srinivasan <kys@...rosoft.com>
> ---

> +	/*
> +	 * Write dump contents to the page. No need to synchronize; panic should
> +	 * be single-threaded.
> +	 */
> +	if (!kmsg_dump_get_buffer(dumper, true, hv_panic_page,
> +				  PAGE_SIZE, &bytes_written)) {
> +		pr_err("Hyper-V: Unable to get kmsg data for panic\n");
> +		return;

>From what I can see, the return value from kmsg_dump_get_buffer()
is not an indication of success or failure -- it's an indication of whether
there is more data available.   There's no reason to output an error
message.

> @@ -1065,6 +1136,32 @@ static int vmbus_bus_init(void)
>  	 * Only register if the crash MSRs are available
>  	 */
>  	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
> +		u64 hyperv_crash_ctl;
> +		/*
> +		 * Sysctl registration is not fatal, since by default
> +		 * reporting is enabled.
> +		 */
> +		hv_ctl_table_hdr = register_sysctl_table(hv_root_table);
> +		if (!hv_ctl_table_hdr)
> +			pr_err("Hyper-V: sysctl table register error");
> +
> +		/*
> +		 * Register for panic kmsg callback only if the right
> +		 * capability is supported by the hypervisor.
> +		 */
> +		rdmsrl(HV_X64_MSR_CRASH_CTL, hyperv_crash_ctl);
> +		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG) {

vmbus_drv.c is architecture independent code, and should not be referencing
x86/x64 MSRs.   Reading the MSR (and maybe the test as well?) should go
in a separate function in an x86-specific source file.

And just to confirm, is this the right way to test for the feature?  Usually,
feature determination is based on one of the feature registers.  The
NOTIFY_MSG flag seems to have a dual meaning -- on read it indicates
the feature is present.  On write in hyperv_report_panic_msg(), it evidently
means that the guest is sending a full page of data to Hyper-V.

> @@ -1081,6 +1178,11 @@ static int vmbus_bus_init(void)
>  	bus_unregister(&hv_bus);
> +	free_page((unsigned long)hv_panic_page);
> +	if (!hv_ctl_table_hdr) {

The above test is backwards.  Remove the bang.

> @@ -1785,10 +1887,18 @@ static void __exit vmbus_exit(void)
> +	free_page((unsigned long)hv_panic_page);
> +	if (!hv_ctl_table_hdr) {

Same here.  Test is backwards.

Michael