KVM: async_pf: use_mm/mm_users fixes

async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm) should work just fine even if current has another or NULL ->mm.

Recently kvm_async_page_present_sync() was added insedie the "use_mm" section, but it seems that it doesn't need current->mm too.

Signed-off-by Oleg Nesterov <[email protected]>

---
 virt/kvm/async_pf.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

virt/kvm/async_pf.c +0 -2

index 10df100..0ced4f3 100644
@@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
 
 	might_sleep();
 
-	use_mm(mm);
 	down_read(&mm->mmap_sem);
 	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
 	up_read(&mm->mmap_sem);
 	kvm_async_page_present_sync(vcpu, apf);
-	unuse_mm(mm);
 
 	spin_lock(&vcpu->async_pf.lock);
 	list_add_tail(&apf->link, &vcpu->async_pf.done);

Quoted 3 lines · Oleg Nesterov

async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm) should work just fine even if current has another or NULL ->mm.

Quoted 4 lines

Recently kvm_async_page_present_sync() was added insedie the "use_mm" section, but it seems that it doesn't need current->mm too.

Signed-off-by Oleg Nesterov <[email protected]>

Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc. This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present might call apf_put_user which might call copy_to_user, so this is not ok, I guess.

Quoted 13 lines

---
 virt/kvm/async_pf.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

virt/kvm/async_pf.c +0 -0

index 10df100..0ced4f3 100644
@@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
 
 	might_sleep();

virt/kvm/async_pf.c static void async_pf_execute(struct work_struct *work) · line 89

Quoted 10 lines

virt/kvm/async_pf.c +0 -2

 
-	use_mm(mm);
 	down_read(&mm->mmap_sem);
 	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
 	up_read(&mm->mmap_sem);
 	kvm_async_page_present_sync(vcpu, apf);
-	unuse_mm(mm);
 
 	spin_lock(&vcpu->async_pf.lock);
 	list_add_tail(&apf->link, &vcpu->async_pf.done);

--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Christian Borntraeger

Quoted 7 lines · Oleg Nesterov

async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm) should work just fine even if current has another or NULL ->mm.

Recently kvm_async_page_present_sync() was added insedie the "use_mm" section, but it seems that it doesn't need current->mm too.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 3 lines

Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc. This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present might call apf_put_user which might call copy_to_user, so this is not ok, I guess.

wanted to say kvm_arch_async_page_not_present, but I have to correct myself. x86 does the "page is there" in the cpu loop, not in the worker. The cpu look d oes have a valid mm. So this patch should be also ok.

Quoted 10 lines

virt/kvm/async_pf.c static void async_pf_execute(struct work_struct *work) · line 88

Quoted 10 lines

virt/kvm/async_pf.c +0 -2

@@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
 
 	might_sleep();
 
-	use_mm(mm);
 	down_read(&mm->mmap_sem);
 	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
 	up_read(&mm->mmap_sem);
 	kvm_async_page_present_sync(vcpu, apf);
-	unuse_mm(mm);

Quoted 11 lines · Christian Borntraeger

Quoted 9 lines · Christian Borntraeger

Quoted 7 lines · Oleg Nesterov

async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm) should work just fine even if current has another or NULL ->mm.

Recently kvm_async_page_present_sync() was added insedie the "use_mm" section, but it seems that it doesn't need current->mm too.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 7 lines

Quoted 3 lines

Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc. This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present might call apf_put_user which might call copy_to_user, so this is not ok, I guess.

wanted to say kvm_arch_async_page_not_present, but I have to correct myself. x86 does the "page is there" in the cpu loop, not in the worker. The cpu look d oes have a valid mm. So this patch should be also ok.

Thanks ;)

Btw, I forgot to mention this in the changelog, but

Quoted 10 lines

Quoted 10 lines

virt/kvm/async_pf.c static void async_pf_execute(struct work_struct *work) · line 88

Quoted 10 lines

virt/kvm/async_pf.c +0 -2

@@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
 
 	might_sleep();
 
-	use_mm(mm);
 	down_read(&mm->mmap_sem);
 	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
 	up_read(&mm->mmap_sem);
 	kvm_async_page_present_sync(vcpu, apf);
-	unuse_mm(mm);

it can actually do

	get_user_pages(NULL, mm, addr, 1, 1, 0, NULL, NULL);

"task" is only used to increment task_struct->xxx_flt. I don't think async_pf_execute() actually needs this (current is PF_WQ_WORKER after all), but I didn't dare to do another change in the code I can hardly understand.

Quoted 5 lines

Quoted 5 lines

"task" is only used to increment task_struct->xxx_flt. I don't think async_pf_execute() actually needs this (current is PF_WQ_WORKER after all), but I didn't dare to do another change in the code I can hardly understand.

Considering the faults would be randomly distributed among the kworker threads my preference would also be for NULL instead of current.

ptrace and uprobes tends to be the only two places that look into other mm with gup, ptrace knows the exact pid that it is triggering the fault into, so it also can specify the correct task so the fault goes in the right task struct. uprobes uses NULL.

get_user_pages(mm) is simply wrong if mm->mm_users == 0 and exit_mmap/etc was already called (or is in progress), mm->mm_count can only pin mm->pgd and mm_struct itself.

Change kvm_setup_async_pf/async_pf_execute to inc/dec mm->mm_users.

kvm_create_vm/kvm_destroy_vm play with ->mm_count too but this case looks fine at first glance, it seems that this ->mm is only used to verify that current->mm == kvm->mm.

Signed-off-by Oleg Nesterov <[email protected]>

---
 virt/kvm/async_pf.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

virt/kvm/async_pf.c +4 -4

index 0ced4f3..cda703e 100644
@@ -99,7 +99,7 @@ static void async_pf_execute(struct work_struct *work)
 	if (waitqueue_active(&vcpu->wq))
 		wake_up_interruptible(&vcpu->wq);
 
-	mmdrop(mm);
+	mmput(mm);
 	kvm_put_kvm(vcpu->kvm);
 }
 

@@ -116,7 +116,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
 		flush_work(&work->work);
 #else
 		if (cancel_work_sync(&work->work)) {
-			mmdrop(work->mm);
+			mmput(work->mm);
 			kvm_put_kvm(vcpu->kvm); /* == work->vcpu->kvm */
 			kmem_cache_free(async_pf_cache, work);
 		}

@@ -181,7 +181,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, unsigned long hva,
 	work->addr = hva;
 	work->arch = *arch;
 	work->mm = current->mm;
-	atomic_inc(&work->mm->mm_count);
+	atomic_inc(&work->mm->mm_users);
 	kvm_get_kvm(work->vcpu->kvm);
 
 	/* this can't really happen otherwise gfn_to_pfn_async

@@ -199,7 +199,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, unsigned long hva,
 	return 1;
 retry_sync:
 	kvm_put_kvm(work->vcpu->kvm);
-	mmdrop(work->mm);
+	mmput(work->mm);
 	kmem_cache_free(async_pf_cache, work);
 	return 0;
 }

Hello.

Completely untested and I know nothing about kvm ;) Please review.

But use_mm() really looks misleading, and the usage of mm_users looks "obviously wrong". I already sent this change while we were discussing vmacache, but it was ignored. Since then kvm_async_page_present_sync() was added into async_pf_execute() into async_pf_execute(), but it seems to me that use_mm() is still unnecessary.

Oleg.

 virt/kvm/async_pf.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

Quoted 10 lines · Oleg Nesterov

Hello.

Completely untested and I know nothing about kvm ;) Please review.

But use_mm() really looks misleading, and the usage of mm_users looks "obviously wrong". I already sent this change while we were discussing vmacache, but it was ignored. Since then kvm_async_page_present_sync() was added into async_pf_execute() into async_pf_execute(), but it seems to me that use_mm() is still unnecessary.

Quoted 5 lines

Oleg.

 virt/kvm/async_pf.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

I gave both patches some testing on s390, seems fine. I think patch2 really does fix a bug. So if Paolo, Marcelo, Gleb agree (maybe do a test on x86 for async_pf) both patches are good to go. Given that somebody tests this on x86:

Acked-by Christian Borntraeger <[email protected]>

Hello.

Completely untested and I know nothing about kvm ;) Please review.

But use_mm() really looks misleading, and the usage of mm_users looks "obviously wrong". I already sent this change while we were discussing vmacache, but it was ignored. Since then kvm_async_page_present_sync() was added into async_pf_execute() into async_pf_execute(), but it seems to me that use_mm() is still unnecessary.

Oleg.

 virt/kvm/async_pf.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

Patch 1 will be for 3.16 only, I'd like a review from Marcelo or Andrea though (that's "KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()" for easier googling).

Quoted 3 lines · Paolo Bonzini

Patch 1 will be for 3.16 only, I'd like a review from Marcelo or Andrea though (that's "KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()" for easier googling).

Patch 1:

Reviewed-by Andrea Arcangeli <[email protected]>

I think current->NULL would be better too.

async_pf_execute() passes tsk == current to gup(), this is doesn't hurt but unnecessary and misleading. "tsk" is only used to account the number of faults and current is the random workqueue thread.

Signed-off-by Oleg Nesterov <[email protected]>

---
 virt/kvm/async_pf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

virt/kvm/async_pf.c +0 -0

index 0ced4f3..62f4223 100644