On 10/19/2015 11:15 AM, Dmitry Vyukov wrote: > On Mon, Oct 19, 2015 at 5:08 PM, Sasha Levin <sasha.le...@oracle.com> wrote: >> > On 10/19/2015 10:47 AM, Dmitry Vyukov wrote: >>>> >>> Right, the memory areas that are accessed both by the hypervisor and >>>> >>> the guest >>>>> >>> > should be treated as untrusted input, but the hypervisor is >>>>> >>> > supposed to validate >>>>> >>> > the input carefully before using it - so I'm not sure how data >>>>> >>> > races would >>>>> >>> > introduce anything new that we didn't catch during validation. >>> >> >>> >> One possibility would be: if result of a racy read is passed to guest, >>> >> that can leak arbitrary host data into guest. Does not sound good. >>> >> Also, without usage of proper atomic operations, it is basically >>> >> impossible to verify untrusted data, as it can be changing under your >>> >> feet. And storing data into a local variable does not prevent the data >>> >> from changing. >> > >> > What's missing here is that the guest doesn't directly read/write the >> > memory: >> > every time it accesses a memory that is shared with the host it will >> > trigger >> > an exit, which will stop the vcpu thread that made the access and kernel >> > side >> > kvm will pass the hypervisor the value the guest wrote (or the memory >> > address >> > it attempted to read). The value/address can't change under us in that >> > scenario. > But still: if result of a racy read is passed to guest, that can leak > arbitrary host data into guest.
I see what you're saying. I need to think about it a bit, maybe we do need locking for each of the virtio devices we emulate. On an unrelated note, a few of the reports are pointing to ioport__unregister(): ================== WARNING: ThreadSanitizer: data race (pid=109228) Write of size 8 at 0x7d1c0000df40 by main thread: #0 free tsan/rtl/tsan_interceptors.cc:570 (lkvm+0x000000443376) #1 ioport__unregister ioport.c:138:2 (lkvm+0x0000004a9ff9) #2 pci__exit pci.c:247:2 (lkvm+0x0000004ac857) #3 init_list__exit util/init.c:59:8 (lkvm+0x0000004bca6e) #4 kvm_cmd_run_exit builtin-run.c:645:2 (lkvm+0x0000004a68a7) #5 kvm_cmd_run builtin-run.c:661 (lkvm+0x0000004a68a7) #6 handle_command kvm-cmd.c:84:8 (lkvm+0x0000004bc40c) #7 handle_kvm_command main.c:11:9 (lkvm+0x0000004ac0b4) #8 main main.c:18 (lkvm+0x0000004ac0b4) Previous read of size 8 at 0x7d1c0000df40 by thread T55: #0 rb_int_search_single util/rbtree-interval.c:14:17 (lkvm+0x0000004bf968) #1 ioport_search ioport.c:41:9 (lkvm+0x0000004aa05f) #2 kvm__emulate_io ioport.c:186 (lkvm+0x0000004aa05f) #3 kvm_cpu__emulate_io x86/include/kvm/kvm-cpu-arch.h:41:9 (lkvm+0x0000004aa718) #4 kvm_cpu__start kvm-cpu.c:126 (lkvm+0x0000004aa718) #5 kvm_cpu_thread builtin-run.c:174:6 (lkvm+0x0000004a6e3e) Thread T55 'kvm-vcpu-2' (tid=109285, finished) created by main thread at: #0 pthread_create tsan/rtl/tsan_interceptors.cc:848 (lkvm+0x0000004478a3) #1 kvm_cmd_run_work builtin-run.c:633:7 (lkvm+0x0000004a683f) #2 kvm_cmd_run builtin-run.c:660 (lkvm+0x0000004a683f) #3 handle_command kvm-cmd.c:84:8 (lkvm+0x0000004bc40c) #4 handle_kvm_command main.c:11:9 (lkvm+0x0000004ac0b4) #5 main main.c:18 (lkvm+0x0000004ac0b4) SUMMARY: ThreadSanitizer: data race ioport.c:138:2 in ioport__unregister ================== I think this is because we don't perform locking using pthread, but rather pause the vm entirely - so the cpu threads it's pointing to aren't actually running when we unregister ioports. Is there a way to annotate that for tsan? Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html