> Date: Thu, 5 Nov 2020 09:20:49 -0300
> From: Martin Pieuchot <[email protected]>
>
> One of the functions call in uvm_fault() passes a non-initialized
> `oanon' argument. This bug is harmless as long as there is no locking
> associated to amap & anons. But more importantly an `amap' is passed
> to the function any given anon should share its lock, so this parameter
> is redundant.
>
> ok to kill it?
If you're confident that you don't need to lock the anon independently
of the amap then this can indeed go.
> Index: dev/pci/drm/drm_gem.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/drm/drm_gem.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 drm_gem.c
> --- dev/pci/drm/drm_gem.c 21 Oct 2020 09:08:14 -0000 1.13
> +++ dev/pci/drm/drm_gem.c 5 Nov 2020 12:13:32 -0000
> @@ -101,7 +101,7 @@ drm_fault(struct uvm_faultinfo *ufi, vad
> */
>
> if (UVM_ET_ISCOPYONWRITE(entry)) {
> - uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj, NULL);
> + uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj);
> return(VM_PAGER_ERROR);
> }
>
> @@ -115,7 +115,7 @@ drm_fault(struct uvm_faultinfo *ufi, vad
> mtx_enter(&dev->quiesce_mtx);
> if (dev->quiesce && dev->quiesce_count == 0) {
> mtx_leave(&dev->quiesce_mtx);
> - uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj, NULL);
> + uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj);
> mtx_enter(&dev->quiesce_mtx);
> while (dev->quiesce) {
> msleep_nsec(&dev->quiesce, &dev->quiesce_mtx,
> Index: dev/pci/drm/ttm/ttm_bo_vm.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/drm/ttm/ttm_bo_vm.c,v
> retrieving revision 1.23
> diff -u -p -r1.23 ttm_bo_vm.c
> --- dev/pci/drm/ttm/ttm_bo_vm.c 21 Oct 2020 09:08:14 -0000 1.23
> +++ dev/pci/drm/ttm/ttm_bo_vm.c 5 Nov 2020 12:12:49 -0000
> @@ -750,7 +750,7 @@ ttm_bo_vm_fault(struct uvm_faultinfo *uf
> break;
> }
>
> - uvmfault_unlockall(ufi, NULL, NULL, NULL);
> + uvmfault_unlockall(ufi, NULL, NULL);
> return ret;
> }
>
> @@ -769,7 +769,7 @@ ttm_bo_vm_fault(struct uvm_faultinfo *uf
>
> dma_resv_unlock(bo->base.resv);
>
> - uvmfault_unlockall(ufi, NULL, NULL, NULL);
> + uvmfault_unlockall(ufi, NULL, NULL);
> return ret;
> }
> EXPORT_SYMBOL(ttm_bo_vm_fault);
> Index: dev/pci/drm/i915/gem/i915_gem_mman.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/drm/i915/gem/i915_gem_mman.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 i915_gem_mman.c
> --- dev/pci/drm/i915/gem/i915_gem_mman.c 21 Oct 2020 02:16:53 -0000
> 1.2
> +++ dev/pci/drm/i915/gem/i915_gem_mman.c 5 Nov 2020 12:04:58 -0000
> @@ -473,7 +473,7 @@ vm_fault_cpu(struct i915_mmap_offset *mm
>
> /* Sanity check that we allow writing into this object */
> if (unlikely(i915_gem_object_is_readonly(obj) && write)) {
> - uvmfault_unlockall(ufi, NULL, &obj->base.uobj, NULL);
> + uvmfault_unlockall(ufi, NULL, &obj->base.uobj);
> return VM_PAGER_BAD;
> }
>
> @@ -518,7 +518,7 @@ vm_fault_cpu(struct i915_mmap_offset *mm
> i915_gem_object_unpin_pages(obj);
>
> out:
> - uvmfault_unlockall(ufi, NULL, &obj->base.uobj, NULL);
> + uvmfault_unlockall(ufi, NULL, &obj->base.uobj);
> return i915_error_to_vmf_fault(err);
> }
>
> @@ -559,7 +559,7 @@ vm_fault_gtt(struct i915_mmap_offset *mm
>
> /* Sanity check that we allow writing into this object */
> if (i915_gem_object_is_readonly(obj) && write) {
> - uvmfault_unlockall(ufi, NULL, &obj->base.uobj, NULL);
> + uvmfault_unlockall(ufi, NULL, &obj->base.uobj);
> return VM_PAGER_BAD;
> }
>
> @@ -664,7 +664,7 @@ err_rpm:
> intel_runtime_pm_put(rpm, wakeref);
> i915_gem_object_unpin_pages(obj);
> err:
> - uvmfault_unlockall(ufi, NULL, &obj->base.uobj, NULL);
> + uvmfault_unlockall(ufi, NULL, &obj->base.uobj);
> return i915_error_to_vmf_fault(ret);
> }
>
> @@ -687,7 +687,7 @@ i915_gem_fault(struct drm_gem_object *ge
> mmo = container_of(node, struct i915_mmap_offset, vma_node);
> drm_vma_offset_unlock_lookup(dev->vma_offset_manager);
> if (!mmo) {
> - uvmfault_unlockall(ufi, NULL, &gem_obj->uobj, NULL);
> + uvmfault_unlockall(ufi, NULL, &gem_obj->uobj);
> return VM_PAGER_BAD;
> }
>
> Index: uvm/uvm_device.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_device.c,v
> retrieving revision 1.59
> diff -u -p -r1.59 uvm_device.c
> --- uvm/uvm_device.c 24 Oct 2020 21:07:53 -0000 1.59
> +++ uvm/uvm_device.c 5 Nov 2020 12:14:24 -0000
> @@ -306,7 +306,7 @@ udv_fault(struct uvm_faultinfo *ufi, vad
> * so we kill any attempt to do so here.
> */
> if (UVM_ET_ISCOPYONWRITE(entry)) {
> - uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj, NULL);
> + uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj);
> return(VM_PAGER_ERROR);
> }
>
> @@ -354,7 +354,7 @@ udv_fault(struct uvm_faultinfo *ufi, vad
> * XXX case.
> */
> uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap,
> - uobj, NULL);
> + uobj);
>
> /* sync what we have so far */
> pmap_update(ufi->orig_map->pmap);
> @@ -363,7 +363,7 @@ udv_fault(struct uvm_faultinfo *ufi, vad
> }
> }
>
> - uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj, NULL);
> + uvmfault_unlockall(ufi, ufi->entry->aref.ar_amap, uobj);
> pmap_update(ufi->orig_map->pmap);
> return (retval);
> }
> Index: uvm/uvm_fault.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.c,v
> retrieving revision 1.103
> diff -u -p -r1.103 uvm_fault.c
> --- uvm/uvm_fault.c 21 Oct 2020 08:55:40 -0000 1.103
> +++ uvm/uvm_fault.c 5 Nov 2020 12:05:57 -0000
> @@ -301,7 +301,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> * the last unlock must be an atomic unlock+wait on
> * the owner of page
> */
> - uvmfault_unlockall(ufi, amap, NULL, NULL);
> + uvmfault_unlockall(ufi, amap, NULL);
> tsleep_nsec(pg, PVM, "anonget2", INFSLP);
> /* ready to relock and try again */
> } else {
> @@ -309,14 +309,14 @@ uvmfault_anonget(struct uvm_faultinfo *u
> pg = uvm_pagealloc(NULL, 0, anon, 0);
>
> if (pg == NULL) { /* out of RAM. */
> - uvmfault_unlockall(ufi, amap, NULL, anon);
> + uvmfault_unlockall(ufi, amap, NULL);
> uvmexp.fltnoram++;
> uvm_wait("flt_noram1");
> /* ready to relock and try again */
> } else {
> /* we set the PG_BUSY bit */
> we_own = TRUE;
> - uvmfault_unlockall(ufi, amap, NULL, anon);
> + uvmfault_unlockall(ufi, amap, NULL);
>
> /*
> * we are passing a PG_BUSY+PG_FAKE+PG_CLEAN
> @@ -368,8 +368,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> pmap_page_protect(pg, PROT_NONE);
> uvm_anfree(anon); /* frees page for us */
> if (locked)
> - uvmfault_unlockall(ufi, amap, NULL,
> - NULL);
> + uvmfault_unlockall(ufi, amap, NULL);
> uvmexp.fltpgrele++;
> return (VM_PAGER_REFAULT); /* refault! */
> }
> @@ -399,8 +398,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> uvm_unlock_pageq();
>
> if (locked)
> - uvmfault_unlockall(ufi, amap, NULL,
> - anon);
> + uvmfault_unlockall(ufi, amap, NULL);
> return (VM_PAGER_ERROR);
> }
>
> @@ -423,7 +421,7 @@ uvmfault_anonget(struct uvm_faultinfo *u
> amap_lookup(&ufi->entry->aref,
> ufi->orig_rvaddr - ufi->entry->start) != anon) {
>
> - uvmfault_unlockall(ufi, amap, NULL, anon);
> + uvmfault_unlockall(ufi, amap, NULL);
> return (VM_PAGER_REFAULT);
> }
>
> @@ -937,7 +935,7 @@ ReFault:
>
> /* check for out of RAM */
> if (anon == NULL || pg == NULL) {
> - uvmfault_unlockall(&ufi, amap, NULL, oanon);
> + uvmfault_unlockall(&ufi, amap, NULL);
> if (anon == NULL)
> uvmexp.fltnoanon++;
> else {
> @@ -997,7 +995,7 @@ ReFault:
> * We do, however, have to go through the ReFault path,
> * as the map may change while we're asleep.
> */
> - uvmfault_unlockall(&ufi, amap, NULL, oanon);
> + uvmfault_unlockall(&ufi, amap, NULL);
> if (uvm_swapisfull()) {
> /* XXX instrumentation */
> return (ENOMEM);
> @@ -1028,7 +1026,7 @@ ReFault:
> uvm_unlock_pageq();
>
> /* done case 1! finish up by unlocking everything and returning
> success */
> - uvmfault_unlockall(&ufi, amap, NULL, oanon);
> + uvmfault_unlockall(&ufi, amap, NULL);
> pmap_update(ufi.orig_map->pmap);
> return (0);
>
> @@ -1065,7 +1063,7 @@ Case2:
> /* update rusage counters */
> curproc->p_ru.ru_majflt++;
>
> - uvmfault_unlockall(&ufi, amap, NULL, NULL);
> + uvmfault_unlockall(&ufi, amap, NULL);
>
> uvmexp.fltget++;
> gotpages = 1;
> @@ -1100,7 +1098,7 @@ Case2:
> if (locked && amap && amap_lookup(&ufi.entry->aref,
> ufi.orig_rvaddr - ufi.entry->start)) {
> if (locked)
> - uvmfault_unlockall(&ufi, amap, NULL, NULL);
> + uvmfault_unlockall(&ufi, amap, NULL);
> locked = FALSE;
> }
>
> @@ -1191,7 +1189,7 @@ Case2:
> }
>
> /* unlock and fail ... */
> - uvmfault_unlockall(&ufi, amap, uobj, NULL);
> + uvmfault_unlockall(&ufi, amap, uobj);
> if (anon == NULL)
> uvmexp.fltnoanon++;
> else {
> @@ -1244,7 +1242,7 @@ Case2:
>
> if (amap_add(&ufi.entry->aref,
> ufi.orig_rvaddr - ufi.entry->start, anon, 0)) {
> - uvmfault_unlockall(&ufi, amap, NULL, oanon);
> + uvmfault_unlockall(&ufi, amap, NULL);
> uvm_anfree(anon);
> uvmexp.fltnoamap++;
>
> @@ -1277,7 +1275,7 @@ Case2:
>
> atomic_clearbits_int(&pg->pg_flags, PG_BUSY|PG_FAKE|PG_WANTED);
> UVM_PAGE_OWN(pg, NULL);
> - uvmfault_unlockall(&ufi, amap, uobj, NULL);
> + uvmfault_unlockall(&ufi, amap, uobj);
> if (uvm_swapisfull()) {
> /* XXX instrumentation */
> return (ENOMEM);
> @@ -1312,7 +1310,7 @@ Case2:
>
> atomic_clearbits_int(&pg->pg_flags, PG_BUSY|PG_FAKE|PG_WANTED);
> UVM_PAGE_OWN(pg, NULL);
> - uvmfault_unlockall(&ufi, amap, uobj, NULL);
> + uvmfault_unlockall(&ufi, amap, uobj);
> pmap_update(ufi.orig_map->pmap);
>
> return (0);
> @@ -1445,7 +1443,7 @@ uvmfault_unlockmaps(struct uvm_faultinfo
> */
> void
> uvmfault_unlockall(struct uvm_faultinfo *ufi, struct vm_amap *amap,
> - struct uvm_object *uobj, struct vm_anon *anon)
> + struct uvm_object *uobj)
> {
>
> uvmfault_unlockmaps(ufi, FALSE);
> Index: uvm/uvm_fault.h
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_fault.h,v
> retrieving revision 1.15
> diff -u -p -r1.15 uvm_fault.h
> --- uvm/uvm_fault.h 11 Jul 2014 16:35:40 -0000 1.15
> +++ uvm/uvm_fault.h 5 Nov 2020 12:04:11 -0000
> @@ -70,7 +70,7 @@ void uvmfault_init(void);
> boolean_t uvmfault_lookup(struct uvm_faultinfo *, boolean_t);
> boolean_t uvmfault_relock(struct uvm_faultinfo *);
> void uvmfault_unlockall(struct uvm_faultinfo *, struct vm_amap *,
> - struct uvm_object *, struct vm_anon *);
> + struct uvm_object *);
> int uvmfault_anonget(struct uvm_faultinfo *, struct vm_amap *,
> struct vm_anon *);
>
>
>
