Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: Prevent race in uvm_fault_lower_io()
To:
Martin Pieuchot <mpi@grenadille.net>
Cc:
tech@openbsd.org
Date:
Sun, 22 Dec 2024 18:27:13 +0100

Download raw body.

Thread
> Date: Sun, 22 Dec 2024 18:10:44 +0100
> From: Martin Pieuchot <mpi@grenadille.net>
> 
> Read entry's value before releasing the locks.  Otherwise pgo_get()
> could be called with garbage values as found by claudio@ the hardway.
> 
> ok?

Are you adding that KASSERT deliberately?  If so, ok kettenis@

> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> diff -u -p -r1.155 uvm_fault.c
> --- uvm/uvm_fault.c	20 Dec 2024 18:46:51 -0000	1.155
> +++ uvm/uvm_fault.c	22 Dec 2024 16:57:32 -0000
> @@ -1503,22 +1503,28 @@ uvm_fault_lower_io(
>  	struct uvm_object *uobj = *ruobj;
>  	struct vm_page *pg;
>  	boolean_t locked;
> -	int gotpages;
> +	int gotpages, advice;
>  	int result;
>  	voff_t uoff;
> +	vm_prot_t access_type;
> +
> +	/* grab everything we need from the entry before we unlock */
> +	uoff = (ufi->orig_rvaddr - ufi->entry->start) + ufi->entry->offset;
> +	access_type = flt->access_type & MASK(ufi->entry);
> +	advice = ufi->entry->advice;
> +
> +	uvmfault_unlockall(ufi, amap, NULL);
>  
>  	/* update rusage counters */
>  	curproc->p_ru.ru_majflt++;
>  
> -	uvmfault_unlockall(ufi, amap, NULL);
> +	KASSERT(rw_write_held(uobj->vmobjlock));
>  
>  	counters_inc(uvmexp_counters, flt_get);
>  	gotpages = 1;
>  	pg = NULL;
> -	uoff = (ufi->orig_rvaddr - ufi->entry->start) + ufi->entry->offset;
>  	result = uobj->pgops->pgo_get(uobj, uoff, &pg, &gotpages,
> -	    0, flt->access_type & MASK(ufi->entry), ufi->entry->advice,
> -	    PGO_SYNCIO);
> +	    0, access_type, advice, PGO_SYNCIO);
>  
>  	/*
>  	 * recover from I/O
> 
> 
>