netdev - 答复: [PATCH] Export mm_update_next_owner function for vhost-net(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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0D556C4F8D27C74EB25927291B4B0602A90693@EXMBX-SZ087.tencent.com>
Date:   Thu, 13 Dec 2018 09:13:34 +0000
From:   guominchen(陈国民) <guominchen@...cent.com>
To:     Jason Wang <jasowang@...hat.com>,
        "gchen.guomin@...il.com" <gchen.guomin@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Dominik Brodowski <linux@...inikbrodowski.net>
Subject: 答复: [PATCH] Export mm_update_next_owner function for vhost-net(Internet mail)

>>   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.


Thank your for taking a look and apologize for my distrub.

>Could you describe how you reproduce this issue?
Sorry, this issue is hard for my to reproduce, But there is such a critical situation.

>I believe vhost process should exit before process C?
Yes, the A, B will exit before C, because usually C will close the open fd and then exit.
However, if C is abnormally exited, such as killed by some fatal signal, A may exit before C

The current issue flow is as follows:
Process C              Process A         Process B
qemu-system-x86_64:     kernel:vhost_net  kernel: vhost_net
open /dev/vhost-net
  VHOST_SET_OWNER   create kthread vhost-%d  create kthread vhost-%d
  network init           use_mm()          use_mm()
   ...                   ...
   Abnormal exited
   ...
  do_exit
  exit_mm()
  update mm->owner to A
  exit_files()
   close_files()
   kthread_should_stop() unuse_mm()
    Stop Process A       tsk->mm=NULL
                         do_exit()
                         can't update owner
                         A exit completed   vhost-%d  rcv first package
                                            vhost-%d build rcv buffer for vq
                                            page fault
                                            access mm & mm->owner
                                            NOW,mm->owner still pointer A
                                            kernel NULL pointer at mem_cgroup_from_task()
    stop Process B

>>
>> Cc: "Michael S. Tsirkin" <mst@...hat.com>
>> Cc: Jason Wang <jasowang@...hat.com>
>> Cc: kvm@...r.kernel.org
>> Cc: virtualization@...ts.linux-foundation.org
>> Cc: netdev@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
>> Cc: Andrew Morton <akpm@...ux-foundation.org>
>> Cc: Sudip Mukherjee <sudipm.mukherjee@...il.com>
>> Cc: "Luis R. Rodriguez" <mcgrof@...nel.org>
>> Cc: Dominik Brodowski <linux@...inikbrodowski.net>
>> Signed-off-by: guomin chen <gchen.guomin@...il.com>
>> ---
>>   drivers/vhost/vhost.c | 1 +
>>   kernel/exit.c         | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 6b98d8e..7c09087 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -368,6 +368,7 @@ static int vhost_worker(void *data)
>>   		}
>>   	}
>>   	unuse_mm(dev->mm);
>> +	mm_update_next_owner(dev->mm);


>If you analysis is correct, this is still racy isn't it? (E.g page fault 
>happen between unuse_mm() and mm_update_next_owner()).

No, I think this is not racy. 
When page fault happend Between unuse_mm() and mm_update_next_owner(), Although tsk->mm =NULL, 
But tsk has not exited, So mm->onwer = tsk can still be accessed.  

Thanks and regards