On 11/25/24 19:27, Dmitry Osipenko wrote:
> On 11/25/24 10:31, Vivek Kasireddy wrote:
>> +static void virtgpu_dma_buf_free_obj(struct drm_gem_object *obj)
>> +{
>> + struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
>> + struct virtio_gpu_device *vgdev = obj->dev->dev_private;
>> + struct dma_buf_attachment *attach = obj->import_attach;
>> +
>> + if (attach) {
>> + virtio_gpu_detach_object_fenced(bo);
>> +
>> + if (bo->sgt)
>> + dma_buf_unmap_attachment(attach, bo->sgt,
>> + DMA_BIDIRECTIONAL);
>
> I've caught this problem, we're not holding resv lock:
>
> [ 28.550460] WARNING: CPU: 6 PID: 759 at
> drivers/dma-buf/dma-buf.c:1228 dma_buf_unmap_attachment+0x84/0x90
> ...
> [ 28.554217] ? __warn.cold+0xb7/0x150
> [ 28.554322] ? dma_buf_unmap_attachment+0x84/0x90
> [ 28.554455] ? report_bug+0xf7/0x140
> [ 28.554560] ? handle_bug+0x4f/0x90
> [ 28.554661] ? exc_invalid_op+0x13/0x60
> [ 28.554767] ? asm_exc_invalid_op+0x16/0x20
> [ 28.554885] ? dma_buf_unmap_attachment+0x84/0x90
> [ 28.555018] ? dma_buf_unmap_attachment+0x80/0x90
> [ 28.555160] virtgpu_dma_buf_free_obj+0x3b/0x80
> [ 28.555294] drm_gem_object_release_handle+0x4d/0x60
> [ 28.555435] ? drm_gem_object_handle_put_unlocked+0xe0/0xe0
> [ 28.555587] idr_for_each+0x4b/0xb0
> [ 28.555691] drm_gem_release+0x1f/0x30
> [ 28.555798] drm_file_free+0x202/0x290
> [ 28.555905] drm_release+0x5f/0xc0
> [ 28.556001] __fput+0xf9/0x2b0
> [ 28.556093] task_work_run+0x55/0x90
> [ 28.556219] do_exit+0x313/0xaa0
> [ 28.556315] ? lock_release+0xb6/0x260
> [ 28.556423] do_group_exit+0x32/0xa0
> [ 28.556525] __x64_sys_exit_group+0x14/0x20
> [ 28.556645] x64_sys_call+0x714/0x720
> [ 28.556751] do_syscall_64+0x54/0xf0
> [ 28.556854] entry_SYSCALL_64_after_hwframe+0x4b/0x53
>
> both virtio_gpu_detach_object_fenced() and dma_buf_unmap_attachment()
> need to be called under resv lock.
>
> Won't hurt adding dma_resv_assert_held() to
> virtio_gpu_detach_object_fenced().
>
On the other hand, nobody uses BO when it's freed, hence locking
virtio_gpu_detach_object_fenced() not needed. Should be enough to use
dma_buf_unmap_attachment_unlocked() then.
--
Best regards,
Dmitry
