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(-)
>>>
>>
>
