[PATCH] drm/ttm: prevent grabbing page references

[PATCH] drm/ttm: prevent grabbing page references

[Christian König]

TTM owns the pages it uses for backing buffer objects with system
memory. Because of this it is absolutely illegal to mess around with
the reference count of those pages.

So make sure that nobody ever tries to grab an extra reference on
pages allocated through the page pool.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_pool.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
index 1bba0a0ed3f9..cbca84dbd83f 100644
--- a/drivers/gpu/drm/ttm/ttm_pool.c
+++ b/drivers/gpu/drm/ttm/ttm_pool.c
@@ -93,8 +93,17 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
 
 	if (!pool->use_dma_alloc) {
 		p = alloc_pages(gfp_flags, order);
-		if (p)
+		if (p) {
 			p->private = order;
+
+			/* The pages are fully owned by TTM and because of this
+			 * it's illegal to grab extra references to it or
+			 * otherwise we corrupt TTMs internal state. Make sure
+			 * nobody tries to ever increase the reference count of
+			 * those pages.
+			 */
+			set_page_count(p, 0);
+		}
 		return p;
 	}
 
@@ -144,6 +153,9 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
 #endif
 
 	if (!pool || !pool->use_dma_alloc) {
+		/* See alloc why references to TTMs pages are illegal */
+		WARN_ON(page_count(p));
+		set_page_count(p, 1);
 		__free_pages(p, order);
 		return;
 	}
-- 
2.25.1

Re: [PATCH] drm/ttm: prevent grabbing page references

[Dmitry Osipenko]

On 8/15/22 16:08, Christian König wrote:
> TTM owns the pages it uses for backing buffer objects with system
> memory. Because of this it is absolutely illegal to mess around with
> the reference count of those pages.
> 
> So make sure that nobody ever tries to grab an extra reference on
> pages allocated through the page pool.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_pool.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index 1bba0a0ed3f9..cbca84dbd83f 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -93,8 +93,17 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
>  
>  	if (!pool->use_dma_alloc) {
>  		p = alloc_pages(gfp_flags, order);
> -		if (p)
> +		if (p) {
>  			p->private = order;
> +
> +			/* The pages are fully owned by TTM and because of this
> +			 * it's illegal to grab extra references to it or
> +			 * otherwise we corrupt TTMs internal state. Make sure
> +			 * nobody tries to ever increase the reference count of
> +			 * those pages.
> +			 */
> +			set_page_count(p, 0);
> +		}
>  		return p;
>  	}
>  
> @@ -144,6 +153,9 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
>  #endif
>  
>  	if (!pool || !pool->use_dma_alloc) {
> +		/* See alloc why references to TTMs pages are illegal */
> +		WARN_ON(page_count(p));
> +		set_page_count(p, 1);
>  		__free_pages(p, order);
>  		return;
>  	}

Could you please explain why you're skipping the DMA pages?

-- 
Best regards,
Dmitry

Re: [PATCH] drm/ttm: prevent grabbing page references

[Christian König]

Am 18.08.22 um 00:20 schrieb Dmitry Osipenko:
> On 8/15/22 16:08, Christian König wrote:
>> TTM owns the pages it uses for backing buffer objects with system
>> memory. Because of this it is absolutely illegal to mess around with
>> the reference count of those pages.
>>
>> So make sure that nobody ever tries to grab an extra reference on
>> pages allocated through the page pool.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_pool.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
>> index 1bba0a0ed3f9..cbca84dbd83f 100644
>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>> @@ -93,8 +93,17 @@ static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t gfp_flags,
>>   
>>   	if (!pool->use_dma_alloc) {
>>   		p = alloc_pages(gfp_flags, order);
>> -		if (p)
>> +		if (p) {
>>   			p->private = order;
>> +
>> +			/* The pages are fully owned by TTM and because of this
>> +			 * it's illegal to grab extra references to it or
>> +			 * otherwise we corrupt TTMs internal state. Make sure
>> +			 * nobody tries to ever increase the reference count of
>> +			 * those pages.
>> +			 */
>> +			set_page_count(p, 0);
>> +		}
>>   		return p;
>>   	}
>>   
>> @@ -144,6 +153,9 @@ static void ttm_pool_free_page(struct ttm_pool *pool, enum ttm_caching caching,
>>   #endif
>>   
>>   	if (!pool || !pool->use_dma_alloc) {
>> +		/* See alloc why references to TTMs pages are illegal */
>> +		WARN_ON(page_count(p));
>> +		set_page_count(p, 1);
>>   		__free_pages(p, order);
>>   		return;
>>   	}
> Could you please explain why you're skipping the DMA pages?

Lack of sleep :)

Going to fix that, thanks.
Christian.

>