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

[Claudio Imbrenda]

On Fri, 14 Feb 2025 11:27:15 +0100
David Hildenbrand <david@xxxxxxxxxx> wrote:

> On 14.02.25 11:17, Claudio Imbrenda wrote:
> > On Thu, 13 Feb 2025 21:16:03 +0100
> > David Hildenbrand <david@xxxxxxxxxx> 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?)
> >   
> 
> Maybe Reported-by.
> 
> > [...]
> >   
> >>> @@ -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 think for the conversion we would still:
> 
> GFN -> HVA
> 
> Walk to the folio mapped at HVA, lock the PTE and perform the conversion.
> 
> So even with memory notifiers, that should be fine, no?

yes

> 
> So not necessarily "the old handling that we had before" but rather "the 
> old way of looking up what's mapped and performing the conversion under 
> the PTL".

ahhh, yes

> 
> For me to fix the refcount freezing properly on top of your work, we'll 
> need the PTL (esp. to exclude concurrent GUP-slow) etc.

let's discuss this off-list