Move out dmabuf detachment and unmapping into separate function. This
removes duplicated code and there is no need to check the GEM's kref now,
since both bo->attached and bo->sgt are unset under held reservation lock.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 drivers/gpu/drm/virtio/virtgpu_prime.c | 35 ++++++++++++++------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
index 33084ce1d01d..101d1a6517ae 100644
--- a/drivers/gpu/drm/virtio/virtgpu_prime.c
+++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
@@ -184,22 +184,33 @@ int virtgpu_dma_buf_import_sgt(struct virtio_gpu_mem_entry **ents,
 	return 0;
 }
 
-static void virtgpu_dma_buf_free_obj(struct drm_gem_object *obj)
+static void virtgpu_dma_buf_unmap(struct virtio_gpu_object *bo)
 {
-	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;
-	struct dma_resv *resv = attach->dmabuf->resv;
+	struct dma_buf_attachment *attach = bo->base.base.import_attach;
 
-	if (attach) {
-		dma_resv_lock(resv, NULL);
+	dma_resv_assert_held(attach->dmabuf->resv);
 
+	if (bo->created) {
 		virtio_gpu_detach_object_fenced(bo);
 
 		if (bo->sgt)
 			dma_buf_unmap_attachment(attach, bo->sgt,
 						 DMA_BIDIRECTIONAL);
 
+		bo->sgt = NULL;
+	}
+}
+
+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;
+	struct dma_resv *resv = attach->dmabuf->resv;
+
+	if (attach) {
+		dma_resv_lock(resv, NULL);
+		virtgpu_dma_buf_unmap(bo);
 		dma_resv_unlock(resv);
 
 		dma_buf_detach(attach->dmabuf, attach);
@@ -272,15 +283,7 @@ static void virtgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
 	struct drm_gem_object *obj = attach->importer_priv;
 	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
 
-	if (bo->created && kref_read(&obj->refcount)) {
-		virtio_gpu_detach_object_fenced(bo);
-
-		if (bo->sgt)
-			dma_buf_unmap_attachment(attach, bo->sgt,
-						 DMA_BIDIRECTIONAL);
-
-		bo->sgt = NULL;
-	}
+	virtgpu_dma_buf_unmap(bo);
 }
 
 static const struct dma_buf_attach_ops virtgpu_dma_buf_attach_ops = {
-- 
2.47.0

On 12/2/24 08:39, Dmitry Osipenko wrote:
> Move out dmabuf detachment and unmapping into separate function. This
> removes duplicated code and there is no need to check the GEM's kref now,
> since both bo->attached and bo->sgt are unset under held reservation lock.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_prime.c | 35 ++++++++++++++------------
>  1 file changed, 19 insertions(+), 16 deletions(-)

Made a minor rebase on top of the recent UAF fix and applied to misc-next.

-- 
Best regards,
Dmitry

Hi Dmitry,

> Subject: [PATCH v1] drm/virtio: Factor out common dmabuf unmapping code
> 
> Move out dmabuf detachment and unmapping into separate function. This
> removes duplicated code and there is no need to check the GEM's kref now,
> since both bo->attached and bo->sgt are unset under held reservation lock.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_prime.c | 35 ++++++++++++++------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c
> b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index 33084ce1d01d..101d1a6517ae 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -184,22 +184,33 @@ int virtgpu_dma_buf_import_sgt(struct
> virtio_gpu_mem_entry **ents,
>  	return 0;
>  }
> 
> -static void virtgpu_dma_buf_free_obj(struct drm_gem_object *obj)
> +static void virtgpu_dma_buf_unmap(struct virtio_gpu_object *bo)
>  {
> -	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;
> -	struct dma_resv *resv = attach->dmabuf->resv;
> +	struct dma_buf_attachment *attach = bo->base.base.import_attach;
> 
> -	if (attach) {
> -		dma_resv_lock(resv, NULL);
> +	dma_resv_assert_held(attach->dmabuf->resv);
> 
> +	if (bo->created) {
>  		virtio_gpu_detach_object_fenced(bo);
> 
>  		if (bo->sgt)
>  			dma_buf_unmap_attachment(attach, bo->sgt,
>  						 DMA_BIDIRECTIONAL);
> 
> +		bo->sgt = NULL;
> +	}
> +}
> +
> +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;
> +	struct dma_resv *resv = attach->dmabuf->resv;
> +
> +	if (attach) {
> +		dma_resv_lock(resv, NULL);
> +		virtgpu_dma_buf_unmap(bo);
>  		dma_resv_unlock(resv);
> 
>  		dma_buf_detach(attach->dmabuf, attach);
> @@ -272,15 +283,7 @@ static void virtgpu_dma_buf_move_notify(struct
> dma_buf_attachment *attach)
>  	struct drm_gem_object *obj = attach->importer_priv;
>  	struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
> 
> -	if (bo->created && kref_read(&obj->refcount)) {
> -		virtio_gpu_detach_object_fenced(bo);
> -
> -		if (bo->sgt)
> -			dma_buf_unmap_attachment(attach, bo->sgt,
> -						 DMA_BIDIRECTIONAL);
> -
> -		bo->sgt = NULL;
> -	}
> +	virtgpu_dma_buf_unmap(bo);
LGTM.
Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

>  }
> 
>  static const struct dma_buf_attach_ops virtgpu_dma_buf_attach_ops = {
> --
> 2.47.0
>