Date:   Thu, 15 Apr 2021 15:38:26 +0100
From:   Stefan Hajnoczi <stefanha@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Yongji Xie <xieyongji@...edance.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Parav Pandit <parav@...dia.com>,
        Christoph Hellwig <hch@...radead.org>,
        Christian Brauner <christian.brauner@...onical.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Matthew Wilcox <willy@...radead.org>, viro@...iv.linux.org.uk,
        Jens Axboe <axboe@...nel.dk>, bcrl@...ck.org,
        Jonathan Corbet <corbet@....net>,
        Mika Penttilä <mika.penttila@...tfour.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v6 10/10] Documentation: Add documentation for VDUSE

On Thu, Apr 15, 2021 at 04:36:35PM +0800, Jason Wang wrote:
> 
> 在 2021/4/15 下午3:19, Stefan Hajnoczi 写道:
> > On Thu, Apr 15, 2021 at 01:38:37PM +0800, Yongji Xie wrote:
> > > On Wed, Apr 14, 2021 at 10:15 PM Stefan Hajnoczi <stefanha@...hat.com> wrote:
> > > > On Wed, Mar 31, 2021 at 04:05:19PM +0800, Xie Yongji wrote:
> > > > > VDUSE (vDPA Device in Userspace) is a framework to support
> > > > > implementing software-emulated vDPA devices in userspace. This
> > > > > document is intended to clarify the VDUSE design and usage.
> > > > > 
> > > > > Signed-off-by: Xie Yongji <xieyongji@...edance.com>
> > > > > ---
> > > > >   Documentation/userspace-api/index.rst |   1 +
> > > > >   Documentation/userspace-api/vduse.rst | 212 ++++++++++++++++++++++++++++++++++
> > > > >   2 files changed, 213 insertions(+)
> > > > >   create mode 100644 Documentation/userspace-api/vduse.rst
> > > > Just looking over the documentation briefly (I haven't studied the code
> > > > yet)...
> > > > 
> > > Thank you!
> > > 
> > > > > +How VDUSE works
> > > > > +------------
> > > > > +Each userspace vDPA device is created by the VDUSE_CREATE_DEV ioctl on
> > > > > +the character device (/dev/vduse/control). Then a device file with the
> > > > > +specified name (/dev/vduse/$NAME) will appear, which can be used to
> > > > > +implement the userspace vDPA device's control path and data path.
> > > > These steps are taken after sending the VDPA_CMD_DEV_NEW netlink
> > > > message? (Please consider reordering the documentation to make it clear
> > > > what the sequence of steps are.)
> > > > 
> > > No, VDUSE devices should be created before sending the
> > > VDPA_CMD_DEV_NEW netlink messages which might produce I/Os to VDUSE.
> > I see. Please include an overview of the steps before going into detail.
> > Something like:
> > 
> >    VDUSE devices are started as follows:
> > 
> >    1. Create a new VDUSE instance with ioctl(VDUSE_CREATE_DEV) on
> >       /dev/vduse/control.
> > 
> >    2. Begin processing VDUSE messages from /dev/vduse/$NAME. The first
> >       messages will arrive while attaching the VDUSE instance to vDPA.
> > 
> >    3. Send the VDPA_CMD_DEV_NEW netlink message to attach the VDUSE
> >       instance to vDPA.
> > 
> >    VDUSE devices are stopped as follows:
> > 
> >    ...
> > 
> > > > > +     static int netlink_add_vduse(const char *name, int device_id)
> > > > > +     {
> > > > > +             struct nl_sock *nlsock;
> > > > > +             struct nl_msg *msg;
> > > > > +             int famid;
> > > > > +
> > > > > +             nlsock = nl_socket_alloc();
> > > > > +             if (!nlsock)
> > > > > +                     return -ENOMEM;
> > > > > +
> > > > > +             if (genl_connect(nlsock))
> > > > > +                     goto free_sock;
> > > > > +
> > > > > +             famid = genl_ctrl_resolve(nlsock, VDPA_GENL_NAME);
> > > > > +             if (famid < 0)
> > > > > +                     goto close_sock;
> > > > > +
> > > > > +             msg = nlmsg_alloc();
> > > > > +             if (!msg)
> > > > > +                     goto close_sock;
> > > > > +
> > > > > +             if (!genlmsg_put(msg, NL_AUTO_PORT, NL_AUTO_SEQ, famid, 0, 0,
> > > > > +                 VDPA_CMD_DEV_NEW, 0))
> > > > > +                     goto nla_put_failure;
> > > > > +
> > > > > +             NLA_PUT_STRING(msg, VDPA_ATTR_DEV_NAME, name);
> > > > > +             NLA_PUT_STRING(msg, VDPA_ATTR_MGMTDEV_DEV_NAME, "vduse");
> > > > > +             NLA_PUT_U32(msg, VDPA_ATTR_DEV_ID, device_id);
> > > > What are the permission/capability requirements for VDUSE?
> > > > 
> > > Now I think we need privileged permission (root user). Because
> > > userspace daemon is able to access avail vring, used vring, descriptor
> > > table in kernel driver directly.
> > Please state this explicitly at the start of the document. Existing
> > interfaces like FUSE are designed to avoid trusting userspace.
> 
> 
> There're some subtle difference here. VDUSE present a device to kernel which
> means IOMMU is probably the only thing to prevent a malicous device.
> 
> 
> > Therefore
> > people might think the same is the case here. It's critical that people
> > are aware of this before deploying VDUSE with virtio-vdpa.
> > 
> > We should probably pause here and think about whether it's possible to
> > avoid trusting userspace. Even if it takes some effort and costs some
> > performance it would probably be worthwhile.
> 
> 
> Since the bounce buffer is used the only attack surface is the coherent
> area, if we want to enforce stronger isolation we need to use shadow
> virtqueue (which is proposed in earlier version by me) in this case. But I'm
> not sure it's worth to do that.

The security situation needs to be clear before merging this feature.

I think the IOMMU and vring can be made secure. What is more concerning
is the kernel code that runs on top: VIRTIO device drivers, network
stack, file systems, etc. They trust devices to an extent.

Since virtio-vdpa is a big reason for doing VDUSE in the first place I
don't think it makes sense to disable virtio-vdpa with VDUSE. A solution
is needed.

I'm going to be offline for a week and don't want to be a bottleneck.
I'll catch up when I'm back.

Stefan

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists