Re: [PATCH 3/4] drm/xe/devcoredump: Lock snap_mutex earlier

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Hey,

On 2024-03-04 15:05, José Roberto de Souza wrote:
> In case job gets signaled while snapshot capture is running this
> prevents UMD from unbind VMAs that needs to be captured.
How does this protect things exactly?

It could still have happened before devcoredump_snapshot is called. I 
think we should focus on protecting VM data only, if it happens to 
unbind, it could have happened before devcoredump_snapshot was ever 
called. All we can do its ensure that we dump without causing security 
issues, which the current code already does.

Cheers,
~Maarten

> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_devcoredump.c | 7 +++++++
>   drivers/gpu/drm/xe/xe_vm.c          | 2 --
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index 4ab0feca55cdd..24d27f1c9efc6 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -162,6 +162,9 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>   	int i;
>   	bool cookie;
>   
> +	if (q->vm)
> +		mutex_lock(&q->vm->snap_mutex);
> +
>   	ss->snapshot_time = ktime_get_real();
>   	ss->boot_time = ktime_get_boottime();
>   
> @@ -197,6 +200,10 @@ static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>   	queue_work(system_unbound_wq, &ss->work);
>   
>   	xe_force_wake_put(gt_to_fw(q->gt), XE_FORCEWAKE_ALL);
> +
> +	if (q->vm)
> +		mutex_unlock(&q->vm->snap_mutex);
> +
>   	dma_fence_end_signalling(cookie);
>   }
>   
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f7d20bf9b33a9..544907b09289a 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3328,7 +3328,6 @@ struct xe_vm_snapshot *xe_vm_snapshot_capture(struct xe_vm *vm)
>   	if (!vm)
>   		return NULL;
>   
> -	mutex_lock(&vm->snap_mutex);
>   	drm_gpuvm_for_each_va(gpuva, &vm->gpuvm) {
>   		if (gpuva->flags & XE_VMA_DUMPABLE)
>   			num_snaps++;
> @@ -3373,7 +3372,6 @@ struct xe_vm_snapshot *xe_vm_snapshot_capture(struct xe_vm *vm)
>   	}
>   
>   out_unlock:
> -	mutex_unlock(&vm->snap_mutex);
>   	return snap;
>   }
>