Re: [PATCH v3 1/2] drm/virtio: factor out the sg_table from virtio_gpu_object

On 05. 03. 20, 2:32, Gurchetan Singh wrote:

A resource will be a shmem based resource or a (planned) vram based resource, so it makes sense to factor out common fields (resource handle, dumb).

v2: move mapped field to shmem object

Hi,

I bisected the slab-out-of-bounds below to this patch. Is it known?

[drm] pci: virtio-vga detected at 0000:00:0a.0 virtio-pci 0000:00:0a.0: vgaarb: deactivate vga console [drm] features: -virgl +edid [drm] number of scanouts: 1 [drm] number of cap sets: 0 [drm] Initialized virtio_gpu 0.1.0 0 for virtio6 on minor 0 ================================================================== BUG: KASAN: slab-out-of-bounds in virtio_gpu_object_create+0x938/0xa10 Write of size 8 at addr ffff8880c321b5c8 by task swapper/0/1

CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.6.0-rc2+ #38 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 Call Trace: dump_stack (/dev/shm/jslaby/linux/lib/dump_stack.c:120) print_address_description.constprop.0 (/dev/shm/jslaby/linux/mm/kasan/report.c:375) __kasan_report.cold (/dev/shm/jslaby/linux/mm/kasan/report.c:507) kasan_report (/dev/shm/jslaby/linux/./arch/x86/include/asm/smap.h:69 /dev/shm/jslaby/linux/mm/kasan/common.c:642) virtio_gpu_object_create (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_object.c:145 /dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_object.c:224) virtio_gpu_gem_create (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_gem.c:42) virtio_gpu_mode_dumb_create (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_gem.c:84) drm_client_framebuffer_create (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_client.c:268 /dev/shm/jslaby/linux/drivers/gpu/drm/drm_client.c:412) drm_fb_helper_generic_probe (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2041) __drm_fb_helper_initial_config_and_unlock (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:1588 /dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:1746) drm_fbdev_client_hotplug (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2134) drm_fbdev_generic_setup (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2212 /dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2185) virtio_gpu_probe (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_drv.c:128) virtio_dev_probe (/dev/shm/jslaby/linux/drivers/virtio/virtio.c:248) really_probe (/dev/shm/jslaby/linux/drivers/base/dd.c:551) driver_probe_device (/dev/shm/jslaby/linux/drivers/base/dd.c:724) device_driver_attach (/dev/shm/jslaby/linux/drivers/base/dd.c:998) __driver_attach (/dev/shm/jslaby/linux/drivers/base/dd.c:1077) bus_for_each_dev (/dev/shm/jslaby/linux/drivers/base/bus.c:305) bus_add_driver (/dev/shm/jslaby/linux/drivers/base/bus.c:623) driver_register (/dev/shm/jslaby/linux/drivers/base/driver.c:171) do_one_initcall (/dev/shm/jslaby/linux/init/main.c:1146) kernel_init_freeable (/dev/shm/jslaby/linux/init/main.c:1218 /dev/shm/jslaby/linux/init/main.c:1235 /dev/shm/jslaby/linux/init/main.c:1255 /dev/shm/jslaby/linux/init/main.c:1439) kernel_init (/dev/shm/jslaby/linux/init/main.c:1348) ret_from_fork (/dev/shm/jslaby/linux/arch/x86/entry/entry_64.S:358)

Allocated by task 1: save_stack (/dev/shm/jslaby/linux/mm/kasan/common.c:72) __kasan_kmalloc.constprop.0 (/dev/shm/jslaby/linux/mm/kasan/common.c:80 /dev/shm/jslaby/linux/mm/kasan/common.c:515 /dev/shm/jslaby/linux/mm/kasan/common.c:488) virtio_gpu_create_object (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_object.c:124) drm_gem_shmem_create (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_gem_shmem_helper.c:58) virtio_gpu_object_create (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_object.c:193) virtio_gpu_gem_create (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_gem.c:42) virtio_gpu_mode_dumb_create (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_gem.c:84) drm_client_framebuffer_create (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_client.c:268 /dev/shm/jslaby/linux/drivers/gpu/drm/drm_client.c:412) drm_fb_helper_generic_probe (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2041) __drm_fb_helper_initial_config_and_unlock (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:1588 /dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:1746) drm_fbdev_client_hotplug (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2134) drm_fbdev_generic_setup (/dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2212 /dev/shm/jslaby/linux/drivers/gpu/drm/drm_fb_helper.c:2185) virtio_gpu_probe (/dev/shm/jslaby/linux/drivers/gpu/drm/virtio/virtgpu_drv.c:128) virtio_dev_probe (/dev/shm/jslaby/linux/drivers/virtio/virtio.c:248) really_probe (/dev/shm/jslaby/linux/drivers/base/dd.c:551) driver_probe_device (/dev/shm/jslaby/linux/drivers/base/dd.c:724) device_driver_attach (/dev/shm/jslaby/linux/drivers/base/dd.c:998) __driver_attach (/dev/shm/jslaby/linux/drivers/base/dd.c:1077) bus_for_each_dev (/dev/shm/jslaby/linux/drivers/base/bus.c:305) bus_add_driver (/dev/shm/jslaby/linux/drivers/base/bus.c:623) driver_register (/dev/shm/jslaby/linux/drivers/base/driver.c:171) do_one_initcall (/dev/shm/jslaby/linux/init/main.c:1146) kernel_init_freeable (/dev/shm/jslaby/linux/init/main.c:1218 /dev/shm/jslaby/linux/init/main.c:1235 /dev/shm/jslaby/linux/init/main.c:1255 /dev/shm/jslaby/linux/init/main.c:1439) kernel_init (/dev/shm/jslaby/linux/init/main.c:1348) ret_from_fork (/dev/shm/jslaby/linux/arch/x86/entry/entry_64.S:358)

Freed by task 1: save_stack (/dev/shm/jslaby/linux/mm/kasan/common.c:72) __kasan_slab_free (/dev/shm/jslaby/linux/mm/kasan/common.c:478) kfree (/dev/shm/jslaby/linux/mm/slub.c:1477 /dev/shm/jslaby/linux/mm/slub.c:3024 /dev/shm/jslaby/linux/mm/slub.c:3976) consume_skb (/dev/shm/jslaby/linux/net/core/skbuff.c:681 /dev/shm/jslaby/linux/net/core/skbuff.c:839 /dev/shm/jslaby/linux/net/core/skbuff.c:833) netlink_broadcast_filtered (/dev/shm/jslaby/linux/./include/asm-generic/atomic-instrumented.h:747 /dev/shm/jslaby/linux/net/netlink/af_netlink.c:465 /dev/shm/jslaby/linux/net/netlink/af_netlink.c:1514) netlink_broadcast (/dev/shm/jslaby/linux/net/netlink/af_netlink.c:1534) genl_ctrl_event (/dev/shm/jslaby/linux/net/netlink/genetlink.c:1042) genl_register_family (/dev/shm/jslaby/linux/net/netlink/genetlink.c:380 /dev/shm/jslaby/linux/net/netlink/genetlink.c:322) acpi_event_init (/dev/shm/jslaby/linux/drivers/acpi/event.c:178) do_one_initcall (/dev/shm/jslaby/linux/init/main.c:1146) kernel_init_freeable (/dev/shm/jslaby/linux/init/main.c:1218 /dev/shm/jslaby/linux/init/main.c:1235 /dev/shm/jslaby/linux/init/main.c:1255 /dev/shm/jslaby/linux/init/main.c:1439) kernel_init (/dev/shm/jslaby/linux/init/main.c:1348) ret_from_fork (/dev/shm/jslaby/linux/arch/x86/entry/entry_64.S:358)

Signed-off-by: Gurchetan Singh gurchetansingh@chromium.org

drivers/gpu/drm/virtio/virtgpu_drv.h | 13 +++++++---- drivers/gpu/drm/virtio/virtgpu_object.c | 31 ++++++++++++++----------- drivers/gpu/drm/virtio/virtgpu_vq.c | 6 +++-- 3 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index ce73895cf74b..8e2027da6cce 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -69,16 +69,21 @@ struct virtio_gpu_object_params { struct virtio_gpu_object { struct drm_gem_shmem_object base; uint32_t hw_res_handle;

• struct sg_table *pages;
• uint32_t mapped;
• bool dumb; bool created;

}; #define gem_to_virtio_gpu_obj(gobj) \ container_of((gobj), struct virtio_gpu_object, base.base)

+struct virtio_gpu_object_shmem {

• struct virtio_gpu_object base;
• struct sg_table *pages;
• uint32_t mapped;

+};

+#define to_virtio_gpu_shmem(virtio_gpu_object) \

• container_of((virtio_gpu_object), struct virtio_gpu_object_shmem, base)

struct virtio_gpu_object_array { struct ww_acquire_ctx ticket; struct list_head next; diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index c5cad949eb8d..1f8b062bb9eb 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -65,16 +65,17 @@ static void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo) { struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;

• struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);

• if (bo->pages) {

• if (bo->mapped) {
  

• if (shmem->pages) {

• if (shmem->mapped) {
  dma_unmap_sg(vgdev->vdev->dev.parent,
  

• 		     bo->pages->sgl, bo->mapped,
  

• 		     shmem->pages->sgl, shmem->mapped,
  	     DMA_TO_DEVICE);
  

• 	bo->mapped = 0;
  

• 	shmem->mapped = 0;
  

  }

• sg_free_table(bo->pages);
  

• bo->pages = NULL;
  

• sg_free_table(shmem->pages);
  

• shmem->pages = NULL;
  

  drm_gem_shmem_unpin(&bo->base.base); } virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);

@@ -133,6 +134,7 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev, unsigned int *nents) { bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);

• struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo); struct scatterlist *sg; int si, ret;

@@ -140,19 +142,20 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev, if (ret < 0) return -EINVAL;

• bo->pages = drm_gem_shmem_get_sg_table(&bo->base.base);
• if (!bo->pages) {

• shmem->pages = drm_gem_shmem_get_sg_table(&bo->base.base);

• if (!shmem->pages) { drm_gem_shmem_unpin(&bo->base.base); return -EINVAL; }

  if (use_dma_api) {

• bo->mapped = dma_map_sg(vgdev->vdev->dev.parent,
  

• 			bo->pages->sgl, bo->pages->nents,
  

• 			DMA_TO_DEVICE);
  

• *nents = bo->mapped;
  

• shmem->mapped = dma_map_sg(vgdev->vdev->dev.parent,
  

• 			   shmem->pages->sgl,
  

• 			   shmem->pages->nents,
  

• 			   DMA_TO_DEVICE);
  

• *nents = shmem->mapped;
  

  } else {

• *nents = bo->pages->nents;
  

• *nents = shmem->pages->nents;
  

  }

  *ents = kmalloc_array(*nents, sizeof(struct virtio_gpu_mem_entry),

@@ -162,7 +165,7 @@ static int virtio_gpu_object_shmem_init(struct virtio_gpu_device *vgdev, return -ENOMEM; }

• for_each_sg(bo->pages->sgl, sg, *nents, si) {

• for_each_sg(shmem->pages->sgl, sg, *nents, si) { (*ents)[si].addr = cpu_to_le64(use_dma_api ? sg_dma_address(sg) : sg_phys(sg));

diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c index 5e2375e0f7bb..73854915ec34 100644 --- a/drivers/gpu/drm/virtio/virtgpu_vq.c +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c @@ -600,10 +600,11 @@ void virtio_gpu_cmd_transfer_to_host_2d(struct virtio_gpu_device *vgdev, struct virtio_gpu_transfer_to_host_2d *cmd_p; struct virtio_gpu_vbuffer *vbuf; bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);

• struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);

  if (use_dma_api) dma_sync_sg_for_device(vgdev->vdev->dev.parent,

• 		       bo->pages->sgl, bo->pages->nents,
  

• 		       shmem->pages->sgl, shmem->pages->nents,
  	       DMA_TO_DEVICE);
  
  

  cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));

@@ -1015,10 +1016,11 @@ void virtio_gpu_cmd_transfer_to_host_3d(struct virtio_gpu_device *vgdev, struct virtio_gpu_transfer_host_3d *cmd_p; struct virtio_gpu_vbuffer *vbuf; bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);

• struct virtio_gpu_object_shmem *shmem = to_virtio_gpu_shmem(bo);

  if (use_dma_api) dma_sync_sg_for_device(vgdev->vdev->dev.parent,

• 		       bo->pages->sgl, bo->pages->nents,
  

• 		       shmem->pages->sgl, shmem->pages->nents,
  	       DMA_TO_DEVICE);
  
  

  cmd_p = virtio_gpu_alloc_cmd(vgdev, &vbuf, sizeof(*cmd_p));

thanks,