Re: [PATCH v2 0/3] udmabuf: Sync to attached devices

["Christian König" <christian.koenig@amd.com>]

On 15.08.25 21:40, Andrew Davis wrote:
> On 8/15/25 4:41 AM, Christian König wrote:
>> On 14.08.25 18:10, Andrew Davis wrote:
>>> Hello all,
>>>
>>> This series makes it so the udmabuf will sync the backing buffer
>>> with the set of attached devices as required for DMA-BUFs when
>>> doing {begin,end}_cpu_access.
>>
>> Yeah the reason why we didn't do that is that this doesn't even work 100% reliable in theory. So this patchset here might make your use case work but is a bit questionable in general.
>>
>> udmabuf is about turning a file descriptor created by memfd_create() into a DMA-buf. Mapping that memory can happen through the memfd as well and so it is perfectly valid to skip the DMA-buf begin_access and end_access callbacks.
>>
> 
> If someone maps the memory backed by the DMA-buf outside of the DMA-APIs then we cannot really
> control that, but in this case if they do map with the DMA-API then it is *not* valid to skip
> these begin_access and end_access callbacks. And that is the case I am addressing here.

Good argument, but that needs quite some documentation then. udmabuf.c could use some general documentation anyway.

> 
> Right now we are not syncing the mapping for any attached device, we just zap it from
> the CPU caches using some hacky loopback and hope that is enough for the devices :/

Yeah that is just pretty horrible.

> 
>> Additional to that when CONFIG_DMABUF_DEBUG is enabled the DMA-buf code mangles the page addresses in the sg table to prevent importers from abusing it. That makes dma_sync_sgtable_for_cpu() and dma_sync_sgtable_for_device() on the exporter side crash.
>>
> 
> I was not aware of this mangle_sg_table() hack, must have been added while I was not looking :)
> 
> Seems very broken TBH, taking a quick look, I see on this line[0] you call it, then
> just a couple lines later you use that same mangled page_link to walk the SG table[1]..

sg_next() is skipping over the chain entries, only page entries are mangled, but I completely agree that this is as hackish as it can get.

We just had quite a number of harsh problems and even CVEs because importers didn't got that they absolutely shouldn't touch the underlying page of a mapping.

Allowing userspace to R/W to freed up memory or messing up the page count is not funny at all. 

> If anyone enables DMA_API_DEBUG and tried to attach/map a non-contiguous DMA-BUF with
> a chained sg I don't see how that doesn't crash out.
> 
>> That's the reason why DMA-buf heaps uses a copy of the sg table for calling dma_sync_sgtable_for_cpu()/dma_sync_sgtable_for_device().
>>
> 
> Could you point me to where Heaps uses a copy of the SG table? I see it using the
> exact same SG table for dma_sync_sgtable_for_*() that we created when mapping the
> device attachments.

See dup_sg_table() in system_heap.c.

Apart from stopping using sg_table in DMA-buf at all what we could potentially do is to improve the mangling. E.g. just allocate a new sg_table, copy over all the DMA addresses and keep the page_link pointing to the original one.

Regards,
Christian.

> 
> Thanks,
> Andrew
> 
> [0] https://github.com/torvalds/linux/blob/master/drivers/dma-buf/dma-buf.c#L1142
> [1] https://github.com/torvalds/linux/blob/master/drivers/dma-buf/dma-buf.c#L1151
> 
>> It's basically a hack and should be removed, but for this we need to change all clients which is tons of work.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Thanks
>>> Andrew
>>>
>>> Changes for v2:
>>>   - fix attachment table use-after-free
>>>   - rebased on v6.17-rc1
>>>
>>> Andrew Davis (3):
>>>    udmabuf: Keep track current device mappings
>>>    udmabuf: Sync buffer mappings for attached devices
>>>    udmabuf: Use module_misc_device() to register this device
>>>
>>>   drivers/dma-buf/udmabuf.c | 133 +++++++++++++++++++-------------------
>>>   1 file changed, 67 insertions(+), 66 deletions(-)
>>>
>>
>