linux-kernel - Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

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, 27 Jan 2022 08:59:36 +0100
From:   Christian König <christian.koenig@....com>
To:     Matthew Brost <matthew.brost@...el.com>
Cc:     Lucas De Marchi <lucas.demarchi@...el.com>,
        intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linaro-mm-sig@...ts.linaro.org,
        linux-media@...r.kernel.org
Subject: Re: [PATCH 01/19] dma-buf-map: Add read/write helpers

Am 27.01.22 um 08:36 schrieb Matthew Brost:
> [SNIP]
>>>    /**
>>>     * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
>>>     * @dst:	The dma-buf mapping structure
>>> @@ -263,4 +304,44 @@ static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
>>>    		map->vaddr += incr;
>>>    }
>>> +/**
>>> + * dma_buf_map_read_field - Read struct member from dma-buf mapping with
>>> + * arbitrary size and handling un-aligned accesses
>>> + *
>>> + * @map__:	The dma-buf mapping structure
>>> + * @type__:	The struct to be used containing the field to read
>>> + * @field__:	Member from struct we want to read
>>> + *
>>> + * Read a value from dma-buf mapping calculating the offset and size: this assumes
>>> + * the dma-buf mapping is aligned with a a struct type__. A single u8, u16, u32
>>> + * or u64 can be read, based on the offset and size of type__.field__.
>>> + */
>>> +#define dma_buf_map_read_field(map__, type__, field__) ({				\
>>> +	type__ *t__;									\
>>> +	typeof(t__->field__) val__;							\
>>> +	dma_buf_map_memcpy_from_offset(&val__, map__, offsetof(type__, field__),	\
>>> +				       sizeof(t__->field__));				\
>>> +	val__;										\
>>> +})
>>> +
>>> +/**
>>> + * dma_buf_map_write_field - Write struct member to the dma-buf mapping with
>>> + * arbitrary size and handling un-aligned accesses
>>> + *
>>> + * @map__:	The dma-buf mapping structure
>>> + * @type__:	The struct to be used containing the field to write
>>> + * @field__:	Member from struct we want to write
>>> + * @val__:	Value to be written
>>> + *
>>> + * Write a value to the dma-buf mapping calculating the offset and size.
>>> + * A single u8, u16, u32 or u64 can be written based on the offset and size of
>>> + * type__.field__.
>>> + */
>>> +#define dma_buf_map_write_field(map__, type__, field__, val__) ({			\
>>> +	type__ *t__;									\
>>> +	typeof(t__->field__) val____ = val__;						\
>>> +	dma_buf_map_memcpy_to_offset(map__, offsetof(type__, field__),			\
>>> +				     &val____, sizeof(t__->field__));			\
>>> +})
>>> +
>> Uff well that absolutely looks like overkill to me.
>>
> Hold on...
>
>> That's a rather special use case as far as I can see and I think we should
>> only have this in the common framework if more than one driver is using it.
>>
> I disagree, this is rather elegant.
>
> The i915 can't be the *only* driver that defines a struct which
> describes the layout of a dma_buf object.

That's not the problem, amdgpu as well as nouveau are doing that as 
well. The problem is DMA-buf is a buffer sharing framework between drivers.

In other words which importer is supposed to use this with a DMA-buf 
exported by another device?

> IMO this base macro allows *all* other drivers to build on this write
> directly to fields in structures those drivers have defined.

Exactly that's the point. This is something drivers should absolutely 
*NOT* do.

That are driver internals and it is extremely questionable to move this 
into the common framework.

Regards,
Christian.

>   Patches
> later in this series do this for the GuC ads.
>
> Matt
>   
>> Regards,
>> Christian.
>>
>>>    #endif /* __DMA_BUF_MAP_H__ */