* [PATCH] Fix mm->owner point to a task that does not exists
@ 2018-12-09 7:21 gchen.guomin
2018-12-10 1:36 ` Michael S. Tsirkin
0 siblings, 1 reply; 4+ messages in thread
From: gchen.guomin @ 2018-12-09 7:21 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: guominchen, linux-mm, linux-kernel, Jason Wang, netdev
From: guominchen <guominchen@tencent.com>
Under normal circumstances,When do_exit exits, mm->owner will
be updated, but when the kernel process calls unuse_mm and exits,
mm->owner cannot be updated. And will point to a task that has
been released.
Below is my issue on vhost_net:
A, B are two kernel processes(such as vhost_worker),
C is a user space process(such as qemu), and all
three use the mm of the user process C.
Now, because user process C exits abnormally, the owner of this
mm becomes A. When A calls unuse_mm and exits, this mm->ower
still points to the A that has been released.
When B accesses this mm->owner again, A has been released.
Process A Process B
vhost_worker() vhost_worker()
--------- ---------
use_mm() use_mm()
...
unuse_mm()
tsk->mm=NULL
do_exit() page fault
exit_mm() access mm->owner
can't update owner kernel Oops
unuse_mm()
Cc: <linux-mm@kvack.org>
Cc: <linux-kernel@vger.kernel.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: <netdev@vger.kernel.org>
Signed-off-by: guominchen <guominchen@tencent.com>
---
mm/mmu_context.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/mmu_context.c b/mm/mmu_context.c
index 3e612ae..185bb23 100644
--- a/mm/mmu_context.c
+++ b/mm/mmu_context.c
@@ -56,7 +56,6 @@ void unuse_mm(struct mm_struct *mm)
task_lock(tsk);
sync_mm_rss(mm);
- tsk->mm = NULL;
/* active_mm is still 'mm' */
enter_lazy_tlb(mm, tsk);
task_unlock(tsk);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Fix mm->owner point to a task that does not exists
2018-12-09 7:21 [PATCH] Fix mm->owner point to a task that does not exists gchen.guomin
@ 2018-12-10 1:36 ` Michael S. Tsirkin
2018-12-10 3:14 ` 答复: [PATCH] Fix mm->owner point to a task that does not exists(Internet mail) guominchen(陈国民)
0 siblings, 1 reply; 4+ messages in thread
From: Michael S. Tsirkin @ 2018-12-10 1:36 UTC (permalink / raw)
To: gchen.guomin
Cc: guominchen, linux-mm, linux-kernel, Jason Wang, netdev, paulmck
On Sun, Dec 09, 2018 at 03:21:17PM +0800, gchen.guomin@gmail.com wrote:
> From: guominchen <guominchen@tencent.com>
>
> Under normal circumstances,When do_exit exits, mm->owner will
> be updated, but when the kernel process calls unuse_mm and exits,
> mm->owner cannot be updated. And will point to a task that has
> been released.
>
> Below is my issue on vhost_net:
> A, B are two kernel processes(such as vhost_worker),
> C is a user space process(such as qemu), and all
> three use the mm of the user process C.
> Now, because user process C exits abnormally, the owner of this
> mm becomes A. When A calls unuse_mm and exits, this mm->ower
> still points to the A that has been released.
> When B accesses this mm->owner again, A has been released.
>
> Process A Process B
> vhost_worker() vhost_worker()
> --------- ---------
> use_mm() use_mm()
> ...
> unuse_mm()
> tsk->mm=NULL
> do_exit() page fault
> exit_mm() access mm->owner
> can't update owner kernel Oops
>
> unuse_mm()
>
> Cc: <linux-mm@kvack.org>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: <netdev@vger.kernel.org>
> Signed-off-by: guominchen <guominchen@tencent.com>
> ---
> mm/mmu_context.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/mmu_context.c b/mm/mmu_context.c
> index 3e612ae..185bb23 100644
> --- a/mm/mmu_context.c
> +++ b/mm/mmu_context.c
> @@ -56,7 +56,6 @@ void unuse_mm(struct mm_struct *mm)
>
> task_lock(tsk);
> sync_mm_rss(mm);
> - tsk->mm = NULL;
> /* active_mm is still 'mm' */
> enter_lazy_tlb(mm, tsk);
> task_unlock(tsk);
So that will work for vhost because we never drop
the mm reference before destroying the task.
I wonder whether that's true for other users though.
It would seem cleaner to onvoke some callback so
tasks such as vhost can drop the reference.
And looking at all this code, I don't understand why
is mm->owner safe to change like this:
mm->owner = NULL;
when users seem to use it under RCU.
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* 答复: [PATCH] Fix mm->owner point to a task that does not exists(Internet mail)
2018-12-10 1:36 ` Michael S. Tsirkin
@ 2018-12-10 3:14 ` guominchen(陈国民)
0 siblings, 0 replies; 4+ messages in thread
From: guominchen(陈国民) @ 2018-12-10 3:14 UTC (permalink / raw)
To: Michael S. Tsirkin, gchen.guomin
Cc: linux-mm, linux-kernel, Jason Wang, netdev, paulmck
>> From: guominchen <guominchen@tencent.com>
>>
>> Under normal circumstances,When do_exit exits, mm->owner will
>> be updated, but when the kernel process calls unuse_mm and exits,
>> mm->owner cannot be updated. And will point to a task that has
>> been released.
>>
>> Below is my issue on vhost_net:
>> A, B are two kernel processes(such as vhost_worker),
>> C is a user space process(such as qemu), and all
>> three use the mm of the user process C.
>> Now, because user process C exits abnormally, the owner of this
>> mm becomes A. When A calls unuse_mm and exits, this mm->ower
>> still points to the A that has been released.
>> When B accesses this mm->owner again, A has been released.
>>
>> Process A Process B
>> vhost_worker() vhost_worker()
>> --------- ---------
>> use_mm() use_mm()
>> ...
>> unuse_mm()
>> tsk->mm=NULL
>> do_exit() page fault
>> exit_mm() access mm->owner
>> can't update owner kernel Oops
>>
>> unuse_mm()
>>
>> Cc: <linux-mm@kvack.org>
>> Cc: <linux-kernel@vger.kernel.org>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: <netdev@vger.kernel.org>
>> Signed-off-by: guominchen <guominchen@tencent.com>
>> ---
>> mm/mmu_context.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/mm/mmu_context.c b/mm/mmu_context.c index
>> 3e612ae..185bb23 100644
>> --- a/mm/mmu_context.c
>> +++ b/mm/mmu_context.c
>> @@ -56,7 +56,6 @@ void unuse_mm(struct mm_struct *mm)
>>
>> task_lock(tsk);
>> sync_mm_rss(mm);
>> - tsk->mm = NULL;
>> /* active_mm is still 'mm' */
>> enter_lazy_tlb(mm, tsk);
>> task_unlock(tsk);
>So that will work for vhost because we never drop the mm reference before destroying the task.
>I wonder whether that's true for other users though.
>It would seem cleaner to onvoke some callback so tasks such as vhost can drop the reference.
Yes, I can remove this call in vhost, but I think use_mm(), and unuse_mm() are called in pairs in
order to share mm.
And exit_mm() as a unified mm handler, it doing very well, So we should leave mm to exit_mm()
to handle it.
>And looking at all this code, I don't understand why is mm->owner safe to change like this:
> mm->owner = NULL;
>when users seem to use it under RCU.
I think that mm->owner=NULL just changes the value of the pointer, and the task_struct it points to
is present and not released.
>> --
>> 1.8.3.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Fix mm->owner point to a task that does not exists.
@ 2018-12-12 1:54 gchen.guomin
0 siblings, 0 replies; 4+ messages in thread
From: gchen.guomin @ 2018-12-12 1:54 UTC (permalink / raw)
To: guominchen, Michael S. Tsirkin, Jason Wang
Cc: guomin chen, kvm, virtualization, netdev, linux-kernel
From: guomin chen <gchen.guomin@gmail.com>
Under normal circumstances,When do_exit exits, mm->owner will
be updated on exit_mm(). but when the kernel process calls
unuse_mm() and then exits,mm->owner cannot be updated. And it
will point to a task that has been released.
Below is my issue on vhost_net:
A, B are two kernel processes(such as vhost_worker),
C is a user space process(such as qemu), and all
three use the mm of the user process C.
Now, because user process C exits abnormally, the owner of this
mm becomes A. When A calls unuse_mm and exits, this mm->ower
still points to the A that has been released.
When B accesses this mm->owner again, A has been released.
Process A Process B
vhost_worker() vhost_worker()
--------- ---------
use_mm() use_mm()
...
unuse_mm()
tsk->mm=NULL
do_exit() page fault
exit_mm() access mm->owner
can't update owner kernel Oops
unuse_mm()
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: <kvm@vger.kernel.org>
Cc: <virtualization@lists.linux-foundation.org>
Cc: <netdev@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Signed-off-by: guomin chen <gchen.guomin@gmail.com>
---
drivers/vhost/vhost.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 6b98d8e..7f8586a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -368,6 +368,10 @@ static int vhost_worker(void *data)
}
}
unuse_mm(dev->mm);
+ /* current->mm needs to be processed by the later exit_mm() */
+ task_lock(current);
+ current->mm = dev->mm;
+ task_unlock(current);
set_fs(oldfs);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-12 1:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-09 7:21 [PATCH] Fix mm->owner point to a task that does not exists gchen.guomin
2018-12-10 1:36 ` Michael S. Tsirkin
2018-12-10 3:14 ` 答复: [PATCH] Fix mm->owner point to a task that does not exists(Internet mail) guominchen(陈国民)
2018-12-12 1:54 [PATCH] Fix mm->owner point to a task that does not exists gchen.guomin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).