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: <aTCVsGcIYQmHjqM5@skinsburskii.localdomain>
Date: Wed, 3 Dec 2025 11:55:28 -0800
From: Stanislav Kinsburskii <skinsburskii@...ux.microsoft.com>
To: Nuno Das Neves <nunodasneves@...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 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.

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);
> > 
> >