[openib-general] Re: [PATCH] process locked in D state.

[Michael S. Tsirkin]

> By your patch you completely removed locked_vm accounting on deregister
> path. I don't think this was your intention.

Of course. Closing the file should be handled separately from regular
deregistration. A new patch (below) fixes that.

---

Dont touch locked_vm when file is being closed, since a well-behaved
user is either exiting or has freed all mrs already, and a malicious
user is only hurting himself by making locked_vm seem higher than the
actual usage.

Note that current code doesnt seem to handle the later case anyway,
since there's a window after close() returns where locked_vm isnt updated yet.

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

Index: core/uverbs_mem.c
===================================================================
--- core/uverbs_mem.c	(revision 2726)
+++ core/uverbs_mem.c	(working copy)
@@ -37,13 +37,6 @@
 
 #include "uverbs.h"
 
-struct ib_umem_account_work {
-	struct work_struct work;
-	struct mm_struct  *mm;
-	unsigned long      diff;
-};
-
-
 static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
 {
 	struct ib_umem_chunk *chunk, *tmp;
@@ -167,15 +160,9 @@ out:
 	return ret;
 }
 
-static void ib_umem_account(void *work_ptr)
+void ib_umem_put(struct ib_device *dev, struct ib_umem *umem)
 {
-	struct ib_umem_account_work *work = work_ptr;
-
-	down_write(&work->mm->mmap_sem);
-	work->mm->locked_vm -= work->diff;
-	up_write(&work->mm->mmap_sem);
-	mmput(work->mm);
-	kfree(work);
+	__ib_umem_release(dev, umem, 1);
 }
 
 void ib_umem_release(struct ib_device *dev, struct ib_umem *umem)
@@ -185,35 +172,11 @@ void ib_umem_release(struct ib_device *d
 	__ib_umem_release(dev, umem, 1);
 
 	mm = get_task_mm(current);
-	if (mm) {
-		/*
-		 * We may be called with the mm's mmap_sem already
-		 * held.  This can happen when a userspace munmap() is
-		 * the call that drops the last reference to our file
-		 * and calls our release method.  If there are memory
-		 * regions to destroy, we'll end up here and not be
-		 * able to take the mmap_sem.
-		 *
-		 * To handle this, we try to grab the mmap_sem, and if
-		 * we can't get it immediately, we defer the
-		 * accounting to the system workqueue.
-		 */
-		if (down_write_trylock(&mm->mmap_sem)) {
-			mm->locked_vm -= PAGE_ALIGN(umem->length + umem->offset) >> PAGE_SHIFT;
-			up_write(&mm->mmap_sem);
-			mmput(mm);
-		} else {
-			struct ib_umem_account_work *work;
-
-			work = kmalloc(sizeof *work, GFP_KERNEL);
-			if (!work)
-				return;
-
-			INIT_WORK(&work->work, ib_umem_account, work);
-			work->mm   = mm;
-			work->diff = PAGE_ALIGN(umem->length + umem->offset) >> PAGE_SHIFT;
+	if (!mm)
+		return;
 
-			schedule_work(&work->work);
-		}
-	}
+	down_write(&mm->mmap_sem);
+	mm->locked_vm -= PAGE_ALIGN(umem->length + umem->offset) >> PAGE_SHIFT;
+	up_write(&mm->mmap_sem);
+	mmput(mm);
 }
Index: core/uverbs_main.c
===================================================================
--- core/uverbs_main.c	(revision 2726)
+++ core/uverbs_main.c	(working copy)
@@ -133,7 +133,7 @@ static int ib_dealloc_ucontext(struct ib
 		struct ib_umem_object *memobj;
 
 		memobj = container_of(uobj, struct ib_umem_object, uobject);
-		ib_umem_release(mr->device, &memobj->umem);
+		ib_umem_put(mr->device, &memobj->umem);
 
 		idr_remove(&ib_uverbs_mr_idr, uobj->id);
 		ib_dereg_mr(mr);
Index: core/uverbs.h
===================================================================
--- core/uverbs.h	(revision 2726)
+++ core/uverbs.h	(working copy)
@@ -104,6 +104,7 @@ void ib_uverbs_qp_event_handler(struct i
 int ib_umem_get(struct ib_device *dev, struct ib_umem *mem,
 		void *addr, size_t size, int write);
 void ib_umem_release(struct ib_device *dev, struct ib_umem *umem);
+void ib_umem_put(struct ib_device *dev, struct ib_umem *umem);
 
 #define IB_UVERBS_DECLARE_CMD(name)					\
 	ssize_t ib_uverbs_##name(struct ib_uverbs_file *file,		\
-- 
MST