linux-kernel - Re: [RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

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]
Message-ID: <874k0kirbf.fsf@redhat.com>
Date:   Thu, 16 Jun 2022 17:03:16 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Vit Kabele <vit.kabele@...go.com>, linux-hyperv@...r.kernel.org
Cc:     mikelley@...rosoft.com, linux-kernel@...r.kernel.org,
        kys@...rosoft.com
Subject: Re: [RFC PATCH] Hyper-V: Initialize crash reporting before vmbus

Vit Kabele <vit.kabele@...go.com> writes:

> The Hyper-V crash reporting feature is initialized after a successful
> vmbus setup. The reporting feature however does not require vmbus at all
> and Windows guests can indeed use the reporting capabilities even with
> the minimal Hyper-V implementation (as described in the Minimal
> Requirements document).
>
> Reorder the initialization callbacks so that the crash reporting
> callbacks are registered before the vmbus initialization starts.
>
> Nevertheless, I am not sure about following:
>
> 1/ The vmbus_initiate_unload function is called within the panic handler
> even when the vmbus initialization does not finish (there might be no
> vmbus at all). This should probably not be problem because the vmbus
> unload function always checks for current connection state and does
> nothing when this is "DISCONNECTED". For better readability, it might be
> better to add separate panic notifier for vmbus and crash reporting.
>
> 2/ Wouldn't it be better to extract the whole reporting capability out
> of the vmbus module, so that it stays present in the kernel even when
> the vmbus module is possibly unloaded?

IMHO yes but as you mention hyperv_panic_event() currently does to
things:
1) Initiates VMBus unload
2) Reports panic to the hypervisor

I think untangling them moving the later to arch/x86/hyper-v (and
arch/arm64/hyperv/) makes sense.

>
> Signed-off-by: Vit Kabele <vit.kabele@...go.com>
>
> ---
>  drivers/hv/vmbus_drv.c | 77 +++++++++++++++++++++++-------------------
>  1 file changed, 42 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 714d549b7b46..97873f03aa7a 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -1509,41 +1509,6 @@ static int vmbus_bus_init(void)
>  	if (hv_is_isolation_supported())
>  		sysctl_record_panic_msg = 0;
>  
> -	/*
> -	 * Only register if the crash MSRs are available
> -	 */
> -	if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
> -		u64 hyperv_crash_ctl;
> -		/*
> -		 * Panic message recording (sysctl_record_panic_msg)
> -		 * is enabled by default in non-isolated guests and
> -		 * disabled by default in isolated guests; the panic
> -		 * message recording won't be available in isolated
> -		 * guests should the following registration fail.
> -		 */
> -		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.
> -		 */
> -		hyperv_crash_ctl = hv_get_register(HV_REGISTER_CRASH_CTL);
> -		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
> -			hv_kmsg_dump_register();
> -
> -		register_die_notifier(&hyperv_die_block);
> -	}
> -
> -	/*
> -	 * Always register the panic notifier because we need to unload
> -	 * the VMbus channel connection to prevent any VMbus
> -	 * activity after the VM panics.
> -	 */
> -	atomic_notifier_chain_register(&panic_notifier_list,
> -			       &hyperv_panic_block);
> -
>  	vmbus_request_offers();
>  
>  	return 0;
> @@ -2675,6 +2640,46 @@ static struct syscore_ops hv_synic_syscore_ops = {
>  	.resume = hv_synic_resume,
>  };
>  
> +static void __init crash_reporting_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;
> +		/*
> +		 * Panic message recording (sysctl_record_panic_msg)
> +		 * is enabled by default in non-isolated guests and
> +		 * disabled by default in isolated guests; the panic
> +		 * message recording won't be available in isolated
> +		 * guests should the following registration fail.
> +		 */
> +		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.
> +		 */
> +		hyperv_crash_ctl = hv_get_register(HV_REGISTER_CRASH_CTL);
> +		if (hyperv_crash_ctl & HV_CRASH_CTL_CRASH_NOTIFY_MSG)
> +			hv_kmsg_dump_register();
> +
> +		register_die_notifier(&hyperv_die_block);
> +	}
> +
> +	/*
> +	 * Always register the panic notifier because we need to unload
> +	 * the VMbus channel connection to prevent any VMbus
> +	 * activity after the VM panics.
> +	 */
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +			       &hyperv_panic_block);
> +
> +
> +}
> +
>  static int __init hv_acpi_init(void)
>  {
>  	int ret, t;
> @@ -2687,6 +2692,8 @@ static int __init hv_acpi_init(void)
>  
>  	init_completion(&probe_event);
>  
> +	crash_reporting_init();
> +
>  	/*
>  	 * Get ACPI resources first.
>  	 */

-- 
Vitaly