From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: uvm_fault_unwire() & truncation
To: tech@openbsd.org
Date: Fri, 1 Nov 2024 13:23:39 +0100

On 21/10/24(Mon) 13:09, Martin Pieuchot wrote:
> Syzkaller reports [0] a race in uvmspace_free() where the page `pg' given
> to uvm_pageunwire() is not associated to the `entry'.
> 
> Diff below adapts NetBSD's 416427e0 that fixed a similar syzkaller report [2].
> 
> - in uvm_fault_check(), if the map entry is wired, handle the fault the same
>   way that we would handle UVM_FAULT_WIRE.  faulting on wired mappings is valid
>   if the mapped object was truncated and then later grown again.
> 
> - in uvm_fault_unwire_locked(), we must hold the locks for the vm_map_entry
>   while calling pmap_extract() in order to avoid races with the mapped object
>   being truncated while we are unwiring it.
> 
> Ok?

Anyone?  This is a small step to reduce differences with NetBSD and work
on reducing the contention on rwlocks.  I'd also like to see if this help
the existing Syzkaller reports.

> 
> [0] https://syzkaller.appspot.com/bug?extid=78e55b3fe74e144f033b
> [1] https://github.com/IIJ-NetBSD/netbsd-src/commit/416427e0
> [2] https://syzkaller.appspot.com/bug?id=8840dce484094a926e1ec388ffb83acb2fa291c9
> 
> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> diff -u -p -r1.135 uvm_fault.c
> --- uvm/uvm_fault.c	5 Sep 2023 05:08:26 -0000	1.135
> +++ uvm/uvm_fault.c	21 Oct 2024 11:02:33 -0000
> @@ -552,7 +552,7 @@ struct uvm_faultctx {
>  
>  int		uvm_fault_check(
>  		    struct uvm_faultinfo *, struct uvm_faultctx *,
> -		    struct vm_anon ***);
> +		    struct vm_anon ***, vm_fault_t);
>  
>  int		uvm_fault_upper(
>  		    struct uvm_faultinfo *, struct uvm_faultctx *,
> @@ -585,11 +585,6 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad
>  	ufi.orig_map = orig_map;
>  	ufi.orig_rvaddr = trunc_page(vaddr);
>  	ufi.orig_size = PAGE_SIZE;	/* can't get any smaller than this */
> -	if (fault_type == VM_FAULT_WIRE)
> -		flt.narrow = TRUE;	/* don't look for neighborhood
> -					 * pages on wire */
> -	else
> -		flt.narrow = FALSE;	/* normal fault */
>  	flt.access_type = access_type;
>  
>  
> @@ -597,7 +592,7 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad
>  	while (error == ERESTART) { /* ReFault: */
>  		anons = anons_store;
>  
> -		error = uvm_fault_check(&ufi, &flt, &anons);
> +		error = uvm_fault_check(&ufi, &flt, &anons, fault_type);
>  		if (error != 0)
>  			continue;
>  
> @@ -660,7 +655,7 @@ uvm_fault(vm_map_t orig_map, vaddr_t vad
>   */
>  int
>  uvm_fault_check(struct uvm_faultinfo *ufi, struct uvm_faultctx *flt,
> -    struct vm_anon ***ranons)
> +    struct vm_anon ***ranons, vm_fault_t fault_type)
>  {
>  	struct vm_amap *amap;
>  	struct uvm_object *uobj;
> @@ -694,12 +689,14 @@ uvm_fault_check(struct uvm_faultinfo *uf
>  	 * be more strict than ufi->entry->protection.  "wired" means either
>  	 * the entry is wired or we are fault-wiring the pg.
>  	 */
> -
>  	flt->enter_prot = ufi->entry->protection;
>  	flt->pa_flags = UVM_ET_ISWC(ufi->entry) ? PMAP_WC : 0;
> -	flt->wired = VM_MAPENT_ISWIRED(ufi->entry) || (flt->narrow == TRUE);
> -	if (flt->wired)
> +	if (VM_MAPENT_ISWIRED(ufi->entry) || (fault_type == VM_FAULT_WIRE)) {
> +		flt->wired = TRUE;
>  		flt->access_type = flt->enter_prot; /* full access for wired */
> +		/*  don't look for neighborhood * pages on "wire" fault */
> +		flt->narrow = TRUE;
> +	}
>  
>  	* handle "needs_copy" case. */
>  	if (UVM_ET_ISNEEDSCOPY(ufi->entry)) {
> @@ -1654,9 +1651,6 @@ uvm_fault_unwire_locked(vm_map_t map, va
>  		panic("uvm_fault_unwire_locked: address not in map");
>  
>  	for (va = start; va < end ; va += PAGE_SIZE) {
> -		if (pmap_extract(pmap, va, &pa) == FALSE)
> -			continue;
> -
>  		/*
>  		 * find the map entry for the current address.
>  		 */
> @@ -1681,6 +1675,9 @@ uvm_fault_unwire_locked(vm_map_t map, va
>  			uvm_map_lock_entry(entry);
>  			oentry = entry;
>  		}
> +
> +		if (!pmap_extract(pmap, va, &pa))
> +			continue;
>  
>  		/*
>  		 * if the entry is no longer wired, tell the pmap.
>