[PATCH 1/1] drm/qxl: add mutex_lock/mutex_unlock to ensure the order in which resources are released.

[PATCH 1/1] drm/qxl: add mutex_lock/mutex_unlock to ensure the order in which resources are released.

[Caicai]

When a qxl resource is released, the list that needs to be released is
fetched from the linked list ring and cleared. When you empty the list,
instead of trying to determine whether the ttm buffer object for each
qxl in the list is locked, you release the qxl object and remove the
element from the list until the list is empty. It was found that the
linked list was cleared first, and that the lock on the corresponding
ttm Bo for the QXL had not been released, so that the new qxl could not
be locked when it used the TTM. By adding an additional mutex to ensure
timing, qxl elements are not allowed to be removed from the list until
ttm Bo's lock is released

Signed-off-by: Caicai <caizhaopeng@uniontech.com>
---
 drivers/gpu/drm/qxl/qxl_cmd.c     | 12 +++++++++++-
 drivers/gpu/drm/qxl/qxl_display.c | 15 +++++++++++++++
 drivers/gpu/drm/qxl/qxl_draw.c    | 19 +++++++++++++++++++
 drivers/gpu/drm/qxl/qxl_drv.h     |  3 ++-
 drivers/gpu/drm/qxl/qxl_ioctl.c   |  3 +++
 drivers/gpu/drm/qxl/qxl_release.c |  2 ++
 6 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
index 208af9f37914..484405dae253 100644
--- a/drivers/gpu/drm/qxl/qxl_cmd.c
+++ b/drivers/gpu/drm/qxl/qxl_cmd.c
@@ -203,7 +203,9 @@ bool qxl_queue_garbage_collect(struct qxl_device *qdev, bool flush)
 	if (!qxl_check_idle(qdev->release_ring)) {
 		schedule_work(&qdev->gc_work);
 		if (flush)
-			flush_work(&qdev->gc_work);
+			//can't flush work, it may lead to deadlock
+			usleep_range(500, 1000);
+
 		return true;
 	}
 	return false;
@@ -241,7 +243,11 @@ int qxl_garbage_collect(struct qxl_device *qdev)
 			}
 			id = next_id;
 
+			mutex_lock(&release->async_release_mutex);
+
 			qxl_release_free(qdev, release);
+
+			mutex_unlock(&release->async_release_mutex);
 			++i;
 		}
 	}
@@ -475,6 +481,8 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 	if (ret)
 		return ret;
 
+	mutex_lock(&release->async_release_mutex);
+
 	cmd = (struct qxl_surface_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_SURFACE_CMD_CREATE;
 	cmd->flags = QXL_SURF_FLAG_KEEP_DATA;
@@ -503,6 +511,8 @@ int qxl_hw_surface_alloc(struct qxl_device *qdev,
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_SURFACE, false);
 	qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
+
 	surf->hw_surf_alloc = true;
 	spin_lock(&qdev->surf_id_idr_lock);
 	idr_replace(&qdev->surf_id_idr, surf, surf->surface_id);
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 0570c6826bff..04bfb181a402 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -522,6 +522,8 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
 	if (ret)
 		goto out_free_release;
 
+	mutex_lock(&release->async_release_mutex);
+
 	cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_CURSOR_SET;
 	cmd->u.set.position.x = plane->state->crtc_x + fb->hot_x;
@@ -535,6 +537,8 @@ static int qxl_primary_apply_cursor(struct drm_plane *plane)
 	qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
 	qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
+
 	return ret;
 
 out_free_release:
@@ -653,6 +657,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		if (ret)
 			goto out_free_bo;
 
+		mutex_lock(&release->async_release_mutex);
+
 		ret = qxl_bo_kmap(cursor_bo, (void **)&cursor);
 		if (ret)
 			goto out_backoff;
@@ -686,6 +692,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		if (ret)
 			goto out_free_release;
 
+		mutex_lock(&release->async_release_mutex);
+
 		cmd = (struct qxl_cursor_cmd *) qxl_release_map(qdev, release);
 		cmd->type = QXL_CURSOR_MOVE;
 	}
@@ -697,6 +705,8 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 	qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
 	qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
+
 	if (old_cursor_bo)
 		qxl_bo_unref(&old_cursor_bo);
 
@@ -706,6 +716,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 
 out_backoff:
 	qxl_release_backoff_reserve_list(release);
+	mutex_unlock(&release->async_release_mutex);
 out_free_bo:
 	qxl_bo_unref(&cursor_bo);
 out_kunmap:
@@ -736,12 +747,16 @@ static void qxl_cursor_atomic_disable(struct drm_plane *plane,
 		return;
 	}
 
+	mutex_lock(&release->async_release_mutex);
+
 	cmd = (struct qxl_cursor_cmd *)qxl_release_map(qdev, release);
 	cmd->type = QXL_CURSOR_HIDE;
 	qxl_release_unmap(qdev, release, &cmd->release_info);
 
 	qxl_push_cursor_ring_release(qdev, release, QXL_CMD_CURSOR, false);
 	qxl_release_fence_buffer_objects(release);
+
+	mutex_unlock(&release->async_release_mutex);
 }
 
 static int qxl_plane_prepare_fb(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index 4d8681e84e68..4dc24739c79c 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -192,6 +192,8 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
 	if (ret)
 		goto out_free_palette;
 
+	mutex_lock(&release->async_release_mutex);
+
 	rect.left = x;
 	rect.right = x + width;
 	rect.top = y;
@@ -200,6 +202,7 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
 	ret = make_drawable(qdev, 0, QXL_DRAW_COPY, &rect, release);
 	if (ret) {
 		qxl_release_backoff_reserve_list(release);
+		mutex_unlock(&release->async_release_mutex);
 		goto out_free_palette;
 	}
 
@@ -208,6 +211,7 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
 			     width, height, depth, stride);
 	if (ret) {
 		qxl_release_backoff_reserve_list(release);
+		mutex_unlock(&release->async_release_mutex);
 		qxl_release_free(qdev, release);
 		return;
 	}
@@ -244,6 +248,8 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false);
 	qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
+
 out_free_palette:
 	if (palette_bo)
 		qxl_bo_unref(&palette_bo);
@@ -326,6 +332,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 	if (ret)
 		goto out_free_image;
 
+	mutex_lock(&release->async_release_mutex);
+
 	drawable_rect.left = left;
 	drawable_rect.right = right;
 	drawable_rect.top = top;
@@ -387,6 +395,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 out_release_backoff:
 	if (ret)
 		qxl_release_backoff_reserve_list(release);
+	mutex_unlock(&release->async_release_mutex);
 out_free_image:
 	qxl_image_free_objects(qdev, dimage);
 out_free_clips:
@@ -417,6 +426,8 @@ void qxl_draw_copyarea(struct qxl_device *qdev,
 	if (ret)
 		goto out_free_release;
 
+	mutex_lock(&release->async_release_mutex);
+
 	rect.left = dx;
 	rect.top = dy;
 	rect.right = dx + width;
@@ -424,6 +435,7 @@ void qxl_draw_copyarea(struct qxl_device *qdev,
 	ret = make_drawable(qdev, 0, QXL_COPY_BITS, &rect, release);
 	if (ret) {
 		qxl_release_backoff_reserve_list(release);
+		mutex_unlock(&release->async_release_mutex);
 		goto out_free_release;
 	}
 
@@ -435,6 +447,8 @@ void qxl_draw_copyarea(struct qxl_device *qdev,
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false);
 	qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
+
 out_free_release:
 	if (ret)
 		free_drawable(qdev, release);
@@ -459,9 +473,12 @@ void qxl_draw_fill(struct qxl_draw_fill *qxl_draw_fill_rec)
 	if (ret)
 		goto out_free_release;
 
+	mutex_lock(&release->async_release_mutex);
+
 	ret = make_drawable(qdev, 0, QXL_DRAW_FILL, &rect, release);
 	if (ret) {
 		qxl_release_backoff_reserve_list(release);
+		mutex_unlock(&release->async_release_mutex);
 		goto out_free_release;
 	}
 
@@ -479,6 +496,8 @@ void qxl_draw_fill(struct qxl_draw_fill *qxl_draw_fill_rec)
 	qxl_push_command_ring_release(qdev, release, QXL_CMD_DRAW, false);
 	qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
+
 out_free_release:
 	if (ret)
 		free_drawable(qdev, release);
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 01220d386b0a..cbc7bdb5b8d3 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -54,7 +54,7 @@
 
 #define DRIVER_NAME		"qxl"
 #define DRIVER_DESC		"RH QXL"
-#define DRIVER_DATE		"20120117"
+#define DRIVER_DATE		"20200107"
 
 #define DRIVER_MAJOR 0
 #define DRIVER_MINOR 1
@@ -172,6 +172,7 @@ struct qxl_release {
 	uint32_t surface_release_id;
 	struct ww_acquire_ctx ticket;
 	struct list_head bos;
+	struct mutex	async_release_mutex;
 };
 
 struct qxl_drm_chunk {
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 6cc9f3367fa0..189d016b871b 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -250,6 +250,8 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	if (ret)
 		goto out_free_bos;
 
+	mutex_lock(&release->async_release_mutex);
+
 	for (i = 0; i < cmd->relocs_num; ++i) {
 		if (reloc_info[i].type == QXL_RELOC_TYPE_BO)
 			apply_reloc(qdev, &reloc_info[i]);
@@ -263,6 +265,7 @@ static int qxl_process_single_command(struct qxl_device *qdev,
 	else
 		qxl_release_fence_buffer_objects(release);
 
+	mutex_unlock(&release->async_release_mutex);
 out_free_bos:
 out_free_release:
 	if (ret)
diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c
index e37f0097f744..f4c066dcabfe 100644
--- a/drivers/gpu/drm/qxl/qxl_release.c
+++ b/drivers/gpu/drm/qxl/qxl_release.c
@@ -133,6 +133,7 @@ qxl_release_alloc(struct qxl_device *qdev, int type,
 	release->type = type;
 	release->release_offset = 0;
 	release->surface_release_id = 0;
+	mutex_init(&release->async_release_mutex);
 	INIT_LIST_HEAD(&release->bos);
 
 	idr_preload(GFP_KERNEL);
@@ -343,6 +344,7 @@ int qxl_alloc_release_reserved(struct qxl_device *qdev, unsigned long size,
 	}
 
 	mutex_lock(&qdev->release_mutex);
+
 	if (qdev->current_release_bo_offset[cur_idx] + 1 >= releases_per_bo[cur_idx]) {
 		qxl_bo_unref(&qdev->current_release_bo[cur_idx]);
 		qdev->current_release_bo_offset[cur_idx] = 0;
-- 
2.20.1

Re: [PATCH 1/1] drm/qxl: add mutex_lock/mutex_unlock to ensure the order in which resources are released.

[Gerd Hoffmann]

On Sat, Apr 18, 2020 at 02:39:17PM +0800, Caicai wrote:
> When a qxl resource is released, the list that needs to be released is
> fetched from the linked list ring and cleared. When you empty the list,
> instead of trying to determine whether the ttm buffer object for each
> qxl in the list is locked, you release the qxl object and remove the
> element from the list until the list is empty. It was found that the
> linked list was cleared first, and that the lock on the corresponding
> ttm Bo for the QXL had not been released, so that the new qxl could not
> be locked when it used the TTM.

So the dma_resv_reserve_shared() call in qxl_release_validate_bo() is
unbalanced?  Because the dma_resv_unlock() call in
qxl_release_fence_buffer_objects() never happens due to
qxl_release_free_list() clearing the list beforehand?  Is that correct?

The only way I see for this to happen is that the guest is preempted
between qxl_push_{cursor,command}_ring_release() and
qxl_release_fence_buffer_objects() calls.  The host can complete the qxl
command then, signal the guest, and the IRQ handler calls
qxl_release_free_list() before qxl_release_fence_buffer_objects() runs.

Looking through the code I think it should be safe to simply swap the
qxl_release_fence_buffer_objects() +
qxl_push_{cursor,command}_ring_release() calls to close that race
window.  Can you try that and see if it fixes the bug for you?

>  		if (flush)
> -			flush_work(&qdev->gc_work);
> +			//can't flush work, it may lead to deadlock
> +			usleep_range(500, 1000);
> +

The commit message doesn't explain this chunk.

take care,
  Gerd

Re: [PATCH 1/1] drm/qxl: add mutex_lock/mutex_unlock to ensure the order in which resources are released

[Vasily Averin]

On 4/18/20 9:39 AM, Caicai wrote:
> When a qxl resource is released, the list that needs to be released is
> fetched from the linked list ring and cleared. When you empty the list,
> instead of trying to determine whether the ttm buffer object for each
> qxl in the list is locked, you release the qxl object and remove the
> element from the list until the list is empty. It was found that the
> linked list was cleared first, and that the lock on the corresponding
> ttm Bo for the QXL had not been released, so that the new qxl could not
> be locked when it used the TTM. By adding an additional mutex to ensure
> timing, qxl elements are not allowed to be removed from the list until
> ttm Bo's lock is released

> @@ -241,7 +243,11 @@ int qxl_garbage_collect(struct qxl_device *qdev)
>  			}
>  			id = next_id;
>  
> +			mutex_lock(&release->async_release_mutex);
> +
>  			qxl_release_free(qdev, release);
> +
> +			mutex_unlock(&release->async_release_mutex);
It does not work,
release was freed inside qxl_release_free() so you cannot access here any its fields

>  			++i;
>  		}
>  	}