[Intel-xe] [PATCH] drm/xe: Get the engine ref while holding a lock

[Mika Kuoppala]

Thomas Hellström <thomas.hellstrom at linux.intel.com> writes:

> Hi, Mika,
>
> There is a reviewed patch on the list that fixes half of this already,
>
> "Fix engine lookup refcount race", but I'm holding off commiting because 
> we need to sort out where to
> push fixes in response to Oded's review.
>
> But since your patch is more complete, we can use that instead.

Oh did not notice. I bumped into uaf on engine teardown
when running eudebug tests and while hunting that noticed
that the lookup might let unreffed pointer escape.

>
> One comment below, though. Otherwise LGTM.
>
> /Thomas
>
>
> On 6/2/23 17:21, Mika Kuoppala wrote:
>> The engine xarray holds a ref to engine, guarded by the lock.
>> While we do lookup for engine, we need to take the ref inside
>> the lock to prevent potential use-after-free during
>> ioctls.
>>
>> Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_engine.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
>> index b3036c4a8ec3..ec5a79c9f2a3 100644
>> --- a/drivers/gpu/drm/xe/xe_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_engine.c
>> @@ -161,10 +161,9 @@ struct xe_engine *xe_engine_lookup(struct xe_file *xef, u32 id)
>>   
>>   	mutex_lock(&xef->engine.lock);
>>   	e = xa_load(&xef->engine.xa, id);
>> -	mutex_unlock(&xef->engine.lock);
>> -
>> -	if (e)
>> +	if (likely(e))
>>   		xe_engine_get(e);
>
> Old rule is the branch prediction hints shouldn't be used in drivers, 
> but exceptions might be
> in place where a code path is hit so often it might actually make a 
> difference. Not sure this is such a path though?
>

I peeked that submission path does lookup engine and added this
as in addition to ioctls. But that path seems to be once per submission
so cant argue that it is often enough. Will remove.

-Mika

>> +	mutex_unlock(&xef->engine.lock);
>>   
>>   	return e;
>>   }
>> @@ -641,26 +640,27 @@ int xe_engine_get_property_ioctl(struct drm_device *dev, void *data,
>>   	struct xe_file *xef = to_xe_file(file);
>>   	struct drm_xe_engine_get_property *args = data;
>>   	struct xe_engine *e;
>> +	int ret;
>>   
>>   	if (XE_IOCTL_ERR(xe, args->reserved[0] || args->reserved[1]))
>>   		return -EINVAL;
>>   
>> -	mutex_lock(&xef->engine.lock);
>> -	e = xa_load(&xef->engine.xa, args->engine_id);
>> -	mutex_unlock(&xef->engine.lock);
>> -
>> +	e = xe_engine_lookup(xef, args->engine_id);
>>   	if (XE_IOCTL_ERR(xe, !e))
>>   		return -ENOENT;
>>   
>>   	switch (args->property) {
>>   	case XE_ENGINE_GET_PROPERTY_BAN:
>>   		args->value = !!(e->flags & ENGINE_FLAG_BANNED);
>> +		ret = 0;
>>   		break;
>>   	default:
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>>   	}
>>   
>> -	return 0;
>> +	xe_engine_put(e);
>> +
>> +	return ret;
>>   }
>>   
>>   static void engine_kill_compute(struct xe_engine *e)