Re: [PATCH v3 1/1] KVM: s390: pv: fix race when making a page secure

[David Hildenbrand <david@redhat.com>]

On 27.02.25 14:09, Claudio Imbrenda wrote:
> Holding the pte lock for the page that is being converted to secure is
> needed to avoid races. A previous commit removed the locking, which
> caused issues. Fix by locking the pte again.
> 
> Fixes: 5cbe24350b7d ("KVM: s390: move pv gmap functions into kvm")
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Tested with shmem / memory-backend-memfd that ends up using large folios 
/ THPs.

Tested-by: David Hildenbrand <david@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>

Two comments below.

[...]

> +
> +int make_hva_secure(struct mm_struct *mm, unsigned long hva, struct uv_cb_header *uvcb)
> +{
> +	struct folio *folio;
> +	spinlock_t *ptelock;
> +	pte_t *ptep;
> +	int rc;
> +
> +	ptep = get_locked_valid_pte(mm, hva, &ptelock);
> +	if (!ptep)
> +		return -ENXIO;
> +
> +	folio = page_folio(pte_page(*ptep));
> +	folio_get(folio);

Grabbing a folio reference is only required if you want to keep using 
the folio after the pte_unmap_unlock. While the PTL is locked it cannot 
vanish.

So consider grabbing a reference only before dropping the PTL and you 
inted to call kvm_s390_wiggle_split_folio(). Then, you would effectively 
not require these two atomics on the expected hot path.

(I recall that the old code did that)

> +	/*
> +	 * Secure pages cannot be huge and userspace should not combine both.
> +	 * In case userspace does it anyway this will result in an -EFAULT for
> +	 * the unpack. The guest is thus never reaching secure mode.
> +	 * If userspace plays dirty tricks and decides to map huge pages at a
> +	 * later point in time, it will receive a segmentation fault or
> +	 * KVM_RUN will return -EFAULT.
> +	 */
> +	if (folio_test_hugetlb(folio))
> +		rc =  -EFAULT;
> +	else if (folio_test_large(folio))
> +		rc = -E2BIG;
> +	else if (!pte_write(*ptep))
> +		rc = -ENXIO;
> +	else
> +		rc = make_folio_secure(mm, folio, uvcb);
> +	pte_unmap_unlock(ptep, ptelock);
> +
> +	if (rc == -E2BIG || rc == -EBUSY)
> +		rc = kvm_s390_wiggle_split_folio(mm, folio, rc == -E2BIG);
> +	folio_put(folio);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(make_hva_secure);
>   
>   /*
>    * To be called with the folio locked or with an extra reference! This will
> diff --git a/arch/s390/kvm/gmap.c b/arch/s390/kvm/gmap.c
> index 02adf151d4de..c08950b4301c 100644


There is one remaining reference to __gmap_make_secure, which you remove:

$ git grep __gmap_make_secure
arch/s390/kvm/gmap.c: * Return: 0 on success, < 0 in case of error (see 
__gmap_make_secure()).



-- 
Cheers,

David / dhildenb