linux-kernel - Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 23 Nov 2023 01:04:56 +0300
From:   Dmitry Osipenko <dmitry.osipenko@...labora.com>
To:     Boris Brezillon <boris.brezillon@...labora.com>
Cc:     David Airlie <airlied@...il.com>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Christian König <christian.koenig@....com>,
        Qiang Yu <yuq825@...il.com>,
        Steven Price <steven.price@....com>,
        Emma Anholt <emma@...olt.net>, Melissa Wen <mwen@...lia.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v18 15/26] drm/panfrost: Explicitly get and put drm-shmem
 pages

On 11/10/23 13:53, Boris Brezillon wrote:
> Hm, there was no drm_gem_shmem_get_pages_sgt() call here, why should we
> add a drm_gem_shmem_get_pages()? What we should do instead is add a
> drm_gem_shmem_get_pages() for each drm_gem_shmem_get_pages_sgt() we
> have in the driver (in panfrost_mmu_map()), and add
> drm_gem_shmem_put_pages() calls where they are missing
> (panfrost_mmu_unmap()).
> 
>> +		if (err)
>> +			goto err_free;
>> +	}
>> +
>>  	return bo;
>> +
>> +err_free:
>> +	drm_gem_shmem_free(&bo->base);
>> +
>> +	return ERR_PTR(err);
>>  }
>>  
>>  struct drm_gem_object *
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index 770dab1942c2..ac145a98377b 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -504,7 +504,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>>  		if (IS_ERR(pages[i])) {
>>  			ret = PTR_ERR(pages[i]);
>>  			pages[i] = NULL;
>> -			goto err_pages;
>> +			goto err_unlock;
>>  		}
>>  	}
>>  
>> @@ -512,7 +512,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>>  	ret = sg_alloc_table_from_pages(sgt, pages + page_offset,
>>  					NUM_FAULT_PAGES, 0, SZ_2M, GFP_KERNEL);
>>  	if (ret)
>> -		goto err_pages;
>> +		goto err_unlock;
> Feels like the panfrost_gem_mapping object should hold a ref on the BO
> pages, not the BO itself, because, ultimately, the user of the BO is
> the GPU. This matches what I was saying about moving get/put_pages() to
> panfrost_mmu_map/unmap(): everytime a panfrost_gem_mapping becomes
> active, to want to take a pages ref, every time it becomes inactive,
> you should release the pages ref.

The panfrost_mmu_unmap() is also used by shrinker when BO is purged. I'm
unhappy with how icky it all becomes if unmap is made to put pages.

Previously map() was implicitly allocating pages with get_sgt() and then
pages were implicitly released by drm_gem_shmem_free(). A non-heap BO is
mapped when it's created by Panfrost, hence the actual lifetime of pages
is kept unchanged by this patch. The implicit allocation is turned into
explicit one, i.e. pages are explicitly allocated before BO is mapped.

-- 
Best regards,
Dmitry