[PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Sean Christopherson]

Add more context and information to the comment in kvm_gmem_release() that
explains why there's no synchronization on RCU _or_ kvm->srcu.  Point (b)
from commit 67b43038ce14 ("KVM: guest_memfd: Remove RCU-protected attribute
from slot->gmem.file")

      b) kvm->srcu ensures that kvm_gmem_unbind() and freeing of a memslot
         occur after the memslot is no longer visible to kvm_gmem_get_pfn().

is especially difficult to fully grok, particularly in light of commit
ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when
gmem is dying"), which addressed a race between unbind() and release().

See the extended on-list discussion[*] for more details about exactly what
KVM guards against, and how.

No functional change intended.

Link: https://lore.kernel.org/all/CAEvNRgGmyd1yqQXsnz5hWRZpBZUs%3DpiEWbEaqP9%2Bcz9ZqEMQ6g@mail.gmail.com [*]
Cc: Yan Zhao <yan.y.zhao@intel.com>
Cc: Vishal Annapurve <vannapurve@google.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

v2: Explain how this all works in even gorier detail. [Yan]

v1: https://lore.kernel.org/all/20251113232229.1698886-1-seanjc@google.com

 virt/kvm/guest_memfd.c | 50 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index b596486d184c..7f1c6a0f8039 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -300,17 +300,55 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 * dereferencing the slot for existing bindings needs to be protected
 	 * against memslot updates, specifically so that unbind doesn't race
 	 * and free the memslot (kvm_gmem_get_file() will return NULL).
-	 *
-	 * Since .release is called only when the reference count is zero,
-	 * after which file_ref_get() and get_file_active() fail,
-	 * kvm_gmem_get_pfn() cannot be using the file concurrently.
-	 * file_ref_put() provides a full barrier, and get_file_active() the
-	 * matching acquire barrier.
 	 */
 	mutex_lock(&kvm->slots_lock);
 
 	filemap_invalidate_lock(inode->i_mapping);
 
+	/*
+	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
+	 * bindings as slot->gmem.file cannot be set back to a non-null value
+	 * without the memslot first being deleted.  I.e. this relies on the
+	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
+	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
+	 * grab a reference to slot->gmem.file even if the struct file object
+	 * is reallocated.
+	 *
+	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
+	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
+	 * __get_file_rcu()) sees refcount==0 or fails the "file reloaded"
+	 * check (file != NULL due to nullifying the file pointer here).
+	 *
+	 * Unlike most other users of get_file_rcu(), where callers don't care
+	 * if they race with a write, only that they have a reference to _a_
+	 * live file, kvm_gmem_get_pfn() needs to get the exact file that is
+	 * associated with the memslot.  Without the aforementioned SRCU
+	 * synchronization, the following could happen:
+	 *
+	 *  CPU0				CPU1
+	 *  kvm_gmem_get_pfn()
+	 *    f = X (from slot->gmem.file)
+	 *					kvm_gmem_release())
+	 *					  slot->gmem.file = NULL
+	 *
+	 *					kvm_set_memory_region()
+	 *					  slot deleted
+	 *
+	 *					kvm_set_memory_region()
+	 *					  slot created
+	 *					  slot->gmem.file = f (alloc the same object)
+	 *
+	 *  get_file_active()
+	 *    file = f
+	 *    file_reloaded = f
+	 *
+	 * <KVM does weird things with an old memslot+file>
+	 *
+	 * Obviously KVM would be broken in many places if the synchronization
+	 * were omitted, but it's important to note that get_file_active() does
+	 * NOT guarantee a reference to the correct file was obtained, only
+	 * that the file doesn't point at a reallocated object.
+	 */
 	xa_for_each(&f->bindings, index, slot)
 		WRITE_ONCE(slot->gmem.file, NULL);
 

base-commit: 76671054f9a1ff6abb976583cd8da37650acdc97
-- 
2.55.0.887.g758fc8c411-goog

Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Yan Zhao]

On Wed, Aug 26, 2026 at 09:56:47AM -0700, Sean Christopherson wrote:
> Add more context and information to the comment in kvm_gmem_release() that
> explains why there's no synchronization on RCU _or_ kvm->srcu.  Point (b)
> from commit 67b43038ce14 ("KVM: guest_memfd: Remove RCU-protected attribute
> from slot->gmem.file")
> 
>       b) kvm->srcu ensures that kvm_gmem_unbind() and freeing of a memslot
>          occur after the memslot is no longer visible to kvm_gmem_get_pfn().
> 
> is especially difficult to fully grok, particularly in light of commit
> ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when
> gmem is dying"), which addressed a race between unbind() and release().
> 
> See the extended on-list discussion[*] for more details about exactly what
> KVM guards against, and how.
> 
> No functional change intended.
> 
> Link: https://lore.kernel.org/all/CAEvNRgGmyd1yqQXsnz5hWRZpBZUs%3DpiEWbEaqP9%2Bcz9ZqEMQ6g@mail.gmail.com [*]
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Cc: Vishal Annapurve <vannapurve@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> v2: Explain how this all works in even gorier detail. [Yan]
> 
> v1: https://lore.kernel.org/all/20251113232229.1698886-1-seanjc@google.com
> 
>  virt/kvm/guest_memfd.c | 50 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index b596486d184c..7f1c6a0f8039 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -300,17 +300,55 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
>  	 * dereferencing the slot for existing bindings needs to be protected
>  	 * against memslot updates, specifically so that unbind doesn't race
>  	 * and free the memslot (kvm_gmem_get_file() will return NULL).
> -	 *
> -	 * Since .release is called only when the reference count is zero,
> -	 * after which file_ref_get() and get_file_active() fail,
> -	 * kvm_gmem_get_pfn() cannot be using the file concurrently.
> -	 * file_ref_put() provides a full barrier, and get_file_active() the
> -	 * matching acquire barrier.
>  	 */
>  	mutex_lock(&kvm->slots_lock);
>  
>  	filemap_invalidate_lock(inode->i_mapping);
>  
> +	/*
> +	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
> +	 * bindings as slot->gmem.file cannot be set back to a non-null value
> +	 * without the memslot first being deleted.  I.e. this relies on the
> +	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
> +	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
> +	 * grab a reference to slot->gmem.file even if the struct file object
> +	 * is reallocated.

Nit:

Do we need to update the code comment in __kvm_gmem_unbind() from
	/*
	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
	 * cannot see this memslot.
	 */
to
	/*
	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() ensured
	 * that kvm_gmem_get_pfn() cannot see this memslot.
	 */

to align with the above comment.

> +	 *
> +	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
> +	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
> +	 * __get_file_rcu()) sees refcount==0 or fails the "file reloaded"
> +	 * check (file != NULL due to nullifying the file pointer here).
> +	 *
> +	 * Unlike most other users of get_file_rcu(), where callers don't care
> +	 * if they race with a write, only that they have a reference to _a_
> +	 * live file, kvm_gmem_get_pfn() needs to get the exact file that is
> +	 * associated with the memslot.  Without the aforementioned SRCU
> +	 * synchronization, the following could happen:
> +	 *
> +	 *  CPU0				CPU1
> +	 *  kvm_gmem_get_pfn()
> +	 *    f = X (from slot->gmem.file)
> +	 *					kvm_gmem_release())
> +	 *					  slot->gmem.file = NULL
> +	 *
> +	 *					kvm_set_memory_region()
> +	 *					  slot deleted
> +	 *
> +	 *					kvm_set_memory_region()
> +	 *					  slot created
> +	 *					  slot->gmem.file = f (alloc the same object)
> +	 *
> +	 *  get_file_active()
> +	 *    file = f
> +	 *    file_reloaded = f
> +	 *
> +	 * <KVM does weird things with an old memslot+file>
> +	 *
> +	 * Obviously KVM would be broken in many places if the synchronization
> +	 * were omitted, but it's important to note that get_file_active() does
> +	 * NOT guarantee a reference to the correct file was obtained, only
> +	 * that the file doesn't point at a reallocated object.
reallocated -> reloaded?

> +	 */
>  	xa_for_each(&f->bindings, index, slot)
>  		WRITE_ONCE(slot->gmem.file, NULL);
With the nits addressed,

Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>

Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Sean Christopherson]

On Sun, Sep 20, 2026, Yan Zhao wrote:
> On Wed, Aug 26, 2026 at 09:56:47AM -0700, Sean Christopherson wrote:
> > Add more context and information to the comment in kvm_gmem_release() that
> > explains why there's no synchronization on RCU _or_ kvm->srcu.  Point (b)
> > from commit 67b43038ce14 ("KVM: guest_memfd: Remove RCU-protected attribute
> > from slot->gmem.file")
> > 
> >       b) kvm->srcu ensures that kvm_gmem_unbind() and freeing of a memslot
> >          occur after the memslot is no longer visible to kvm_gmem_get_pfn().
> > 
> > is especially difficult to fully grok, particularly in light of commit
> > ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when
> > gmem is dying"), which addressed a race between unbind() and release().
> > 
> > See the extended on-list discussion[*] for more details about exactly what
> > KVM guards against, and how.
> > 
> > No functional change intended.
> > 
> > Link: https://lore.kernel.org/all/CAEvNRgGmyd1yqQXsnz5hWRZpBZUs%3DpiEWbEaqP9%2Bcz9ZqEMQ6g@mail.gmail.com [*]
> > Cc: Yan Zhao <yan.y.zhao@intel.com>
> > Cc: Vishal Annapurve <vannapurve@google.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> > 
> > v2: Explain how this all works in even gorier detail. [Yan]
> > 
> > v1: https://lore.kernel.org/all/20251113232229.1698886-1-seanjc@google.com
> > 
> >  virt/kvm/guest_memfd.c | 50 +++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index b596486d184c..7f1c6a0f8039 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -300,17 +300,55 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
> >  	 * dereferencing the slot for existing bindings needs to be protected
> >  	 * against memslot updates, specifically so that unbind doesn't race
> >  	 * and free the memslot (kvm_gmem_get_file() will return NULL).
> > -	 *
> > -	 * Since .release is called only when the reference count is zero,
> > -	 * after which file_ref_get() and get_file_active() fail,
> > -	 * kvm_gmem_get_pfn() cannot be using the file concurrently.
> > -	 * file_ref_put() provides a full barrier, and get_file_active() the
> > -	 * matching acquire barrier.
> >  	 */
> >  	mutex_lock(&kvm->slots_lock);
> >  
> >  	filemap_invalidate_lock(inode->i_mapping);
> >  
> > +	/*
> > +	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
> > +	 * bindings as slot->gmem.file cannot be set back to a non-null value
> > +	 * without the memslot first being deleted.  I.e. this relies on the
> > +	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
> > +	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
> > +	 * grab a reference to slot->gmem.file even if the struct file object
> > +	 * is reallocated.
> 
> Nit:
> 
> Do we need to update the code comment in __kvm_gmem_unbind() from
> 	/*
> 	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
> 	 * cannot see this memslot.
> 	 */
> to
> 	/*
> 	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() ensured
> 	 * that kvm_gmem_get_pfn() cannot see this memslot.
> 	 */
> 
> to align with the above comment.

How about this?  Because the "rule" is that kvm_gmem_unbind() can only be called
on a memslot that is unreachable, either by synchronizing SRCU after uninstalling
the memslot *or* because the memslot was never installed.  Simply stating that
synchronize_srcu_expedited() makes everything safe isn't the whole story, as it's
specifically synchronzing after removing/deleting/deactivating the slot that
makes this safe.

	/*
	 * Note, the caller is responsible for ensuring the slot is unreachable
	 * before unbinding, e.g. by synchronizing SRCU after deleting the slot.
	 */

> > +	 *
> > +	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
> > +	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
> > +	 * __get_file_rcu()) sees refcount==0 or fails the "file reloaded"
> > +	 * check (file != NULL due to nullifying the file pointer here).
> > +	 *
> > +	 * Unlike most other users of get_file_rcu(), where callers don't care
> > +	 * if they race with a write, only that they have a reference to _a_
> > +	 * live file, kvm_gmem_get_pfn() needs to get the exact file that is
> > +	 * associated with the memslot.  Without the aforementioned SRCU
> > +	 * synchronization, the following could happen:
> > +	 *
> > +	 *  CPU0				CPU1
> > +	 *  kvm_gmem_get_pfn()
> > +	 *    f = X (from slot->gmem.file)
> > +	 *					kvm_gmem_release())
> > +	 *					  slot->gmem.file = NULL
> > +	 *
> > +	 *					kvm_set_memory_region()
> > +	 *					  slot deleted
> > +	 *
> > +	 *					kvm_set_memory_region()
> > +	 *					  slot created
> > +	 *					  slot->gmem.file = f (alloc the same object)
> > +	 *
> > +	 *  get_file_active()
> > +	 *    file = f
> > +	 *    file_reloaded = f
> > +	 *
> > +	 * <KVM does weird things with an old memslot+file>
> > +	 *
> > +	 * Obviously KVM would be broken in many places if the synchronization
> > +	 * were omitted, but it's important to note that get_file_active() does
> > +	 * NOT guarantee a reference to the correct file was obtained, only
> > +	 * that the file doesn't point at a reallocated object.
> reallocated -> reloaded?

No, "reallocated" is correct.  From the comment for SLAB_TYPESAFE_BY_RCU:

  * This delays freeing the SLAB page by a grace period, it does _NOT_
  * delay object freeing. This means that if you do kmem_cache_free()
  * that memory location is free to be reused at any time. Thus it may
  * be possible to see another object there in the same RCU grace period.
  *
  * This feature only ensures the memory location backing the object
  * stays valid, the trick to using this is relying on an independent
  * object validation pass. Something like:
  *
  * ::
  *
  *  begin:
  *   rcu_read_lock();
  *   obj = lockless_lookup(key);
  *   if (obj) {
  *     if (!try_get_ref(obj)) // might fail for free objects
  *       rcu_read_unlock();
  *       goto begin;
  *
  *     if (obj->key != key) { // not the object we expected
  *       put_ref(obj);
  *       rcu_read_unlock();
  *       goto begin;
  *     }
  *   }
  *  rcu_read_unlock();

The above pseudocode is what get_file_active() is doing; it verifies the found
file ("obj" above) is the correct file.  Specifically, from __get_file_rcu():

	 * If the pointers don't match the file has been reallocated by
	 * SLAB_TYPESAFE_BY_RCU.

Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Sean Christopherson]

On Mon, Sep 21, 2026, Sean Christopherson wrote:
> On Sun, Sep 20, 2026, Yan Zhao wrote:
> > On Wed, Aug 26, 2026 at 09:56:47AM -0700, Sean Christopherson wrote:
> > Do we need to update the code comment in __kvm_gmem_unbind() from
> > 	/*
> > 	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
> > 	 * cannot see this memslot.
> > 	 */
> > to
> > 	/*
> > 	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() ensured
> > 	 * that kvm_gmem_get_pfn() cannot see this memslot.
> > 	 */
> > 
> > to align with the above comment.
> 
> How about this?  Because the "rule" is that kvm_gmem_unbind() can only be called
> on a memslot that is unreachable, either by synchronizing SRCU after uninstalling
> the memslot *or* because the memslot was never installed.  Simply stating that
> synchronize_srcu_expedited() makes everything safe isn't the whole story, as it's
> specifically synchronzing after removing/deleting/deactivating the slot that
> makes this safe.
> 
> 	/*
> 	 * Note, the caller is responsible for ensuring the slot is unreachable
> 	 * before unbinding, e.g. by synchronizing SRCU after deleting the slot.
> 	 */

Hmm, though it's probably a good idea to preserve the connection to kvm_gmem_get_pfn():

	/*
	 * Note, the caller is responsible for ensuring the slot is unreachable
	 * before unbinding, e.g. by synchronizing SRCU after deleting the slot,
	 * to guarantee kvm_gmem_get_pfn() can't see the slot+file.
	 */

Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Yan Zhao]

On Mon, Sep 21, 2026 at 06:43:52AM -0700, Sean Christopherson wrote:
> On Mon, Sep 21, 2026, Sean Christopherson wrote:
> > On Sun, Sep 20, 2026, Yan Zhao wrote:
> > > On Wed, Aug 26, 2026 at 09:56:47AM -0700, Sean Christopherson wrote:
> > > Do we need to update the code comment in __kvm_gmem_unbind() from
> > > 	/*
> > > 	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
> > > 	 * cannot see this memslot.
> > > 	 */
> > > to
> > > 	/*
> > > 	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() ensured
> > > 	 * that kvm_gmem_get_pfn() cannot see this memslot.
> > > 	 */
> > > 
> > > to align with the above comment.
> > 
> > How about this?  Because the "rule" is that kvm_gmem_unbind() can only be called
> > on a memslot that is unreachable, either by synchronizing SRCU after uninstalling
> > the memslot *or* because the memslot was never installed.  Simply stating that
> > synchronize_srcu_expedited() makes everything safe isn't the whole story, as it's
> > specifically synchronzing after removing/deleting/deactivating the slot that
> > makes this safe.
> > 
> > 	/*
> > 	 * Note, the caller is responsible for ensuring the slot is unreachable
> > 	 * before unbinding, e.g. by synchronizing SRCU after deleting the slot.
> > 	 */
> 
> Hmm, though it's probably a good idea to preserve the connection to kvm_gmem_get_pfn():
> 
> 	/*
> 	 * Note, the caller is responsible for ensuring the slot is unreachable
> 	 * before unbinding, e.g. by synchronizing SRCU after deleting the slot,
> 	 * to guarantee kvm_gmem_get_pfn() can't see the slot+file.
> 	 */
LGTM.

Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Yan Zhao]

On Mon, Sep 21, 2026 at 06:40:15AM -0700, Sean Christopherson wrote:
> > > +	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
> > > +	 * bindings as slot->gmem.file cannot be set back to a non-null value
> > > +	 * without the memslot first being deleted.  I.e. this relies on the
> > > +	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
> > > +	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
> > > +	 * grab a reference to slot->gmem.file even if the struct file object
> > > +	 * is reallocated.
Does "reallocated" here refer to the case (*) below?

> > > +	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
> > > +	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
> > > +	 * __get_file_rcu()) sees refcount==0 or fails the "file reloaded"
> > > +	 * check (file != NULL due to nullifying the file pointer here).
> > > +	 *
> > > +	 * Unlike most other users of get_file_rcu(), where callers don't care
> > > +	 * if they race with a write, only that they have a reference to _a_
> > > +	 * live file, kvm_gmem_get_pfn() needs to get the exact file that is
> > > +	 * associated with the memslot.  Without the aforementioned SRCU
> > > +	 * synchronization, the following could happen:
> > > +	 *
> > > +	 *  CPU0				CPU1
> > > +	 *  kvm_gmem_get_pfn()
> > > +	 *    f = X (from slot->gmem.file)
> > > +	 *					kvm_gmem_release())
> > > +	 *					  slot->gmem.file = NULL
> > > +	 *
> > > +	 *					kvm_set_memory_region()
> > > +	 *					  slot deleted
> > > +	 *
> > > +	 *					kvm_set_memory_region()
> > > +	 *					  slot created
> > > +	 *					  slot->gmem.file = f (alloc the same object)
Case (*):

I thought "reallocated" refers to this case, which get_file_active() cannot
guarantee against.

> > > +	 *
> > > +	 *  get_file_active()
> > > +	 *    file = f
> > > +	 *    file_reloaded = f
> > > +	 *
> > > +	 * <KVM does weird things with an old memslot+file>
> > > +	 *
> > > +	 * Obviously KVM would be broken in many places if the synchronization
> > > +	 * were omitted, but it's important to note that get_file_active() does
> > > +	 * NOT guarantee a reference to the correct file was obtained, only
> > > +	 * that the file doesn't point at a reallocated object.
> > reallocated -> reloaded?
> 
> No, "reallocated" is correct.  From the comment for SLAB_TYPESAFE_BY_RCU:
> 
>   * This delays freeing the SLAB page by a grace period, it does _NOT_
>   * delay object freeing. This means that if you do kmem_cache_free()
>   * that memory location is free to be reused at any time. Thus it may
>   * be possible to see another object there in the same RCU grace period.
>   *
>   * This feature only ensures the memory location backing the object
>   * stays valid, the trick to using this is relying on an independent
>   * object validation pass. Something like:
>   *
>   * ::
>   *
>   *  begin:
>   *   rcu_read_lock();
>   *   obj = lockless_lookup(key);
>   *   if (obj) {
>   *     if (!try_get_ref(obj)) // might fail for free objects
>   *       rcu_read_unlock();
>   *       goto begin;
>   *
>   *     if (obj->key != key) { // not the object we expected
>   *       put_ref(obj);
>   *       rcu_read_unlock();
>   *       goto begin;
>   *     }
>   *   }
>   *  rcu_read_unlock();
> 
> The above pseudocode is what get_file_active() is doing; it verifies the found
> file ("obj" above) is the correct file.  Specifically, from __get_file_rcu():
> 
> 	 * If the pointers don't match the file has been reallocated by
> 	 * SLAB_TYPESAFE_BY_RCU.
Yeah, I understand this is what you mean by "reallocated" :)
However, I think case (*) above could also be considered a form of "reallocated".
Would it make sense to rename "reallocated" to "reassigned" in case (*)?

Re: [PATCH v2] KVM: guest_memfd: Elaborate on how release() vs. get_pfn() is safe against UAF

[Sean Christopherson]

On Tue, Sep 22, 2026, Yan Zhao wrote:
> On Mon, Sep 21, 2026 at 06:40:15AM -0700, Sean Christopherson wrote:
> > > > +	 * Note!  synchronize_srcu() is _not_ needed after nullifying memslot
> > > > +	 * bindings as slot->gmem.file cannot be set back to a non-null value
> > > > +	 * without the memslot first being deleted.  I.e. this relies on the
> > > > +	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
> > > > +	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
> > > > +	 * grab a reference to slot->gmem.file even if the struct file object
> > > > +	 * is reallocated.
> Does "reallocated" here refer to the case (*) below?

Yes.  I'll clarify this with:

		 * is reallocated (for use as a different file).

> > > > +	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
> > > > +	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
> > > > +	 * __get_file_rcu()) sees refcount==0 or fails the "file reloaded"
> > > > +	 * check (file != NULL due to nullifying the file pointer here).
> > > > +	 *
> > > > +	 * Unlike most other users of get_file_rcu(), where callers don't care
> > > > +	 * if they race with a write, only that they have a reference to _a_
> > > > +	 * live file, kvm_gmem_get_pfn() needs to get the exact file that is
> > > > +	 * associated with the memslot.  Without the aforementioned SRCU
> > > > +	 * synchronization, the following could happen:
> > > > +	 *
> > > > +	 *  CPU0				CPU1
> > > > +	 *  kvm_gmem_get_pfn()
> > > > +	 *    f = X (from slot->gmem.file)
> > > > +	 *					kvm_gmem_release())
> > > > +	 *					  slot->gmem.file = NULL
> > > > +	 *
> > > > +	 *					kvm_set_memory_region()
> > > > +	 *					  slot deleted
> > > > +	 *
> > > > +	 *					kvm_set_memory_region()
> > > > +	 *					  slot created
> > > > +	 *					  slot->gmem.file = f (alloc the same object)
> Case (*):
> 
> I thought "reallocated" refers to this case, which get_file_active() cannot
> guarantee against.

Ya.

> > > > +	 *
> > > > +	 *  get_file_active()
> > > > +	 *    file = f
> > > > +	 *    file_reloaded = f
> > > > +	 *
> > > > +	 * <KVM does weird things with an old memslot+file>
> > > > +	 *
> > > > +	 * Obviously KVM would be broken in many places if the synchronization
> > > > +	 * were omitted, but it's important to note that get_file_active() does
> > > > +	 * NOT guarantee a reference to the correct file was obtained, only
> > > > +	 * that the file doesn't point at a reallocated object.
> > > reallocated -> reloaded?
> > 
> > No, "reallocated" is correct.  From the comment for SLAB_TYPESAFE_BY_RCU:
> > 
> >   * This delays freeing the SLAB page by a grace period, it does _NOT_
> >   * delay object freeing. This means that if you do kmem_cache_free()
> >   * that memory location is free to be reused at any time. Thus it may
> >   * be possible to see another object there in the same RCU grace period.
> >   *
> >   * This feature only ensures the memory location backing the object
> >   * stays valid, the trick to using this is relying on an independent
> >   * object validation pass. Something like:
> >   *
> >   * ::
> >   *
> >   *  begin:
> >   *   rcu_read_lock();
> >   *   obj = lockless_lookup(key);
> >   *   if (obj) {
> >   *     if (!try_get_ref(obj)) // might fail for free objects
> >   *       rcu_read_unlock();
> >   *       goto begin;
> >   *
> >   *     if (obj->key != key) { // not the object we expected
> >   *       put_ref(obj);
> >   *       rcu_read_unlock();
> >   *       goto begin;
> >   *     }
> >   *   }
> >   *  rcu_read_unlock();
> > 
> > The above pseudocode is what get_file_active() is doing; it verifies the found
> > file ("obj" above) is the correct file.  Specifically, from __get_file_rcu():
> > 
> > 	 * If the pointers don't match the file has been reallocated by
> > 	 * SLAB_TYPESAFE_BY_RCU.
> Yeah, I understand this is what you mean by "reallocated" :)
> However, I think case (*) above could also be considered a form of "reallocated".
> Would it make sense to rename "reallocated" to "reassigned" in case (*)?

Oooh, I see what you're getting at.  Yes, I agree, "reallocated" isn't the best
description here.  Hmm, but neither is "reloaded" or "reassigned", because it
doesn't fully guard against the pointer being reassigned, e.g. if the pointer is
reassigned before the the first load.

How about this?

	 * Obviously KVM would be broken in many places if the synchronization
	 * were omitted, but it's important to note that get_file_active() does
	 * NOT guarantee a reference to the "original" file was obtained, only
	 * that it grabbed a reference for the returned file, i.e. didn't grab
	 * a reference for file A, but then returned a pointer to file B.

And the whole thing as a diff:

diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 9a112daa5df9..483998f0df58 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -312,7 +312,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 * synchronize_srcu_expedited() in kvm_swap_active_memslots() to ensure
 	 * kvm_gmem_get_pfn() (which runs with kvm->srcu held for read) can't
 	 * grab a reference to slot->gmem.file even if the struct file object
-	 * is reallocated.
+	 * is reallocated (for use as a different file).
 	 *
 	 * file_ref_put() provides a full barrier, and __get_file_rcu() the
 	 * matching acquire barrier, to ensure that kvm_gmem_get_file() (via
@@ -327,7 +327,7 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 *
 	 *  CPU0				CPU1
 	 *  kvm_gmem_get_pfn()
-	 *    f = X (from slot->gmem.file)
+	 *    slot->gmem.file == A
 	 *					kvm_gmem_release())
 	 *					  slot->gmem.file = NULL
 	 *
@@ -336,18 +336,19 @@ static int kvm_gmem_release(struct inode *inode, struct file *file)
 	 *
 	 *					kvm_set_memory_region()
 	 *					  slot created
-	 *					  slot->gmem.file = f (alloc the same object)
+	 *					  slot->gmem.file = B (alloc the same object)
 	 *
 	 *  get_file_active()
-	 *    file = f
-	 *    file_reloaded = f
+	 *    file = B
+	 *    file_reloaded = B
 	 *
 	 * <KVM does weird things with an old memslot+file>
 	 *
 	 * Obviously KVM would be broken in many places if the synchronization
 	 * were omitted, but it's important to note that get_file_active() does
-	 * NOT guarantee a reference to the correct file was obtained, only
-	 * that the file doesn't point at a reallocated object.
+	 * NOT guarantee a reference to the "original" file was obtained, only
+	 * that it grabbed a reference for the returned file, i.e. didn't grab
+	 * a reference for file A, but then returned a pointer to file B.
 	 */
 	xa_for_each(&f->bindings, index, slot)
 		WRITE_ONCE(slot->gmem.file, NULL);