Re: [PATCH v2 2/2] drm/xe: Add mutex locking to devcoredump

[John Harrison <john.c.harrison@intel.com>]

On 11/21/2024 15:44, Matthew Brost wrote:
> On Thu, Nov 21, 2024 at 02:55:42PM -0800, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> There are now multiple places that can trigger a coredump. Some of
>> which can happen in parallel. There is already a check against
>> capturing multiple dumps sequentially, but without locking it doesn't
>> guarantee to work against concurrent dumps. And if two dumps do happen
>> in parallel, they can end up doing Bad Things such as one call stack
>> freeing the data the other call stack is still processing. Which leads
>> to a crashed kernel.
>>
>> Further, it is possible for the DRM timeout to expire and trigger a
>> free of the capture while a user is still reading that capture out
>> through sysfs. Again leading to dodgy pointer problems.
>>
>> So, add a mutext lock around the capture, read and free functions to
>> prevent inteference.
>>
>> v2: Swap tiny scope spin_lock for larger scope mutex and fix
>> kernel-doc comment (review feedback from Matthe Brost)
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_devcoredump.c       | 26 +++++++++++++++++++++--
>>   drivers/gpu/drm/xe/xe_devcoredump_types.h |  4 +++-
>>   2 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
>> index dd48745a8a46..0621754ddfd2 100644
>> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
>> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
>> @@ -202,21 +202,29 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
>>   	if (!coredump)
>>   		return -ENODEV;
>>   
>> +	mutex_lock(&coredump->lock);
>> +
> I'll just explain my reclaim comment in the prior rev here.
>
> 'coredump->lock' is the path of reclaim as it can be called from the TDR
> which signals dma-fences. This is why most of the devcoredump core uses
> GFP_ATOMIC to capture smaller state which could lost quickly. We also
So the reason string allocation in patch #1 should also use GFP_ATOMIC 
rather than GFP_KERNEL?

> have worker, ss->work, which opportunisticly captures larger VM state /w
> GFP_KERNEL. The worker is not in the path reclaim. Thus you cannot flush
> the worker under 'coredump->lock' without getting potentail deadlocks.
> With proper annotations lockdep complain.
Okay, that makes sense now. Was forgetting the captures are from the TDR 
/ dma-fence paths which are reclaim requirements. Doh!

>
> e.g.
>
> We should do this on driver load:
>
> fs_reclaim_acquire();
> might_lock();
> fs_reclaim_recalim();
I assume this should be fs_reclaim_release()?

I see three separate instances of a local primelockdep() helper function 
to do this, two which do a might_lock() and one which does an actual 
lock/unlock (plus another which does a lock_map_acquire/release, but I 
assume that is very different). Plus another instance of the construct 
that is just inline with the rest of the init function. The helper 
versions all have a check against CONFIG_LOCKDEP but the unrolled 
version does not. Seems like we should have a generically accessible 
helper function for this? Maybe even as a wrapper around drmm_mutex_init 
itself? Although the xe_ggtt.c and xe_migrate.c copies are not using the 
drmm version of mutex init. Should they be?

John.

>
> Our upper layers should also but may have gaps. Reguardless, priming
> lockdep is a good practice and self-documenting.
>
>>   	ss = &coredump->snapshot;
>>   
>>   	/* Ensure delayed work is captured before continuing */
>>   	flush_work(&ss->work);
>>   
> So this is where the mutex should be locked.
>
>> -	if (!ss->read.buffer)
>> +	if (!ss->read.buffer) {
>> +		mutex_unlock(&coredump->lock);
>>   		return -ENODEV;
>> +	}
>>   
>> -	if (offset >= ss->read.size)
>> +	if (offset >= ss->read.size) {
>> +		mutex_unlock(&coredump->lock);
>>   		return 0;
>> +	}
>>   
>>   	byte_copied = count < ss->read.size - offset ? count :
>>   		ss->read.size - offset;
>>   	memcpy(buffer, ss->read.buffer + offset, byte_copied);
>>   
>> +	mutex_unlock(&coredump->lock);
>> +
>>   	return byte_copied;
>>   }
>>   
>> @@ -228,6 +236,8 @@ static void xe_devcoredump_free(void *data)
>>   	if (!data || !coredump_to_xe(coredump))
>>   		return;
>>   
>> +	mutex_lock(&coredump->lock);
>> +
>>   	cancel_work_sync(&coredump->snapshot.work);
>>   
> Likewise, lock the mutex here.
>
> Matt
>
>>   	xe_devcoredump_snapshot_free(&coredump->snapshot);
>> @@ -238,6 +248,8 @@ static void xe_devcoredump_free(void *data)
>>   	coredump->captured = false;
>>   	drm_info(&coredump_to_xe(coredump)->drm,
>>   		 "Xe device coredump has been deleted.\n");
>> +
>> +	mutex_unlock(&coredump->lock);
>>   }
>>   
>>   static void devcoredump_snapshot(struct xe_devcoredump *coredump,
>> @@ -312,8 +324,11 @@ void xe_devcoredump(struct xe_exec_queue *q, struct xe_sched_job *job, const cha
>>   	struct xe_devcoredump *coredump = &xe->devcoredump;
>>   	va_list varg;
>>   
>> +	mutex_lock(&coredump->lock);
>> +
>>   	if (coredump->captured) {
>>   		drm_dbg(&xe->drm, "Multiple hangs are occurring, but only the first snapshot was taken\n");
>> +		mutex_unlock(&coredump->lock);
>>   		return;
>>   	}
>>   
>> @@ -332,6 +347,7 @@ void xe_devcoredump(struct xe_exec_queue *q, struct xe_sched_job *job, const cha
>>   	dev_coredumpm_timeout(xe->drm.dev, THIS_MODULE, coredump, 0, GFP_KERNEL,
>>   			      xe_devcoredump_read, xe_devcoredump_free,
>>   			      XE_COREDUMP_TIMEOUT_JIFFIES);
>> +	mutex_unlock(&coredump->lock);
>>   }
>>   
>>   static void xe_driver_devcoredump_fini(void *arg)
>> @@ -343,6 +359,12 @@ static void xe_driver_devcoredump_fini(void *arg)
>>   
>>   int xe_devcoredump_init(struct xe_device *xe)
>>   {
>> +	int err;
>> +
>> +	err = drmm_mutex_init(&xe->drm, &xe->devcoredump.lock);
>> +	if (err)
>> +		return err;
>> +
>>   	return devm_add_action_or_reset(xe->drm.dev, xe_driver_devcoredump_fini, &xe->drm);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_devcoredump_types.h b/drivers/gpu/drm/xe/xe_devcoredump_types.h
>> index e6234e887102..1a1d16a96b2d 100644
>> --- a/drivers/gpu/drm/xe/xe_devcoredump_types.h
>> +++ b/drivers/gpu/drm/xe/xe_devcoredump_types.h
>> @@ -80,7 +80,9 @@ struct xe_devcoredump_snapshot {
>>    * for reading the information.
>>    */
>>   struct xe_devcoredump {
>> -	/** @captured: The snapshot of the first hang has already been taken. */
>> +	/** @lock: protects access to entire structure */
>> +	struct mutex lock;
>> +	/** @captured: The snapshot of the first hang has already been taken */
>>   	bool captured;
>>   	/** @snapshot: Snapshot is captured at time of the first crash */
>>   	struct xe_devcoredump_snapshot snapshot;
>> -- 
>> 2.47.0
>>