linux-kernel - Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page secure

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: <20250214111729.000d364e@p-imbrenda>
Date: Fri, 14 Feb 2025 11:17:29 +0100
From: Claudio Imbrenda <imbrenda@...ux.ibm.com>
To: David Hildenbrand <david@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org, frankja@...ux.ibm.com,
        borntraeger@...ibm.com, nrb@...ux.ibm.com, seiden@...ux.ibm.com,
        nsg@...ux.ibm.com, schlameuss@...ux.ibm.com, hca@...ux.ibm.com
Subject: Re: [PATCH v1 2/2] KVM: s390: pv: fix race when making a page
 secure

On Thu, 13 Feb 2025 21:16:03 +0100
David Hildenbrand <david@...hat.com> wrote:

> On 13.02.25 21:07, 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")  
> 
> If you found this because of my report about the changed locking, 
> consider adding a Suggested-by / Reported-y.

yes, sorry; I sent the patch in haste and forgot. Which one would you
prefer (or both?)

[...]

> > @@ -127,8 +128,11 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb)
> >   
> >   	page = gfn_to_page(kvm, gpa_to_gfn(gaddr));
> >   	mmap_read_lock(gmap->mm);
> > -	if (page)
> > -		rc = __gmap_make_secure(gmap, page, uvcb);
> > +	vmaddr = gfn_to_hva(gmap->private, gpa_to_gfn(gaddr));
> > +	if (kvm_is_error_hva(vmaddr))
> > +		rc = -ENXIO;
> > +	if (!rc && page)
> > +		rc = __gmap_make_secure(gmap, page, vmaddr, uvcb);
> >   	kvm_release_page_clean(page);
> >   	mmap_read_unlock(gmap->mm);
> >     
> 
> You effectively make the code more complicated and inefficient than 
> before. Now you effectively walk the page table twice in the common 
> small-folio case ...

I think in every case, but see below

> 
> Can we just go back to the old handling that we had before here?
> 

I'd rather not, this is needed to prepare for the next series (for
6.15) in a couple of weeks, where gmap gets completely removed from
s390/mm, and gmap dat tables will not share ptes with userspace anymore
(i.e. we will use mmu_notifiers, like all other archs)

I will remove the double walk, though, since there is no reason to keep
it in there