Previously BAR resource requests and the corresponding pci_iomap() were performed on-demand and without synchronisation, which was racy. Rather than add synchronisation, it's simplest to address this by doing both activities from vfio_pci_core_enable(). The resource allocation and/or pci_iomap() can still fail; their status is tracked and existing calls to vfio_pci_core_setup_barmap() will fail in the same way as before. This keeps the point of failure as observed by userspace the same, i.e. failures to request/map unused BARs are benign. Fixes: 7f5764e179c6 ("vfio: use vfio_pci_core_setup_barmap to map bar in mmap") Fixes: 0d77ed3589ac0 ("vfio/pci: Pull BAR mapping setup from read-write path") Signed-off-by: Matt Evans --- drivers/vfio/pci/vfio_pci_core.c | 61 +++++++++++++++++++++++++++----- drivers/vfio/pci/vfio_pci_rdwr.c | 29 ++++++--------- include/linux/vfio_pci_core.h | 1 + 3 files changed, 64 insertions(+), 27 deletions(-) diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index 3f8d093aacf8..c59c61861d81 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -482,6 +482,55 @@ static int vfio_pci_core_runtime_resume(struct device *dev) } #endif /* CONFIG_PM */ +static void __vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev) +{ + struct pci_dev *pdev = vdev->pdev; + int i; + + for (i = 0; i < PCI_STD_NUM_BARS; i++) { + int bar = i + PCI_STD_RESOURCES; + + if (vdev->barmap[bar]) + pci_iounmap(pdev, vdev->barmap[bar]); + if (vdev->have_bar_resource[bar]) + pci_release_selected_regions(pdev, 1 << bar); + vdev->barmap[bar] = NULL; + vdev->have_bar_resource[bar] = false; + } +} + +static void __vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev) +{ + struct pci_dev *pdev = vdev->pdev; + int i; + + /* + * Eager-request BAR resources, and iomap; soft failures are + * allowed, and consumers must check before use. + */ + for (i = 0; i < PCI_STD_NUM_BARS; i++) { + int ret; + int bar = i + PCI_STD_RESOURCES; + void __iomem *io; + + if (pci_resource_len(pdev, i) == 0) + continue; + + ret = pci_request_selected_regions(pdev, 1 << bar, "vfio"); + if (ret) { + pci_warn(vdev->pdev, "Failed to reserve region %d\n", bar); + continue; + } + vdev->have_bar_resource[bar] = true; + + io = pci_iomap(pdev, bar, 0); + if (io) + vdev->barmap[bar] = io; + else + pci_warn(vdev->pdev, "Failed to iomap region %d\n", bar); + } +} + /* * The pci-driver core runtime PM routines always save the device state * before going into suspended state. If the device is going into low power @@ -568,6 +617,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev) if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev)) vdev->has_vga = true; + __vfio_pci_core_map_bars(vdev); return 0; @@ -591,7 +641,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev) struct pci_dev *pdev = vdev->pdev; struct vfio_pci_dummy_resource *dummy_res, *tmp; struct vfio_pci_ioeventfd *ioeventfd, *ioeventfd_tmp; - int i, bar; + int i; /* For needs_reset */ lockdep_assert_held(&vdev->vdev.dev_set->lock); @@ -646,14 +696,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev) vfio_config_free(vdev); - for (i = 0; i < PCI_STD_NUM_BARS; i++) { - bar = i + PCI_STD_RESOURCES; - if (!vdev->barmap[bar]) - continue; - pci_iounmap(pdev, vdev->barmap[bar]); - pci_release_selected_regions(pdev, 1 << bar); - vdev->barmap[bar] = NULL; - } + __vfio_pci_core_unmap_bars(vdev); list_for_each_entry_safe(dummy_res, tmp, &vdev->dummy_resources_list, res_next) { diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c index 4251ee03e146..bf7152316db4 100644 --- a/drivers/vfio/pci/vfio_pci_rdwr.c +++ b/drivers/vfio/pci/vfio_pci_rdwr.c @@ -200,25 +200,18 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw); int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar) { - struct pci_dev *pdev = vdev->pdev; - int ret; - void __iomem *io; - - if (vdev->barmap[bar]) - return 0; - - ret = pci_request_selected_regions(pdev, 1 << bar, "vfio"); - if (ret) - return ret; - - io = pci_iomap(pdev, bar, 0); - if (!io) { - pci_release_selected_regions(pdev, 1 << bar); + /* + * The barmap is now always set up in vfio_pci_core_enable(). + * Some legacy callers use this function to ensure the BAR + * resources are requested, and others to ensure the + * pci_iomap() was done, so check here: + */ + if (bar < 0 || bar >= PCI_STD_NUM_BARS) + return -EINVAL; + if (vdev->barmap[bar] == 0) return -ENOMEM; - } - - vdev->barmap[bar] = io; - + if (!vdev->bar_has_rsrc[bar]) + return -EBUSY; return 0; } EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap); diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index 2ebba746c18f..1f508b067d82 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -101,6 +101,7 @@ struct vfio_pci_core_device { const struct vfio_pci_device_ops *pci_ops; void __iomem *barmap[PCI_STD_NUM_BARS]; bool bar_mmap_supported[PCI_STD_NUM_BARS]; + bool have_bar_resource[PCI_STD_NUM_BARS]; u8 *pci_config_map; u8 *vconfig; struct perm_bits *msi_perm; -- 2.47.3 Since "vfio/pci: Set up barmap in vfio_pci_core_enable()", the resource request and iomap for the BARs was performed early, and vfio_pci_core_setup_barmap() now just checks those actions succeeded. There were two types of callers: - Those that need the iomap, because they'll access the BAR - Those that need the resource, because they'll map/export it This replaces vfio_pci_core_setup_barmap() with two helpers, vfio_pci_core_check_barmap_valid() and vfio_pci_core_check_bar_rsrc(), to make it clear which behaviour is required in each caller. Signed-off-by: Matt Evans --- drivers/vfio/pci/nvgrace-gpu/main.c | 8 +++----- drivers/vfio/pci/vfio_pci_core.c | 5 ++--- drivers/vfio/pci/vfio_pci_rdwr.c | 22 ++-------------------- drivers/vfio/pci/virtio/legacy_io.c | 4 ++-- include/linux/vfio_pci_core.h | 23 ++++++++++++++++++++++- 5 files changed, 31 insertions(+), 31 deletions(-) diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c index fa056b69f899..d5f09144ac84 100644 --- a/drivers/vfio/pci/nvgrace-gpu/main.c +++ b/drivers/vfio/pci/nvgrace-gpu/main.c @@ -184,12 +184,10 @@ static int nvgrace_gpu_open_device(struct vfio_device *core_vdev) /* * GPU readiness is checked by reading the BAR0 registers. - * - * ioremap BAR0 to ensure that the BAR0 mapping is present before - * register reads on first fault before establishing any GPU - * memory mapping. + * Ensure that the BAR0 mapping is present before that + * happens. */ - ret = vfio_pci_core_setup_barmap(vdev, 0); + ret = vfio_pci_core_check_barmap_valid(vdev, 0); if (ret) goto error_exit; diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c index c59c61861d81..2771d0f21899 100644 --- a/drivers/vfio/pci/vfio_pci_core.c +++ b/drivers/vfio/pci/vfio_pci_core.c @@ -1804,10 +1804,9 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma return -EINVAL; /* - * Even though we don't make use of the barmap for the mmap, - * we need to request the region and the barmap tracks that. + * Ensure the BAR resource region is reserved for use. */ - ret = vfio_pci_core_setup_barmap(vdev, index); + ret = vfio_pci_core_check_bar_rsrc(vdev, index); if (ret) return ret; diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c index bf7152316db4..40c97d73ff95 100644 --- a/drivers/vfio/pci/vfio_pci_rdwr.c +++ b/drivers/vfio/pci/vfio_pci_rdwr.c @@ -198,24 +198,6 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem, } EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw); -int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar) -{ - /* - * The barmap is now always set up in vfio_pci_core_enable(). - * Some legacy callers use this function to ensure the BAR - * resources are requested, and others to ensure the - * pci_iomap() was done, so check here: - */ - if (bar < 0 || bar >= PCI_STD_NUM_BARS) - return -EINVAL; - if (vdev->barmap[bar] == 0) - return -ENOMEM; - if (!vdev->bar_has_rsrc[bar]) - return -EBUSY; - return 0; -} -EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap); - ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf, size_t count, loff_t *ppos, bool iswrite) { @@ -267,7 +249,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf, */ max_width = VFIO_PCI_IO_WIDTH_4; } else { - int ret = vfio_pci_core_setup_barmap(vdev, bar); + int ret = vfio_pci_core_check_barmap_valid(vdev, bar); if (ret) { done = ret; goto out; @@ -445,7 +427,7 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset, if (count == 8) return -EINVAL; - ret = vfio_pci_core_setup_barmap(vdev, bar); + ret = vfio_pci_core_check_barmap_valid(vdev, bar); if (ret) return ret; diff --git a/drivers/vfio/pci/virtio/legacy_io.c b/drivers/vfio/pci/virtio/legacy_io.c index 1ed349a55629..9c59d1600ac4 100644 --- a/drivers/vfio/pci/virtio/legacy_io.c +++ b/drivers/vfio/pci/virtio/legacy_io.c @@ -305,8 +305,8 @@ static int virtiovf_set_notify_addr(struct virtiovf_pci_core_device *virtvdev) * Setup the BAR where the 'notify' exists to be used by vfio as well * This will let us mmap it only once and use it when needed. */ - ret = vfio_pci_core_setup_barmap(core_device, - virtvdev->notify_bar); + ret = vfio_pci_core_check_barmap_valid(core_device, + virtvdev->notify_bar); if (ret) return ret; diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index 1f508b067d82..6a5384d57f1d 100644 --- a/include/linux/vfio_pci_core.h +++ b/include/linux/vfio_pci_core.h @@ -189,7 +189,6 @@ int vfio_pci_core_match_token_uuid(struct vfio_device *core_vdev, int vfio_pci_core_enable(struct vfio_pci_core_device *vdev); void vfio_pci_core_disable(struct vfio_pci_core_device *vdev); void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev); -int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar); pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev, pci_channel_state_t state); ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem, @@ -225,6 +224,28 @@ VFIO_IOREAD_DECLARATION(32) VFIO_IOREAD_DECLARATION(64) #endif +/* Returns 0 if vdev->barmap[bar] can be accessed, otherwise errno */ +static inline int +vfio_pci_core_check_barmap_valid(struct vfio_pci_core_device *vdev, int bar) +{ + if (bar < 0 || bar >= PCI_STD_NUM_BARS) + return -EINVAL; + if (vdev->barmap[bar] == 0) + return -ENOMEM; + return 0; +} + +/* Returns 0 if BAR has a valid resource reserved for use, otherwise errno */ +static inline int vfio_pci_core_check_bar_rsrc(struct vfio_pci_core_device *vdev, + int bar) +{ + if (bar < 0 || bar >= PCI_STD_NUM_BARS) + return -EINVAL; + if (!vdev->have_bar_resource[bar]) + return -EBUSY; + return 0; +} + static inline bool is_aligned_for_order(struct vm_area_struct *vma, unsigned long addr, unsigned long pfn, -- 2.47.3 A DMABUF exports access to BAR resources and, although they are requested at startup time, we need to ensure they really were reserved before exporting. Otherwise, it's possible to access unreserved resources through the export. Add a check to the DMABUF-creation path. Fixes: 5d74781ebc86c ("vfio/pci: Add dma-buf export support for MMIO regions") Signed-off-by: Matt Evans --- drivers/vfio/pci/vfio_pci_dmabuf.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c index f87fd32e4a01..9139198ae270 100644 --- a/drivers/vfio/pci/vfio_pci_dmabuf.c +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c @@ -244,9 +244,11 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags, return -EINVAL; /* - * For PCI the region_index is the BAR number like everything else. + * For PCI the region_index is the BAR number like everything + * else. Check that PCI resources have been claimed for it. */ - if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX) + if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX || + vfio_pci_core_check_bar_rsrc(vdev, get_dma_buf.region_index) != 0) return -ENODEV; dma_ranges = memdup_array_user(&arg->dma_ranges, get_dma_buf.nr_ranges, -- 2.47.3