Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Michael Kelley <mikelley@microsoft.com>
Cc:     linux-kernel@vger.kernel.org, kys@microsoft.com,
        sthemmin@microsoft.com, wei.liu@kernel.org,
        linux-hyperv@vger.kernel.org
Subject: Re: ** POTENTIAL FRAUD ALERT - RED HAT ** [PATCH 1/1] Drivers: hv: vmbus: Add timeout to vmbus_wait_for_unload
In-Reply-To: <1600026449-23651-1-git-send-email-mikelley@microsoft.com>
References: <1600026449-23651-1-git-send-email-mikelley@microsoft.com>
Date:   Mon, 14 Sep 2020 12:09:36 +0200
Message-ID: <87imcgllen.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Michael Kelley <mikelley@microsoft.com> writes:

> vmbus_wait_for_unload() looks for a CHANNELMSG_UNLOAD_RESPONSE message
> coming from Hyper-V.  But if the message isn't found for some reason,
> the panic path gets hung forever.  Add a timeout of 10 seconds to prevent
> this.

If I remember correctly, the problem I was observing back then was that
if CHANNELMSG_UNLOAD_RESPONSE is not delivered, Hyper-V won't respond to
the consequent CHANNELMSG_INITIATE_CONTACT/CHANNELMSG_REQUESTOFFERS
(don't remember exactly) so we either hang here or crash in the kdump
kernel because we can't find any devices. Maybe the problem was only
with some ancient Hyper-V versions or it was fixed.

>
> Fixes: 415719160de3 ("Drivers: hv: vmbus: avoid scheduling in interrupt context in vmbus_initiate_unload()")
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
>  drivers/hv/channel_mgmt.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 591106c..1d44bb6 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -731,7 +731,7 @@ static void vmbus_wait_for_unload(void)
>  	void *page_addr;
>  	struct hv_message *msg;
>  	struct vmbus_channel_message_header *hdr;
> -	u32 message_type;
> +	u32 message_type, i;
>  
>  	/*
>  	 * CHANNELMSG_UNLOAD_RESPONSE is always delivered to the CPU which was
> @@ -741,8 +741,11 @@ static void vmbus_wait_for_unload(void)
>  	 * functional and vmbus_unload_response() will complete
>  	 * vmbus_connection.unload_event. If not, the last thing we can do is
>  	 * read message pages for all CPUs directly.
> +	 *
> +	 * Wait no more than 10 seconds so that the panic path can't get
> +	 * hung forever in case the response message isn't seen.
>  	 */
> -	while (1) {
> +	for (i = 0; i < 1000; i++) {
>  		if (completion_done(&vmbus_connection.unload_event))
>  			break;

LGTM,

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly