From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A4FACC07E95 for ; Wed, 7 Jul 2021 09:32:49 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8AE6461C6D for ; Wed, 7 Jul 2021 09:32:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231225AbhGGJf2 (ORCPT ); Wed, 7 Jul 2021 05:35:28 -0400 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:35253 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230429AbhGGJf0 (ORCPT ); Wed, 7 Jul 2021 05:35:26 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1625650366; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=6o/7kaTqPTYt2A7Z0zjqMN0nvm6hpJh6BNtPQwdwRLU=; b=eDKqwYSv5XLoCZ3Jc4IVpTeElT2YSRmraFa//83/s2yWBvBqxqKd0iT4UH8gP5c8u4C3bv UVOR+Rls3e6hK1eTjfq/C0qHX2Sj1lCRm53WUubLGaVh6fxKBrWAJ3M0QBmYqhwgSQkPSh FmKG2Ol6mBesZZ5T/sjM1ols3lRSpsI= Received: from mail-pg1-f200.google.com (mail-pg1-f200.google.com [209.85.215.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-407-OHPFkpJwNdOcePrOXjVucw-1; Wed, 07 Jul 2021 05:32:45 -0400 X-MC-Unique: OHPFkpJwNdOcePrOXjVucw-1 Received: by mail-pg1-f200.google.com with SMTP id m13-20020a633f0d0000b0290222ece48979so1316356pga.1 for ; Wed, 07 Jul 2021 02:32:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding :content-language; bh=6o/7kaTqPTYt2A7Z0zjqMN0nvm6hpJh6BNtPQwdwRLU=; b=AeEzDuFO8LI2wvohn+ArUBqKKtLEVjsl/QhegLy0JG48LOk1EZiShkt4l3ZLrfhyCq 2AT4WooJSk++u4JujJRG2kL9CmAq/haELIxgjZ2A/QDDWf5knOuj8JUGMI5SN6Vanh27 hykfyju8F8kDfQeJqnR9Jt9pFk0X1ZsiSssH5lz1mQoJbqcOBYuZ7MJP13hn7wD69fIv AmcAq9YihqOvU4HkoIZ7VB5Dx0llrbeLaXIN1vr7Qcq8OrulbiukUGPG6Ab42WG8n12p V8VjAFfQTFZDdwbm6k/lVZWVSzohCocqlaxaHm0gAHdHBTSRrNLmQyX1hMsNSPO0KURV t34A== X-Gm-Message-State: AOAM533Ews7QQY4J7cpHT8dq1SFudoT+ngkkeFqjaZNxwnsE9YDSGDoe ZiNYBGm48DWFjK8QNt2a9H0aJ5ZUReehiBmZ2uEf5D+evxJPuAEW5aUOeMv+bqaD6x9QCByP5lC g9UYomwBTdFZc9XlbChANI6SdmmMMes+W4CMB3qb8CWumIppCEspUvubQWRLL0eZe6X46zygufJ ng X-Received: by 2002:a63:d908:: with SMTP id r8mr25178244pgg.414.1625649863953; Wed, 07 Jul 2021 02:24:23 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzhom9V3zcwokImKdubLhhEYAw1OEUfXMki4J5NNqR0Ed07VLuH+WgHu4YOpQSJUGm/lh1JIQ== X-Received: by 2002:a63:d908:: with SMTP id r8mr25178193pgg.414.1625649863513; Wed, 07 Jul 2021 02:24:23 -0700 (PDT) Received: from wangxiaodeMacBook-Air.local ([209.132.188.80]) by smtp.gmail.com with ESMTPSA id md15sm5946056pjb.30.2021.07.07.02.24.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 07 Jul 2021 02:24:23 -0700 (PDT) Subject: Re: [PATCH v8 10/10] Documentation: Add documentation for VDUSE To: Stefan Hajnoczi References: <8320d26d-6637-85c6-8773-49553dfa502d@redhat.com> <5b5107fa-3b32-8a3b-720d-eee6b2a84ace@redhat.com> <100e6788-7fdf-1505-d69c-bc28a8bc7a78@redhat.com> Cc: Xie Yongji , "Michael S. Tsirkin" , Stefano Garzarella , Parav Pandit , Christoph Hellwig , Christian Brauner , Randy Dunlap , Matthew Wilcox , Al Viro , Jens Axboe , "bcrl@kvack.org" , Jonathan Corbet , =?UTF-8?Q?Mika_Penttil=c3=a4?= , Dan Carpenter , joro@8bytes.org, gregkh@linuxfoundation.org, "songmuchun@bytedance.com" , virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, kvm@vger.kernel.org, linux-fsdevel@vger.kernel.org, "iommu@lists.linux-foundation.org" , linux-kernel From: Jason Wang Message-ID: Date: Wed, 7 Jul 2021 17:24:08 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 在 2021/7/7 下午4:55, Stefan Hajnoczi 写道: > On Wed, Jul 07, 2021 at 11:43:28AM +0800, Jason Wang wrote: >> 在 2021/7/7 上午1:11, Stefan Hajnoczi 写道: >>> On Tue, Jul 06, 2021 at 09:08:26PM +0800, Jason Wang wrote: >>>> On Tue, Jul 6, 2021 at 6:15 PM Stefan Hajnoczi wrote: >>>>> On Tue, Jul 06, 2021 at 10:34:33AM +0800, Jason Wang wrote: >>>>>> 在 2021/7/5 下午8:49, Stefan Hajnoczi 写道: >>>>>>> On Mon, Jul 05, 2021 at 11:36:15AM +0800, Jason Wang wrote: >>>>>>>> 在 2021/7/4 下午5:49, Yongji Xie 写道: >>>>>>>>>>> OK, I get you now. Since the VIRTIO specification says "Device >>>>>>>>>>> configuration space is generally used for rarely-changing or >>>>>>>>>>> initialization-time parameters". I assume the VDUSE_DEV_SET_CONFIG >>>>>>>>>>> ioctl should not be called frequently. >>>>>>>>>> The spec uses MUST and other terms to define the precise requirements. >>>>>>>>>> Here the language (especially the word "generally") is weaker and means >>>>>>>>>> there may be exceptions. >>>>>>>>>> >>>>>>>>>> Another type of access that doesn't work with the VDUSE_DEV_SET_CONFIG >>>>>>>>>> approach is reads that have side-effects. For example, imagine a field >>>>>>>>>> containing an error code if the device encounters a problem unrelated to >>>>>>>>>> a specific virtqueue request. Reading from this field resets the error >>>>>>>>>> code to 0, saving the driver an extra configuration space write access >>>>>>>>>> and possibly race conditions. It isn't possible to implement those >>>>>>>>>> semantics suing VDUSE_DEV_SET_CONFIG. It's another corner case, but it >>>>>>>>>> makes me think that the interface does not allow full VIRTIO semantics. >>>>>>>> Note that though you're correct, my understanding is that config space is >>>>>>>> not suitable for this kind of error propagating. And it would be very hard >>>>>>>> to implement such kind of semantic in some transports. Virtqueue should be >>>>>>>> much better. As Yong Ji quoted, the config space is used for >>>>>>>> "rarely-changing or intialization-time parameters". >>>>>>>> >>>>>>>> >>>>>>>>> Agreed. I will use VDUSE_DEV_GET_CONFIG in the next version. And to >>>>>>>>> handle the message failure, I'm going to add a return value to >>>>>>>>> virtio_config_ops.get() and virtio_cread_* API so that the error can >>>>>>>>> be propagated to the virtio device driver. Then the virtio-blk device >>>>>>>>> driver can be modified to handle that. >>>>>>>>> >>>>>>>>> Jason and Stefan, what do you think of this way? >>>>>>> Why does VDUSE_DEV_GET_CONFIG need to support an error return value? >>>>>>> >>>>>>> The VIRTIO spec provides no way for the device to report errors from >>>>>>> config space accesses. >>>>>>> >>>>>>> The QEMU virtio-pci implementation returns -1 from invalid >>>>>>> virtio_config_read*() and silently discards virtio_config_write*() >>>>>>> accesses. >>>>>>> >>>>>>> VDUSE can take the same approach with >>>>>>> VDUSE_DEV_GET_CONFIG/VDUSE_DEV_SET_CONFIG. >>>>>>> >>>>>>>> I'd like to stick to the current assumption thich get_config won't fail. >>>>>>>> That is to say, >>>>>>>> >>>>>>>> 1) maintain a config in the kernel, make sure the config space read can >>>>>>>> always succeed >>>>>>>> 2) introduce an ioctl for the vduse usersapce to update the config space. >>>>>>>> 3) we can synchronize with the vduse userspace during set_config >>>>>>>> >>>>>>>> Does this work? >>>>>>> I noticed that caching is also allowed by the vhost-user protocol >>>>>>> messages (QEMU's docs/interop/vhost-user.rst), but the device doesn't >>>>>>> know whether or not caching is in effect. The interface you outlined >>>>>>> above requires caching. >>>>>>> >>>>>>> Is there a reason why the host kernel vDPA code needs to cache the >>>>>>> configuration space? >>>>>> Because: >>>>>> >>>>>> 1) Kernel can not wait forever in get_config(), this is the major difference >>>>>> with vhost-user. >>>>> virtio_cread() can sleep: >>>>> >>>>> #define virtio_cread(vdev, structname, member, ptr) \ >>>>> do { \ >>>>> typeof(((structname*)0)->member) virtio_cread_v; \ >>>>> \ >>>>> might_sleep(); \ >>>>> ^^^^^^^^^^^^^^ >>>>> >>>>> Which code path cannot sleep? >>>> Well, it can sleep but it can't sleep forever. For VDUSE, a >>>> buggy/malicious userspace may refuse to respond to the get_config. >>>> >>>> It looks to me the ideal case, with the current virtio spec, for VDUSE is to >>>> >>>> 1) maintain the device and its state in the kernel, userspace may sync >>>> with the kernel device via ioctls >>>> 2) offload the datapath (virtqueue) to the userspace >>>> >>>> This seems more robust and safe than simply relaying everything to >>>> userspace and waiting for its response. >>>> >>>> And we know for sure this model can work, an example is TUN/TAP: >>>> netdevice is abstracted in the kernel and datapath is done via >>>> sendmsg()/recvmsg(). >>>> >>>> Maintaining the config in the kernel follows this model and it can >>>> simplify the device generation implementation. >>>> >>>> For config space write, it requires more thought but fortunately it's >>>> not commonly used. So VDUSE can choose to filter out the >>>> device/features that depends on the config write. >>> This is the problem. There are other messages like SET_FEATURES where I >>> guess we'll face the same challenge. >> >> Probably not, userspace device can tell the kernel about the device_features >> and mandated_features during creation, and the feature negotiation could be >> done purely in the kernel without bothering the userspace. (For some reason I drop the list accidentally, adding them back, sorry) > Sorry, I confused the messages. I meant SET_STATUS. It's a synchronous > interface where the driver waits for the device. It depends on how we define "synchronous" here. If I understand correctly, the spec doesn't expect there will be any kind of failure for the operation of set_status itself. Instead, anytime it want any synchronization, it should be done via get_status(): 1) re-read device status to make sure FEATURES_OK is set during feature negotiation 2) re-read device status to be 0 to make sure the device has finish the reset > > VDUSE currently doesn't wait for the device emulation process to handle > this message (no reply is needed) but I think this is a mistake because > VDUSE is not following the VIRTIO device model. With the trick that is done for FEATURES_OK above, I think we don't need to wait for the reply. If userspace takes too long to respond, it can be detected since get_status() doesn't return the expected value for long time. And for the case that needs a timeout, we probably can use NEEDS_RESET. > > I strongly suggest designing the VDUSE interface to match the VIRTIO > device model (or at least the vDPA interface). I fully agree with you and that is what we want to achieve in this series. > Defining a custom > interface for VDUSE avoids some implementation complexity and makes it > easier to deal with untrusted userspace, but it's impossible to > implement certain VIRTIO features or devices. It also fragments VIRTIO > more than necessary; we have a standard, let's stick to it. Yes. > >>> I agree that caching the contents of configuration space in the kernel >>> helps, but if there are other VDUSE messages with the same problem then >>> an attacker will exploit them instead. >>> >>> I think a systematic solution is needed. It would be necessary to >>> enumerate the virtio_vdpa and vhost_vdpa cases separately to figure out >>> where VDUSE messages are synchronous/time-sensitive. >> >> This is the case of reset and needs more thought. We should stick a >> consistent uAPI for the userspace. >> >> For vhost-vDPA, it needs synchronzied with the userspace and we can wait for >> ever. > The VMM should still be able to handle signals when a vhost_vdpa ioctl > is waiting for a reply from the VDUSE userspace process. Or if that's > not possible then there needs to be a way to force disconnection from > VDUSE so the VMM can be killed. Note that VDUSE works under vDPA bus, so vhost should be transport to VDUSE. But we can detect this via whether or not the bounce buffer is used. Thanks > > Stefan