linux-kernel - Re: [PATCH v8 5/6] Drivers: hv: Add refcount and locking to mem regions

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: <e00b23dc-c053-4c0c-82cf-c670c557ba71@linux.microsoft.com>
Date: Wed, 3 Dec 2025 12:09:42 -0800
From: Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
Cc: kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
 decui@...rosoft.com, linux-hyperv@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 5/6] Drivers: hv: Add refcount and locking to mem
 regions

On 12/3/2025 11:55 AM, Stanislav Kinsburskii wrote:
> On Wed, Dec 03, 2025 at 11:26:23AM -0800, Nuno Das Neves wrote:
>> On 12/3/2025 10:24 AM, Stanislav Kinsburskii wrote:
>>> Introduce kref-based reference counting and spinlock protection for
>>> memory regions in Hyper-V partition management. This change improves
>>> memory region lifecycle management and ensures thread-safe access to the
>>> region list.
>>>
>>> Also improves the check for overlapped memory regions during region
>>> creation, preventing duplicate or conflicting mappings.
>>>
>>> Previously, the regions list was protected by the partition mutex.
>>> However, this approach is too heavy for frequent fault and invalidation
>>> operations. Finer grained locking is now used to improve efficiency and
>>> concurrency.
>>>
>>> This is a precursor to supporting movable memory regions. Fault and
>>> invalidation handling for movable regions will require safe traversal of
>>> the region list and holding a region reference while performing
>>> invalidation or fault operations.
>>>
>>> Signed-off-by: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
>>> ---
>>>  drivers/hv/mshv_regions.c   |   19 ++++++++++++++++---
>>>  drivers/hv/mshv_root.h      |    6 +++++-
>>>  drivers/hv/mshv_root_main.c |   32 ++++++++++++++++++++++++--------
>>>  3 files changed, 45 insertions(+), 12 deletions(-)
>>>
>>> @@ -1657,8 +1670,10 @@ static void destroy_partition(struct mshv_partition *partition)
>>>  	remove_partition(partition);
>>>  
>>>  	hlist_for_each_entry_safe(region, n, &partition->pt_mem_regions,
>>> -				  hnode)
>>> -		mshv_region_destroy(region);
>>> +				  hnode) {
>>> +		hlist_del(&region->hnode);
>>> +		mshv_region_put(region);
>>> +	}
>>>  
>>
>> With the following patch introducing movable memory, it looks like the
>> list could be traversed by mshv_partition_region_by_gfn() even while
>> the this hlist_del() is being called.
>>
>> Maybe that's not possible for some reason I'm unaware of, could you
>> explain why we don't need to spin_lock here for hlist_del()?
>> Or, alternatively, use hlist_for_each_entry_safe() in
>> mshv_partition_region_by_gfn() to guard against the deletion?
>>
> 
> This function (destroy_partition) is called when there are no active
> references for neither partition not its VPs (they are destroyed in the
> same function above).
> In other words, there can't be any callers for mshv_partition_region_by_gfn.

Ah, I see, even if the mmu_notifier is still active, it doesn't traverse the
list, as it gets the region by container_of().
Thanks.

> 
> As per mshv_partition_region_by_gfn function itself, the caller is
> supposed to take the lock.
> 
> Giving it more thought, I'm strating to think that rw lock her ewould be
> a better option than a spinlock + reference count, as regions won't be
> added or remove to often and using it would allow to get rid of
> reference counting.
> 
> However, this looks like an optimization that isn't required an its
> usefulness can be investigated in future.
> 
> Thanks,
> Stanislav
> 
>>>  	/* Withdraw and free all pages we deposited */
>>>  	hv_call_withdraw_memory(U64_MAX, NUMA_NO_NODE, partition->pt_id);
>>> @@ -1856,6 +1871,7 @@ mshv_ioctl_create_partition(void __user *user_arg, struct device *module_dev)
>>>  
>>>  	INIT_HLIST_HEAD(&partition->pt_devices);
>>>  
>>> +	spin_lock_init(&partition->pt_mem_regions_lock);
>>>  	INIT_HLIST_HEAD(&partition->pt_mem_regions);
>>>  
>>>  	mshv_eventfd_init(partition);
>>>
>>>