From 58a320186d1ee1a9a3e03b4ae57da10d88361d00 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Jos=C3=A9=20Roberto=20de=20Souza?= Date: Mon, 23 Jan 2023 12:09:56 -0800 Subject: [PATCH] intel/ds: Fix crash when allocating more intel_ds_queues than u_vector was initialized MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit u_vector_add() don't keep the returned pointers valid. After the initial size allocated in u_vector_init() is reached it will allocate a bigger buffer and copy data from older buffer to the new one and free the old buffer, making all the previous pointers returned by u_vector_add() invalid and crashing the application when trying to access it. This is reproduced when running dEQP-VK.synchronization.signal_order.timeline_semaphore.* in DG2 SKUs that has 4 CCS engines, INTEL_COMPUTE_CLASS=1 is set and of course perfetto build is enabled. To fix this issue here I'm moving the storage/allocation of struct intel_ds_queue to struct anv_queue/iris_batch and using struct list_head to maintain a chain of intel_ds_queue of the intel_ds_device. This allows us to append or remove queues dynamically in future if necessary. Fixes: e760c5b37be9 ("anv: add perfetto source") Reviewed-by: Lionel Landwerlin Signed-off-by: José Roberto de Souza Part-of: (cherry picked from commit 8092bc2158ebb8a5f85e0ec569387c5dcd0d1627) --- .pick_status.json | 2 +- src/gallium/drivers/iris/iris_batch.c | 6 +++--- src/gallium/drivers/iris/iris_batch.h | 2 +- src/gallium/drivers/iris/iris_utrace.c | 7 +++---- src/intel/ds/intel_driver_ds.cc | 20 +++++++++----------- src/intel/ds/intel_driver_ds.h | 12 ++++++++---- src/intel/vulkan/anv_batch_chain.c | 4 ++-- src/intel/vulkan/anv_private.h | 2 +- src/intel/vulkan/anv_utrace.c | 5 ++--- src/intel/vulkan_hasvk/anv_batch_chain.c | 4 ++-- src/intel/vulkan_hasvk/anv_private.h | 2 +- src/intel/vulkan_hasvk/anv_utrace.c | 5 ++--- 12 files changed, 35 insertions(+), 36 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 42ac2af..52f1a4e 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -3568,7 +3568,7 @@ "description": "intel/ds: Fix crash when allocating more intel_ds_queues than u_vector was initialized", "nominated": true, "nomination_type": 1, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "e760c5b37be938427a9c88182ea99f7f66721ca3" }, diff --git a/src/gallium/drivers/iris/iris_batch.c b/src/gallium/drivers/iris/iris_batch.c index ec32d88..d598fd7 100644 --- a/src/gallium/drivers/iris/iris_batch.c +++ b/src/gallium/drivers/iris/iris_batch.c @@ -1053,10 +1053,10 @@ _iris_batch_flush(struct iris_batch *batch, const char *file, int line) } - uint64_t start_ts = intel_ds_begin_submit(batch->ds); - uint64_t submission_id = batch->ds->submission_id; + uint64_t start_ts = intel_ds_begin_submit(&batch->ds); + uint64_t submission_id = batch->ds.submission_id; int ret = submit_batch(batch); - intel_ds_end_submit(batch->ds, start_ts); + intel_ds_end_submit(&batch->ds, start_ts); /* When batch submission fails, our end-of-batch syncobj remains * unsignalled, and in fact is not even considered submitted. diff --git a/src/gallium/drivers/iris/iris_batch.h b/src/gallium/drivers/iris/iris_batch.h index a1dfa6e..437352a 100644 --- a/src/gallium/drivers/iris/iris_batch.h +++ b/src/gallium/drivers/iris/iris_batch.h @@ -197,7 +197,7 @@ struct iris_batch { struct u_trace trace; /** Batch wrapper structure for perfetto */ - struct intel_ds_queue *ds; + struct intel_ds_queue ds; }; void iris_init_batches(struct iris_context *ice, int priority); diff --git a/src/gallium/drivers/iris/iris_utrace.c b/src/gallium/drivers/iris/iris_utrace.c index 7f49826..e66a560 100644 --- a/src/gallium/drivers/iris/iris_utrace.c +++ b/src/gallium/drivers/iris/iris_utrace.c @@ -95,7 +95,7 @@ iris_utrace_delete_flush_data(struct u_trace_context *utctx, void iris_utrace_flush(struct iris_batch *batch, uint64_t submission_id) { struct intel_ds_flush_data *flush_data = malloc(sizeof(*flush_data)); - intel_ds_flush_data_init(flush_data, batch->ds, submission_id); + intel_ds_flush_data_init(flush_data, &batch->ds, submission_id); u_trace_flush(&batch->trace, flush_data, false); } @@ -122,9 +122,8 @@ void iris_utrace_init(struct iris_context *ice) iris_utrace_delete_flush_data); for (int i = 0; i < IRIS_BATCH_COUNT; i++) { - ice->batches[i].ds = - intel_ds_device_add_queue(&ice->ds, "%s", - iris_batch_name_to_string(i)); + intel_ds_device_init_queue(&ice->ds, &ice->batches[i].ds, "%s", + iris_batch_name_to_string(i)); } } diff --git a/src/intel/ds/intel_driver_ds.cc b/src/intel/ds/intel_driver_ds.cc index 516be6d..dc51faa 100644 --- a/src/intel/ds/intel_driver_ds.cc +++ b/src/intel/ds/intel_driver_ds.cc @@ -185,12 +185,10 @@ static void send_descriptors(IntelRenderpassDataSource::TraceContext &ctx, struct intel_ds_device *device) { - struct intel_ds_queue *queue; - PERFETTO_LOG("Sending renderstage descriptors"); device->event_id = 0; - u_vector_foreach(queue, &device->queues) { + list_for_each_entry_safe(struct intel_ds_queue, queue, &device->queues, link) { for (uint32_t s = 0; s < ARRAY_SIZE(queue->stages); s++) { queue->stages[s].start_ns = 0; } @@ -222,7 +220,7 @@ send_descriptors(IntelRenderpassDataSource::TraceContext &ctx, } /* Emit all the IID picked at device/queue creation. */ - u_vector_foreach(queue, &device->queues) { + list_for_each_entry_safe(struct intel_ds_queue, queue, &device->queues, link) { for (unsigned s = 0; s < INTEL_DS_QUEUE_STAGE_N_STAGES; s++) { { /* We put the stage number in there so that all rows are order @@ -528,23 +526,21 @@ intel_ds_device_init(struct intel_ds_device *device, device->info = *devinfo; device->iid = get_iid(); device->api = api; - u_vector_init(&device->queues, 4, sizeof(struct intel_ds_queue)); + list_inithead(&device->queues); } void intel_ds_device_fini(struct intel_ds_device *device) { u_trace_context_fini(&device->trace_context); - u_vector_finish(&device->queues); } struct intel_ds_queue * -intel_ds_device_add_queue(struct intel_ds_device *device, - const char *fmt_name, - ...) +intel_ds_device_init_queue(struct intel_ds_device *device, + struct intel_ds_queue *queue, + const char *fmt_name, + ...) { - struct intel_ds_queue *queue = - (struct intel_ds_queue *) u_vector_add(&device->queues); va_list ap; memset(queue, 0, sizeof(*queue)); @@ -560,6 +556,8 @@ intel_ds_device_add_queue(struct intel_ds_device *device, queue->stages[s].stage_iid = get_iid(); } + list_add(&queue->link, &device->queues); + return queue; } diff --git a/src/intel/ds/intel_driver_ds.h b/src/intel/ds/intel_driver_ds.h index b5e5091..f88f5f7 100644 --- a/src/intel/ds/intel_driver_ds.h +++ b/src/intel/ds/intel_driver_ds.h @@ -107,7 +107,7 @@ struct intel_ds_device { struct u_trace_context trace_context; /* List of intel_ds_queue */ - struct u_vector queues; + struct list_head queues; }; struct intel_ds_stage { @@ -122,6 +122,8 @@ struct intel_ds_stage { }; struct intel_ds_queue { + struct list_head link; + /* Device this queue belongs to */ struct intel_ds_device *device; @@ -155,9 +157,11 @@ void intel_ds_device_init(struct intel_ds_device *device, enum intel_ds_api api); void intel_ds_device_fini(struct intel_ds_device *device); -struct intel_ds_queue *intel_ds_device_add_queue(struct intel_ds_device *device, - const char *fmt_name, - ...); +struct intel_ds_queue * +intel_ds_device_init_queue(struct intel_ds_device *device, + struct intel_ds_queue *queue, + const char *fmt_name, + ...); void intel_ds_flush_data_init(struct intel_ds_flush_data *data, struct intel_ds_queue *queue, diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/anv_batch_chain.c index 3fd06fd..cf890bd 100644 --- a/src/intel/vulkan/anv_batch_chain.c +++ b/src/intel/vulkan/anv_batch_chain.c @@ -1991,14 +1991,14 @@ anv_queue_submit(struct vk_queue *vk_queue, return VK_SUCCESS; } - uint64_t start_ts = intel_ds_begin_submit(queue->ds); + uint64_t start_ts = intel_ds_begin_submit(&queue->ds); pthread_mutex_lock(&device->mutex); result = anv_queue_submit_locked(queue, submit); /* Take submission ID under lock */ pthread_mutex_unlock(&device->mutex); - intel_ds_end_submit(queue->ds, start_ts); + intel_ds_end_submit(&queue->ds, start_ts); return result; } diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h index 01e1a46..0b2e428 100644 --- a/src/intel/vulkan/anv_private.h +++ b/src/intel/vulkan/anv_private.h @@ -1102,7 +1102,7 @@ struct anv_queue { /** Synchronization object for debug purposes (DEBUG_SYNC) */ struct vk_sync *sync; - struct intel_ds_queue * ds; + struct intel_ds_queue ds; }; struct nir_xfb_info; diff --git a/src/intel/vulkan/anv_utrace.c b/src/intel/vulkan/anv_utrace.c index 35a744d..3a35aef 100644 --- a/src/intel/vulkan/anv_utrace.c +++ b/src/intel/vulkan/anv_utrace.c @@ -111,7 +111,7 @@ anv_device_utrace_flush_cmd_buffers(struct anv_queue *queue, if (!flush) return vk_error(device, VK_ERROR_OUT_OF_HOST_MEMORY); - intel_ds_flush_data_init(&flush->ds, queue->ds, queue->ds->submission_id); + intel_ds_flush_data_init(&flush->ds, &queue->ds, queue->ds.submission_id); result = vk_sync_create(&device->vk, &device->physical->sync_syncobj_type, 0, 0, &flush->sync); @@ -284,8 +284,7 @@ anv_device_utrace_init(struct anv_device *device) for (uint32_t q = 0; q < device->queue_count; q++) { struct anv_queue *queue = &device->queues[q]; - queue->ds = - intel_ds_device_add_queue(&device->ds, "%s%u", + intel_ds_device_init_queue(&device->ds, &queue->ds, "%s%u", intel_engines_class_to_string(queue->family->engine_class), queue->index_in_family); } diff --git a/src/intel/vulkan_hasvk/anv_batch_chain.c b/src/intel/vulkan_hasvk/anv_batch_chain.c index 1a14a34..459f0df 100644 --- a/src/intel/vulkan_hasvk/anv_batch_chain.c +++ b/src/intel/vulkan_hasvk/anv_batch_chain.c @@ -2393,14 +2393,14 @@ anv_queue_submit(struct vk_queue *vk_queue, return VK_SUCCESS; } - uint64_t start_ts = intel_ds_begin_submit(queue->ds); + uint64_t start_ts = intel_ds_begin_submit(&queue->ds); pthread_mutex_lock(&device->mutex); result = anv_queue_submit_locked(queue, submit); /* Take submission ID under lock */ pthread_mutex_unlock(&device->mutex); - intel_ds_end_submit(queue->ds, start_ts); + intel_ds_end_submit(&queue->ds, start_ts); return result; } diff --git a/src/intel/vulkan_hasvk/anv_private.h b/src/intel/vulkan_hasvk/anv_private.h index 0367cef..6116afb 100644 --- a/src/intel/vulkan_hasvk/anv_private.h +++ b/src/intel/vulkan_hasvk/anv_private.h @@ -1074,7 +1074,7 @@ struct anv_queue { /** Synchronization object for debug purposes (DEBUG_SYNC) */ struct vk_sync *sync; - struct intel_ds_queue * ds; + struct intel_ds_queue ds; }; struct nir_xfb_info; diff --git a/src/intel/vulkan_hasvk/anv_utrace.c b/src/intel/vulkan_hasvk/anv_utrace.c index 35a744d..3a35aef 100644 --- a/src/intel/vulkan_hasvk/anv_utrace.c +++ b/src/intel/vulkan_hasvk/anv_utrace.c @@ -111,7 +111,7 @@ anv_device_utrace_flush_cmd_buffers(struct anv_queue *queue, if (!flush) return vk_error(device, VK_ERROR_OUT_OF_HOST_MEMORY); - intel_ds_flush_data_init(&flush->ds, queue->ds, queue->ds->submission_id); + intel_ds_flush_data_init(&flush->ds, &queue->ds, queue->ds.submission_id); result = vk_sync_create(&device->vk, &device->physical->sync_syncobj_type, 0, 0, &flush->sync); @@ -284,8 +284,7 @@ anv_device_utrace_init(struct anv_device *device) for (uint32_t q = 0; q < device->queue_count; q++) { struct anv_queue *queue = &device->queues[q]; - queue->ds = - intel_ds_device_add_queue(&device->ds, "%s%u", + intel_ds_device_init_queue(&device->ds, &queue->ds, "%s%u", intel_engines_class_to_string(queue->family->engine_class), queue->index_in_family); } -- 2.7.4