| 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
| ||
|
Message-ID: <1246e9a8-1a71-4cd4-a55c-5d9cb25f2312@linux.microsoft.com>
Date: Tue, 5 Nov 2024 11:10:44 +0530
From: Naman Jain <namjain@...ux.microsoft.com>
To: Easwar Hariharan <eahariha@...ux.microsoft.com>,
"K . Y . Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>, Wei Liu <wei.liu@...nel.org>,
Dexuan Cui <decui@...rosoft.com>
Cc: linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
John Starks <jostarks@...rosoft.com>, jacob.pan@...ux.microsoft.com,
Michael Kelley <mhklinux@...look.com>,
Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
Subject: Re: [PATCH v2 1/2] Drivers: hv: vmbus: Wait for offers during boot
On 10/31/2024 2:01 AM, Easwar Hariharan wrote:
> On 10/29/2024 1:01 AM, Naman Jain wrote:
>> Channel offers are requested during VMBus initialization and resume
>> from hibernation. Add support to wait for all channel offers to be
>> delivered and processed before returning from vmbus_request_offers.
>>
>> This is in analogy to a PCI bus not returning from probe until it has
>> scanned all devices on the bus.
>>
>> Without this, user mode can race with VMBus initialization and miss
>> channel offers. User mode has no way to work around this other than
>> sleeping for a while, since there is no way to know when VMBus has
>> finished processing offers.
>>
>> With this added functionality, remove earlier logic which keeps track
>> of count of offered channels post resume from hibernation. Once all
>> offers delivered message is received, no further offers are going to
>> be received. Consequently, logic to prevent suspend from happening
>> after previous resume had missing offers, is also removed.
>>
>> Co-developed-by: John Starks <jostarks@...rosoft.com>
>> Signed-off-by: John Starks <jostarks@...rosoft.com>
>> Signed-off-by: Naman Jain <namjain@...ux.microsoft.com>
>> Reviewed-by: Easwar Hariharan <eahariha@...ux.microsoft.com>
>> ---
>> Changes since v1:
>> https://lore.kernel.org/all/20241018115811.5530-1-namjain@linux.microsoft.com
>> * Added Easwar's Reviewed-By tag
>> * Addressed Michael's comments:
>> * Added explanation of all offers delivered message in comments
>> * Removed infinite wait for offers logic, and changed it wait once.
>> * Removed sub channel workqueue flush logic
>> * Added comments on why MLX device offer is not expected as part of
>> this essential boot offer list. I refrained from adding too many details on it as it felt like it is beyond the scope of this patch series and may not be relevant to this. However, please let me know if
>> something needs to be added.
>> * Addressed Saurabh's comments:
>> * Changed timeout value to 10000 ms instead of 10*10000
>> * Changed commit msg as per suggestions
>> * Added a comment for warning case of wait_for_completion timeout
>> ---
>> drivers/hv/channel_mgmt.c | 55 ++++++++++++++++++++++++++++-----------
>> drivers/hv/connection.c | 4 +--
>> drivers/hv/hyperv_vmbus.h | 14 +++-------
>> drivers/hv/vmbus_drv.c | 16 ------------
>> 4 files changed, 45 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index 3c6011a48dab..a2e9ebe5bf72 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -944,16 +944,6 @@ void vmbus_initiate_unload(bool crash)
>> vmbus_wait_for_unload();
>> }
>>
>> -static void check_ready_for_resume_event(void)
>> -{
>> - /*
>> - * If all the old primary channels have been fixed up, then it's safe
>> - * to resume.
>> - */
>> - if (atomic_dec_and_test(&vmbus_connection.nr_chan_fixup_on_resume))
>> - complete(&vmbus_connection.ready_for_resume_event);
>> -}
>> -
>> static void vmbus_setup_channel_state(struct vmbus_channel *channel,
>> struct vmbus_channel_offer_channel *offer)
>> {
>> @@ -1109,8 +1099,6 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
>>
>> /* Add the channel back to the array of channels. */
>> vmbus_channel_map_relid(oldchannel);
>> - check_ready_for_resume_event();
>> -
>> mutex_unlock(&vmbus_connection.channel_mutex);
>> return;
>> }
>> @@ -1296,13 +1284,22 @@ EXPORT_SYMBOL_GPL(vmbus_hvsock_device_unregister);
>>
>> /*
>> * vmbus_onoffers_delivered -
>> - * This is invoked when all offers have been delivered.
>> + * CHANNELMSG_ALLOFFERS_DELIVERED message arrives after all the essential
>> + * boot-time offers are delivered. Other channels can be hot added
>> + * or removed later, even immediately after the all-offers-delivered
>> + * message. A boot-time offer will be any of the virtual hardware the
>> + * VM is configured with at boot.
>> *
>> - * Nothing to do here.
>> + * Virtual devices like Mellanox NIC may not be included in the list of
>> + * these initial boot offers because it is an optional accelerator to
>> + * the synthetic VMBus NIC. It is hot added only after the VMBus NIC
>> + * channel is opened (once it knows the guest can support it, via the
>> + * sriov bit in the netvsc protocol).
>> */
>> static void vmbus_onoffers_delivered(
>> struct vmbus_channel_message_header *hdr)
>> {
>> + complete(&vmbus_connection.all_offers_delivered_event);
>> }
>>
>> /*
>> @@ -1578,7 +1575,8 @@ void vmbus_onmessage(struct vmbus_channel_message_header *hdr)
>> }
>>
>> /*
>> - * vmbus_request_offers - Send a request to get all our pending offers.
>> + * vmbus_request_offers - Send a request to get all our pending offers
>> + * and wait for all offers to arrive.
>> */
>> int vmbus_request_offers(void)
>> {
>> @@ -1596,6 +1594,10 @@ int vmbus_request_offers(void)
>>
>> msg->msgtype = CHANNELMSG_REQUESTOFFERS;
>>
>> + /*
>> + * This REQUESTOFFERS message will result in the host sending an all
>> + * offers delivered message.
>> + */
>> ret = vmbus_post_msg(msg, sizeof(struct vmbus_channel_message_header),
>> true);
>>
>> @@ -1607,6 +1609,29 @@ int vmbus_request_offers(void)
>> goto cleanup;
>> }
>>
>> + /*
>> + * Wait for the host to send all offers.
>> + * Keeping it as a best-effort mechanism, where a warning is
>> + * printed if a timeout occurs, and execution is resumed.
>> + */
>> + if (!wait_for_completion_timeout(
>> + &vmbus_connection.all_offers_delivered_event, msecs_to_jiffies(10000))) {
>> + pr_warn("timed out waiting for all offers to be delivered...\n");
>> + }
>
> secs_to_jiffies() has been merged [1] so please update this to a call to
> secs_to_jiffies(10). A Cocinelle script will probably take care of it
> sooner or later in any case.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=b35108a51cf7bab58d7eace1267d7965978bcdb8
>
Thank you. I will update it.
Regards,
Naman
Powered by blists - more mailing lists