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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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.

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@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?

> +	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)