[PATCH 1/5] drm/prime: fix potential race in drm_gem_map_detach

[Christian König]

Am 28.02.2018 um 10:48 schrieb Lucas Stach:
> Hi Christian,
>
> Am Dienstag, den 27.02.2018, 12:49 +0100 schrieb Christian König:
>> Unpin the GEM object only after freeing the sg table.
> What is the race that is being fixed here? The SG table is private to
> the importer and the importer should hopefully only call map_detach if
> it is done with all operations using the SG table. Thus it shouldn't
> matter that the SG table might point to moved pages during execution of
> this function.

Exactly, it shouldn't matter. This is just a precaution.

When the device driver is buggy I want proper error messages from IOMMU 
and not accessing pages which might already be reused for something else.

Regards,
Christian.

>
> Regards,
> Lucas
>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/drm_prime.c | 32 ++++++++++++++++----------------
>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c
>> b/drivers/gpu/drm/drm_prime.c
>> index e82a976f0fba..c38dacda6119 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -230,26 +230,26 @@ void drm_gem_map_detach(struct dma_buf
>> *dma_buf,
>>   	struct drm_prime_attachment *prime_attach = attach->priv;
>>   	struct drm_gem_object *obj = dma_buf->priv;
>>   	struct drm_device *dev = obj->dev;
>> -	struct sg_table *sgt;
>>   
>> -	if (dev->driver->gem_prime_unpin)
>> -		dev->driver->gem_prime_unpin(obj);
>> +	if (prime_attach) {
>> +		struct sg_table *sgt = prime_attach->sgt;
>>   
>> -	if (!prime_attach)
>> -		return;
>> -
>> -	sgt = prime_attach->sgt;
>> -	if (sgt) {
>> -		if (prime_attach->dir != DMA_NONE)
>> -			dma_unmap_sg_attrs(attach->dev, sgt->sgl,
>> sgt->nents,
>> -					   prime_attach->dir,
>> -					   DMA_ATTR_SKIP_CPU_SYNC);
>> -		sg_free_table(sgt);
>> +		if (sgt) {
>> +			if (prime_attach->dir != DMA_NONE)
>> +				dma_unmap_sg_attrs(attach->dev, sgt-
>>> sgl,
>> +						   sgt->nents,
>> +						   prime_attach-
>>> dir,
>> +						   DMA_ATTR_SKIP_CPU
>> _SYNC);
>> +			sg_free_table(sgt);
>> +		}
>> +
>> +		kfree(sgt);
>> +		kfree(prime_attach);
>> +		attach->priv = NULL;
>>   	}
>>   
>> -	kfree(sgt);
>> -	kfree(prime_attach);
>> -	attach->priv = NULL;
>> +	if (dev->driver->gem_prime_unpin)
>> +		dev->driver->gem_prime_unpin(obj);
>>   }
>>   EXPORT_SYMBOL(drm_gem_map_detach);
>>