| Message ID | 20210713084656.232-1-xieyongji@bytedance.com |
|---|---|
| Headers | show |
| Series | Introduce VDUSE - vDPA Device in Userspace | expand |
On Tue, Jul 13, 2021 at 04:46:46PM +0800, Xie Yongji wrote: > We don't need to set FAILED status bit on device index allocation > failure since the device initialization hasn't been started yet. The commit message should say what the effect of this change is to the user. Is this a bugfix? Will it have any effect on runtime at all? To me, hearing your thoughts on this is valuable even if you have to guess. "I noticed this mistake during review and I don't think it will affect runtime." regards, dan carpenter
在 2021/7/13 下午7:31, Dan Carpenter 写道: > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: >> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) >> } >> } >> >> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, >> - struct vhost_iotlb_msg *msg) >> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, >> + u64 iova, u64 size, u64 uaddr, u32 perm) >> { >> struct vhost_dev *dev = &v->vdev; >> - struct vhost_iotlb *iotlb = dev->iotlb; >> struct page **page_list; >> unsigned long list_size = PAGE_SIZE / sizeof(struct page *); >> unsigned int gup_flags = FOLL_LONGTERM; >> unsigned long npages, cur_base, map_pfn, last_pfn = 0; >> unsigned long lock_limit, sz2pin, nchunks, i; >> - u64 iova = msg->iova; >> + u64 start = iova; >> long pinned; >> int ret = 0; >> >> - if (msg->iova < v->range.first || >> - msg->iova + msg->size - 1 > v->range.last) >> - return -EINVAL; > This is not related to your patch, but can the "msg->iova + msg->size" > addition can have an integer overflow. From looking at the callers it > seems like it can. msg comes from: > vhost_chr_write_iter() > --> dev->msg_handler(dev, &msg); > --> vhost_vdpa_process_iotlb_msg() > --> vhost_vdpa_process_iotlb_update() Yes. > > If I'm thinking of the right thing then these are allowed to overflow to > 0 because of the " - 1" but not further than that. I believe the check > needs to be something like: > > if (msg->iova < v->range.first || > msg->iova - 1 > U64_MAX - msg->size || I guess we don't need - 1 here? Thanks > msg->iova + msg->size - 1 > v->range.last) > > But writing integer overflow check correctly is notoriously difficult. > Do you think you could send a fix for that which is separate from the > patcheset? We'd want to backport it to stable. > > regards, > dan carpenter >
On Tue, Jul 13, 2021 at 7:31 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) > > } > > } > > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, > > - struct vhost_iotlb_msg *msg) > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, > > + u64 iova, u64 size, u64 uaddr, u32 perm) > > { > > struct vhost_dev *dev = &v->vdev; > > - struct vhost_iotlb *iotlb = dev->iotlb; > > struct page **page_list; > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *); > > unsigned int gup_flags = FOLL_LONGTERM; > > unsigned long npages, cur_base, map_pfn, last_pfn = 0; > > unsigned long lock_limit, sz2pin, nchunks, i; > > - u64 iova = msg->iova; > > + u64 start = iova; > > long pinned; > > int ret = 0; > > > > - if (msg->iova < v->range.first || > > - msg->iova + msg->size - 1 > v->range.last) > > - return -EINVAL; > > This is not related to your patch, but can the "msg->iova + msg->size" > addition can have an integer overflow. From looking at the callers it > seems like it can. msg comes from: > vhost_chr_write_iter() > --> dev->msg_handler(dev, &msg); > --> vhost_vdpa_process_iotlb_msg() > --> vhost_vdpa_process_iotlb_update() > > If I'm thinking of the right thing then these are allowed to overflow to > 0 because of the " - 1" but not further than that. I believe the check > needs to be something like: > > if (msg->iova < v->range.first || > msg->iova - 1 > U64_MAX - msg->size || > msg->iova + msg->size - 1 > v->range.last) > Make sense. > But writing integer overflow check correctly is notoriously difficult. > Do you think you could send a fix for that which is separate from the > patcheset? We'd want to backport it to stable. > OK, I will send a patch to fix it. Thanks, Yongji
On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: > > 在 2021/7/13 下午7:31, Dan Carpenter 写道: > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: > > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) > > > } > > > } > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, > > > - struct vhost_iotlb_msg *msg) > > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, > > > + u64 iova, u64 size, u64 uaddr, u32 perm) > > > { > > > struct vhost_dev *dev = &v->vdev; > > > - struct vhost_iotlb *iotlb = dev->iotlb; > > > struct page **page_list; > > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *); > > > unsigned int gup_flags = FOLL_LONGTERM; > > > unsigned long npages, cur_base, map_pfn, last_pfn = 0; > > > unsigned long lock_limit, sz2pin, nchunks, i; > > > - u64 iova = msg->iova; > > > + u64 start = iova; > > > long pinned; > > > int ret = 0; > > > - if (msg->iova < v->range.first || > > > - msg->iova + msg->size - 1 > v->range.last) > > > - return -EINVAL; > > This is not related to your patch, but can the "msg->iova + msg->size" > > addition can have an integer overflow. From looking at the callers it > > seems like it can. msg comes from: > > vhost_chr_write_iter() > > --> dev->msg_handler(dev, &msg); > > --> vhost_vdpa_process_iotlb_msg() > > --> vhost_vdpa_process_iotlb_update() > > > Yes. > > > > > > If I'm thinking of the right thing then these are allowed to overflow to > > 0 because of the " - 1" but not further than that. I believe the check > > needs to be something like: > > > > if (msg->iova < v->range.first || > > msg->iova - 1 > U64_MAX - msg->size || > > > I guess we don't need - 1 here? The - 1 is important. The highest address is 0xffffffff. So it goes start + size = 0 and then start + size - 1 == 0xffffffff. I guess we could move the - 1 to the other side? msg->iova > U64_MAX - msg->size + 1 || regards, dan carpenter
在 2021/7/14 下午4:05, Dan Carpenter 写道: > On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: >> 在 2021/7/13 下午7:31, Dan Carpenter 写道: >>> On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: >>>> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) >>>> } >>>> } >>>> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, >>>> - struct vhost_iotlb_msg *msg) >>>> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, >>>> + u64 iova, u64 size, u64 uaddr, u32 perm) >>>> { >>>> struct vhost_dev *dev = &v->vdev; >>>> - struct vhost_iotlb *iotlb = dev->iotlb; >>>> struct page **page_list; >>>> unsigned long list_size = PAGE_SIZE / sizeof(struct page *); >>>> unsigned int gup_flags = FOLL_LONGTERM; >>>> unsigned long npages, cur_base, map_pfn, last_pfn = 0; >>>> unsigned long lock_limit, sz2pin, nchunks, i; >>>> - u64 iova = msg->iova; >>>> + u64 start = iova; >>>> long pinned; >>>> int ret = 0; >>>> - if (msg->iova < v->range.first || >>>> - msg->iova + msg->size - 1 > v->range.last) >>>> - return -EINVAL; >>> This is not related to your patch, but can the "msg->iova + msg->size" >>> addition can have an integer overflow. From looking at the callers it >>> seems like it can. msg comes from: >>> vhost_chr_write_iter() >>> --> dev->msg_handler(dev, &msg); >>> --> vhost_vdpa_process_iotlb_msg() >>> --> vhost_vdpa_process_iotlb_update() >> >> Yes. >> >> >>> If I'm thinking of the right thing then these are allowed to overflow to >>> 0 because of the " - 1" but not further than that. I believe the check >>> needs to be something like: >>> >>> if (msg->iova < v->range.first || >>> msg->iova - 1 > U64_MAX - msg->size || >> >> I guess we don't need - 1 here? > The - 1 is important. The highest address is 0xffffffff. So it goes > start + size = 0 and then start + size - 1 == 0xffffffff. Right, so actually msg->iova = 0xfffffffe, msg->size=2 is valid. Thanks > > I guess we could move the - 1 to the other side? > > msg->iova > U64_MAX - msg->size + 1 || > > regards, > dan carpenter > >
On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote: > > 在 2021/7/14 下午4:05, Dan Carpenter 写道: > > On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: > > > 在 2021/7/13 下午7:31, Dan Carpenter 写道: > > > > On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: > > > > > @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) > > > > > } > > > > > } > > > > > -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, > > > > > - struct vhost_iotlb_msg *msg) > > > > > +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, > > > > > + u64 iova, u64 size, u64 uaddr, u32 perm) > > > > > { > > > > > struct vhost_dev *dev = &v->vdev; > > > > > - struct vhost_iotlb *iotlb = dev->iotlb; > > > > > struct page **page_list; > > > > > unsigned long list_size = PAGE_SIZE / sizeof(struct page *); > > > > > unsigned int gup_flags = FOLL_LONGTERM; > > > > > unsigned long npages, cur_base, map_pfn, last_pfn = 0; > > > > > unsigned long lock_limit, sz2pin, nchunks, i; > > > > > - u64 iova = msg->iova; > > > > > + u64 start = iova; > > > > > long pinned; > > > > > int ret = 0; > > > > > - if (msg->iova < v->range.first || > > > > > - msg->iova + msg->size - 1 > v->range.last) > > > > > - return -EINVAL; > > > > This is not related to your patch, but can the "msg->iova + msg->size" > > > > addition can have an integer overflow. From looking at the callers it > > > > seems like it can. msg comes from: > > > > vhost_chr_write_iter() > > > > --> dev->msg_handler(dev, &msg); > > > > --> vhost_vdpa_process_iotlb_msg() > > > > --> vhost_vdpa_process_iotlb_update() > > > > > > Yes. > > > > > > > > > > If I'm thinking of the right thing then these are allowed to overflow to > > > > 0 because of the " - 1" but not further than that. I believe the check > > > > needs to be something like: > > > > > > > > if (msg->iova < v->range.first || > > > > msg->iova - 1 > U64_MAX - msg->size || > > > > > > I guess we don't need - 1 here? > > The - 1 is important. The highest address is 0xffffffff. So it goes > > start + size = 0 and then start + size - 1 == 0xffffffff. > > > Right, so actually > > msg->iova = 0xfffffffe, msg->size=2 is valid. I believe so, yes. It's inclusive of 0xfffffffe and 0xffffffff. (Not an expert). regards, dan carpenter
在 2021/7/14 下午5:57, Dan Carpenter 写道: > On Wed, Jul 14, 2021 at 05:41:54PM +0800, Jason Wang wrote: >> 在 2021/7/14 下午4:05, Dan Carpenter 写道: >>> On Wed, Jul 14, 2021 at 10:14:32AM +0800, Jason Wang wrote: >>>> 在 2021/7/13 下午7:31, Dan Carpenter 写道: >>>>> On Tue, Jul 13, 2021 at 04:46:52PM +0800, Xie Yongji wrote: >>>>>> @@ -613,37 +618,28 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v, u64 iova, u64 size) >>>>>> } >>>>>> } >>>>>> -static int vhost_vdpa_process_iotlb_update(struct vhost_vdpa *v, >>>>>> - struct vhost_iotlb_msg *msg) >>>>>> +static int vhost_vdpa_pa_map(struct vhost_vdpa *v, >>>>>> + u64 iova, u64 size, u64 uaddr, u32 perm) >>>>>> { >>>>>> struct vhost_dev *dev = &v->vdev; >>>>>> - struct vhost_iotlb *iotlb = dev->iotlb; >>>>>> struct page **page_list; >>>>>> unsigned long list_size = PAGE_SIZE / sizeof(struct page *); >>>>>> unsigned int gup_flags = FOLL_LONGTERM; >>>>>> unsigned long npages, cur_base, map_pfn, last_pfn = 0; >>>>>> unsigned long lock_limit, sz2pin, nchunks, i; >>>>>> - u64 iova = msg->iova; >>>>>> + u64 start = iova; >>>>>> long pinned; >>>>>> int ret = 0; >>>>>> - if (msg->iova < v->range.first || >>>>>> - msg->iova + msg->size - 1 > v->range.last) >>>>>> - return -EINVAL; >>>>> This is not related to your patch, but can the "msg->iova + msg->size" >>>>> addition can have an integer overflow. From looking at the callers it >>>>> seems like it can. msg comes from: >>>>> vhost_chr_write_iter() >>>>> --> dev->msg_handler(dev, &msg); >>>>> --> vhost_vdpa_process_iotlb_msg() >>>>> --> vhost_vdpa_process_iotlb_update() >>>> Yes. >>>> >>>> >>>>> If I'm thinking of the right thing then these are allowed to overflow to >>>>> 0 because of the " - 1" but not further than that. I believe the check >>>>> needs to be something like: >>>>> >>>>> if (msg->iova < v->range.first || >>>>> msg->iova - 1 > U64_MAX - msg->size || >>>> I guess we don't need - 1 here? >>> The - 1 is important. The highest address is 0xffffffff. So it goes >>> start + size = 0 and then start + size - 1 == 0xffffffff. >> >> Right, so actually >> >> msg->iova = 0xfffffffe, msg->size=2 is valid. > I believe so, yes. It's inclusive of 0xfffffffe and 0xffffffff. > (Not an expert). I think so, and we probably need to fix vhost_overflow() as well which did: static bool vhost_overflow(u64 uaddr, u64 size) { /* Make sure 64 bit math will not overflow. */ return uaddr > ULONG_MAX || size > ULONG_MAX || uaddr > ULONG_MAX - size; } Thanks > > regards, > dan carpenter >
This series introduces a framework that makes it possible to implement software-emulated vDPA devices in userspace. And to make the device emulation more secure, the emulated vDPA device's control path is handled in the kernel and only the data path is implemented in the userspace. Since the emuldated vDPA device's control path is handled in the kernel, a message mechnism is introduced to make userspace be aware of the data path related changes. Userspace can use read()/write() to receive/reply the control messages. In the data path, the core is mapping dma buffer into VDUSE daemon's address space, which can be implemented in different ways depending on the vdpa bus to which the vDPA device is attached. In virtio-vdpa case, we implements a MMU-based on-chip IOMMU driver with bounce-buffering mechanism to achieve that. And in vhost-vdpa case, the dma buffer is reside in a userspace memory region which can be shared to the VDUSE userspace processs via transferring the shmfd. The details and our user case is shown below: ------------------------ ------------------------- ---------------------------------------------- | Container | | QEMU(VM) | | VDUSE daemon | | --------- | | ------------------- | | ------------------------- ---------------- | | |dev/vdx| | | |/dev/vhost-vdpa-x| | | | vDPA device emulation | | block driver | | ------------+----------- -----------+------------ -------------+----------------------+--------- | | | | | | | | ------------+---------------------------+----------------------------+----------------------+--------- | | block device | | vhost device | | vduse driver | | TCP/IP | | | -------+-------- --------+-------- -------+-------- -----+---- | | | | | | | | ----------+---------- ----------+----------- -------+------- | | | | virtio-blk driver | | vhost-vdpa driver | | vdpa device | | | | ----------+---------- ----------+----------- -------+------- | | | | virtio bus | | | | | --------+----+----------- | | | | | | | | | | | ----------+---------- | | | | | | virtio-blk device | | | | | | ----------+---------- | | | | | | | | | | | -----------+----------- | | | | | | virtio-vdpa driver | | | | | | -----------+----------- | | | | | | | | vdpa bus | | | -----------+----------------------+---------------------------+------------ | | | ---+--- | -----------------------------------------------------------------------------------------| NIC |------ ---+--- | ---------+--------- | Remote Storages | ------------------- We make use of it to implement a block device connecting to our distributed storage, which can be used both in containers and VMs. Thus, we can have an unified technology stack in this two cases. To test it with null-blk: $ qemu-storage-daemon \ --chardev socket,id=charmonitor,path=/tmp/qmp.sock,server,nowait \ --monitor chardev=charmonitor \ --blockdev driver=host_device,cache.direct=on,aio=native,filename=/dev/nullb0,node-name=disk0 \ --export type=vduse-blk,id=test,node-name=disk0,writable=on,name=vduse-null,num-queues=16,queue-size=128 The qemu-storage-daemon can be found at https://github.com/bytedance/qemu/tree/vduse To make the userspace VDUSE processes such as qemu-storage-daemon able to be run by an unprivileged user. We limit the supported device type to virtio block device currently. The support for other device types can be added after the security issue of corresponding device driver is clarified or fixed in the future. This series is now based on: https://lore.kernel.org/lkml/20210618084412.18257-1-zhe.he@windriver.com/ https://lore.kernel.org/lkml/20210705071910.31965-1-jasowang@redhat.com/ https://lore.kernel.org/lkml/20210602021536.39525-1-jasowang@redhat.com/ Future work: - Improve performance - Userspace library (find a way to reuse device emulation code in qemu/rust-vmm) - Support more device types V8 to V9: - Add VDUSE_SET_STATUS message to replace VDUSE_START/STOP_DATAPLANE messages - Support packed virtqueue state - Handle the reset failure in both virtio-vdpa and vhost-vdpa cases - Add more details in documentation - Remove VDUSE_REQ_FLAGS_NO_REPLY flag - Add VDUSE_VQ_SETUP ioctl to support per-vq configuration - Separate config interrupt injecting out of config update - Flush kworker for interrupt inject during resetting - Validate the config_size in .get_config() V7 to V8: - Rebased to newest kernel tree - Rework VDUSE driver to handle the device's control path in kernel - Limit the supported device type to virtio block device - Export free_iova_fast() - Remove the virtio-blk and virtio-scsi patches (will send them alone) - Remove all module parameters - Use the same MAJOR for both control device and VDUSE devices - Avoid eventfd cleanup in vduse_dev_release() V6 to V7: - Export alloc_iova_fast() - Add get_config_size() callback - Add some patches to avoid trusting virtio devices - Add limited device emulation - Add some documents - Use workqueue to inject config irq - Add parameter on vq irq injecting - Rename vduse_domain_get_mapping_page() to vduse_domain_get_coherent_page() - Add WARN_ON() to catch message failure - Add some padding/reserved fields to uAPI structure - Fix some bugs - Rebase to vhost.git V5 to V6: - Export receive_fd() instead of __receive_fd() - Factor out the unmapping logic of pa and va separatedly - Remove the logic of bounce page allocation in page fault handler - Use PAGE_SIZE as IOVA allocation granule - Add EPOLLOUT support - Enable setting API version in userspace - Fix some bugs V4 to V5: - Remove the patch for irq binding - Use a single IOTLB for all types of mapping - Factor out vhost_vdpa_pa_map() - Add some sample codes in document - Use receice_fd_user() to pass file descriptor - Fix some bugs V3 to V4: - Rebase to vhost.git - Split some patches - Add some documents - Use ioctl to inject interrupt rather than eventfd - Enable config interrupt support - Support binding irq to the specified cpu - Add two module parameter to limit bounce/iova size - Create char device rather than anon inode per vduse - Reuse vhost IOTLB for iova domain - Rework the message mechnism in control path V2 to V3: - Rework the MMU-based IOMMU driver - Use the iova domain as iova allocator instead of genpool - Support transferring vma->vm_file in vhost-vdpa - Add SVA support in vhost-vdpa - Remove the patches on bounce pages reclaim V1 to V2: - Add vhost-vdpa support - Add some documents - Based on the vdpa management tool - Introduce a workqueue for irq injection - Replace interval tree with array map to store the iova_mapu Xie Yongji (17): iova: Export alloc_iova_fast() and free_iova_fast() file: Export receive_fd() to modules vdpa: Fix code indentation vdpa: Fail the vdpa_reset() if fail to set device status to zero vhost-vdpa: Fail the vhost_vdpa_set_status() on reset failure vhost-vdpa: Handle the failure of vdpa_reset() virtio: Don't set FAILED status bit on device index allocation failure virtio_config: Add a return value to reset function virtio-vdpa: Handle the failure of vdpa_reset() virtio: Handle device reset failure in register_virtio_device() vhost-iotlb: Add an opaque pointer for vhost IOTLB vdpa: Add an opaque pointer for vdpa_config_ops.dma_map() vdpa: factor out vhost_vdpa_pa_map() and vhost_vdpa_pa_unmap() vdpa: Support transferring virtual addressing during DMA mapping vduse: Implement an MMU-based IOMMU driver vduse: Introduce VDUSE - vDPA Device in Userspace Documentation: Add documentation for VDUSE Documentation/userspace-api/index.rst | 1 + Documentation/userspace-api/ioctl/ioctl-number.rst | 1 + Documentation/userspace-api/vduse.rst | 248 ++++ arch/um/drivers/virtio_uml.c | 4 +- drivers/iommu/iova.c | 2 + drivers/platform/mellanox/mlxbf-tmfifo.c | 4 +- drivers/remoteproc/remoteproc_virtio.c | 4 +- drivers/s390/virtio/virtio_ccw.c | 6 +- drivers/vdpa/Kconfig | 10 + drivers/vdpa/Makefile | 1 + drivers/vdpa/ifcvf/ifcvf_main.c | 2 +- drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +- drivers/vdpa/vdpa.c | 9 +- drivers/vdpa/vdpa_sim/vdpa_sim.c | 8 +- drivers/vdpa/vdpa_user/Makefile | 5 + drivers/vdpa/vdpa_user/iova_domain.c | 545 +++++++ drivers/vdpa/vdpa_user/iova_domain.h | 73 + drivers/vdpa/vdpa_user/vduse_dev.c | 1502 ++++++++++++++++++++ drivers/vdpa/virtio_pci/vp_vdpa.c | 2 +- drivers/vhost/iotlb.c | 20 +- drivers/vhost/vdpa.c | 168 ++- drivers/virtio/virtio.c | 17 +- drivers/virtio/virtio_pci_legacy.c | 4 +- drivers/virtio/virtio_pci_modern.c | 4 +- drivers/virtio/virtio_vdpa.c | 4 +- fs/file.c | 6 + include/linux/file.h | 7 +- include/linux/vdpa.h | 46 +- include/linux/vhost_iotlb.h | 3 + include/linux/virtio_config.h | 3 +- include/uapi/linux/vduse.h | 221 +++ 31 files changed, 2858 insertions(+), 74 deletions(-) create mode 100644 Documentation/userspace-api/vduse.rst create mode 100644 drivers/vdpa/vdpa_user/Makefile create mode 100644 drivers/vdpa/vdpa_user/iova_domain.c create mode 100644 drivers/vdpa/vdpa_user/iova_domain.h create mode 100644 drivers/vdpa/vdpa_user/vduse_dev.c create mode 100644 include/uapi/linux/vduse.h