netdev - 答复: [PATCH] Fix mm->owner point to a task that does not exists(Internet mail)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0D556C4F8D27C74EB25927291B4B0602A90358@EXMBX-SZ087.tencent.com>
Date:   Mon, 10 Dec 2018 03:14:10 +0000
From:   guominchen(陈国民) <guominchen@...cent.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>,
        "gchen.guomin@...il.com" <gchen.guomin@...il.com>
CC:     "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jason Wang <jasowang@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "paulmck@...ux.vnet.ibm.com" <paulmck@...ux.vnet.ibm.com>
Subject: 答复: [PATCH]  Fix mm->owner point to a task that does not exists(Internet mail)

>> From: guominchen <guominchen@...cent.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@...ck.org>
>> Cc: <linux-kernel@...r.kernel.org>
>> Cc: "Michael S. Tsirkin" <mst@...hat.com>
>> Cc: Jason Wang <jasowang@...hat.com>
>> Cc: <netdev@...r.kernel.org>
>> Signed-off-by: guominchen <guominchen@...cent.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