Re: [PATCH v15 01/19] drm/etnaviv: Implement drm_gem_object_funcs::print_info()

[Sui Jingfeng <sui.jingfeng@linux.dev>]

Hi,

On 2024/10/1 21:04, Lucas Stach wrote:
> Hi Sui,
>
> Am Sonntag, dem 08.09.2024 um 17:43 +0800 schrieb Sui Jingfeng:
>> It will be called by drm_gem_print_info() if implemented, and it can
>> provide more information about the framebuffer objects.
> Etnaviv GEM BOs are not framebuffer objects.
>
>> Signed-off-by: Sui Jingfeng <sui.jingfeng@linux.dev>
>> ---
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 32 +++++++++++++++++++++++++++
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.h |  2 +-
>>   2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index 4247a10f8d4f..543d881585b3 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -534,8 +534,40 @@ static const struct vm_operations_struct vm_ops = {
>>   	.close = drm_gem_vm_close,
>>   };
>>   
>> +static const char *etnaviv_gem_obj_caching_info(u32 flags)
>> +{
>> +	switch (flags & ETNA_BO_CACHE_MASK) {
>> +	case ETNA_BO_CACHED:
>> +		return "cached";
>> +	case ETNA_BO_UNCACHED:
>> +		return "uncached";
>> +	case ETNA_BO_WC:
>> +		return "write-combine";
> "write-combined" to be consistent with the other two.


OK,


>> +	default:
>> +		break;
>> +	}
>> +
>> +	return "unknown";
>> +}
>> +
>> +static void etnaviv_gem_object_info(struct drm_printer *p,
>> +				    unsigned int indent,
>> +				    const struct drm_gem_object *obj)
>> +{
>> +	const struct etnaviv_gem_object *etnaviv_obj;
>> +
>> +	etnaviv_obj = container_of(obj, struct etnaviv_gem_object, base);
>> +
>> +	drm_printf_indent(p, indent, "caching mode=%s\n",
>> +			  etnaviv_gem_obj_caching_info(etnaviv_obj->flags));
>> +	drm_printf_indent(p, indent, "active=%s\n",
>> +			  str_yes_no(is_active(etnaviv_obj)));
>> +	drm_printf_indent(p, indent, "vaddr=%p\n", etnaviv_obj->vaddr);
> Why should we expose the vaddr to userspace? I don't see why this would
> be relevant even as debug info and it leaks the kernel vmap area
> address, which could be abused to facilitate kernel exploits.


This is nearly a re-implement for the etnaviv_gem_describe(),

It's not intend to leak, but to give us a *hint* that
if a specific buffer object have been VMAP-ed and
we will know how many BOs have been vmap-ed.


> Regards,
> Lucas
>
>> +}
>> +
>>   static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
>>   	.free = etnaviv_gem_free_object,
>> +	.print_info = etnaviv_gem_object_info,
>>   	.pin = etnaviv_gem_prime_pin,
>>   	.unpin = etnaviv_gem_prime_unpin,
>>   	.get_sg_table = etnaviv_gem_prime_get_sg_table,
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> index a42d260cac2c..3f8fe19a77cc 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h
>> @@ -68,7 +68,7 @@ struct etnaviv_gem_ops {
>>   	int (*mmap)(struct etnaviv_gem_object *, struct vm_area_struct *);
>>   };
>>   
>> -static inline bool is_active(struct etnaviv_gem_object *etnaviv_obj)
>> +static inline bool is_active(const struct etnaviv_gem_object *etnaviv_obj)
>>   {
>>   	return atomic_read(&etnaviv_obj->gpu_active) != 0;
>>   }

-- 
Best regards,
Sui