From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dma-buf: heaps: Add Coherent heap to dmabuf heaps Date: Fri, 27 Feb 2026 15:38:23 +1000 Message-ID: In-Reply-To: <20260224-b4-dmabuf-heap-coherent-rmem-v1-1-dffef43298ac@redhat.com> References: <20260224-b4-dmabuf-heap-coherent-rmem-v1-1-dffef43298ac@redhat.com> <20260224-b4-dmabuf-heap-coherent-rmem-v1-1-dffef43298ac@redhat.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review #### Critical Issues **1. `virt_to_page()` on `dma_alloc_coherent()` return value is unsafe** ```c for (pg =3D 0; pg < pagecount; pg++) buffer->pages[pg] =3D virt_to_page((char *)buffer->alloc_vaddr + (pg * PAGE_SIZE)); ``` `dma_alloc_coherent()` on a device backed by reserved coherent memory goes = through `dma_alloc_from_coherent()`, which returns a pointer into `mem->vir= t_base`. That virtual base comes from `memremap(phys_addr, size, MEMREMAP_W= C)` (in `dma_init_coherent_memory()` at `kernel/dma/coherent.c`). On archit= ectures where `memremap()` returns a vmalloc/ioremap-space address rather t= han a linear-map address, `virt_to_page()` will produce garbage or crash. The existing `coherent.c` code avoids this =E2=80=94 it stores `pfn_base = =3D PFN_DOWN(phys_addr)` and uses PFN arithmetic directly in `remap_pfn_ran= ge()` calls. The correct approach here would be to derive PFNs from the DMA= address: ```c phys_addr_t phys =3D dma_to_phys(coh_heap->dev, buffer->dma_addr); for (pg =3D 0; pg < pagecount; pg++) buffer->pages[pg] =3D pfn_to_page(PFN_DOWN(phys) + pg); ``` This is the single most critical issue in the patch. **2. Missing memory zeroing =E2=80=94 information leak to userspace** The CMA heap explicitly zeroes allocated pages before exporting them: ```c /* CMA heap does: */ memset(page_address(cma_pages), 0, size); ``` The coherent heap does not zero the memory returned by `dma_alloc_coherent(= )`. For reserved-memory coherent pools, `dma_alloc_from_coherent()` returns= a pointer into pre-mapped memory without zeroing it. Between allocate/free= cycles, stale data from prior users is exposed. Since these buffers are ex= ported to userspace, this is a security concern. A `memset(buffer->alloc_va= ddr, 0, buffer->len)` is needed after allocation. #### Architectural Issues **3. Reverse dependency: `kernel/dma/coherent.c` calling into dma-buf heaps= ** ```c /* kernel/dma/coherent.c: */ +#include ... + if (IS_ENABLED(CONFIG_DMABUF_HEAPS_COHERENT)) { + int ret =3D dma_heap_coherent_register(rmem); ``` This creates a layering violation =E2=80=94 core DMA infrastructure now cal= ls up into the dma-buf heaps subsystem. The CMA heap does not do this; it u= ses `cma_for_each_area()` to iterate and register CMA areas autonomously fr= om `drivers/dma-buf/heaps/cma_heap.c`, without touching any CMA core code. The coherent heap should similarly find reserved-memory regions on its own = at `late_initcall` time (e.g., by iterating the reserved-memory DT nodes), = rather than hooking into `rmem_dma_setup()`. This would keep the modificati= on entirely within `drivers/dma-buf/heaps/` and avoid touching `kernel/dma/= coherent.c`. **4. All coherent reserved-memory regions are unconditionally exposed to us= erspace** Every `shared-dma-pool` (non-reusable) region automatically gets a dma-buf = heap. Some of these regions may be intended exclusively for specific kernel= drivers. A malicious or buggy userspace process could exhaust a device-spe= cific reserved pool, starving the kernel driver. There should be a DT opt-i= n mechanism (e.g., a `dma-buf-heap` property) to control which regions are = exposed, rather than exposing all of them. #### Code Quality Issues **5. Unnecessary `#if IS_ENABLED(CONFIG_CGROUP_DMEM)` guards throughout** The header `linux/cgroup_dmem.h` already provides no-op stubs when `CONFIG_= CGROUP_DMEM` is disabled (`dmem_cgroup_try_charge()` returns 0, `dmem_cgrou= p_uncharge()` is a no-op, etc.). All the `#if IS_ENABLED(CONFIG_CGROUP_DMEM= )` blocks in the patch are unnecessary and clutter the code. The entire cgr= oup charging path can be written without any `#if` guards: ```c /* In struct coherent_heap =E2=80=94 no #if needed: */ struct dmem_cgroup_region *cg; /* In allocate =E2=80=94 no #if needed: */ if (mem_accounting) { ret =3D dmem_cgroup_try_charge(coh_heap->cg, size, &buffer->pool, NULL); if (ret) goto free_buffer; } ``` The stubs will compile to nothing when disabled. This would significantly c= lean up `coherent_heap_allocate()`, `coherent_heap_dma_buf_release()`, `__c= oherent_heap_register()`, and the error paths. **6. `cg_unregister` error label structure is confusing** ```c cg_unregister: #if IS_ENABLED(CONFIG_CGROUP_DMEM) dmem_cgroup_unregister_region(coh_heap->cg); #endif pdev_unregister: ``` When `CONFIG_CGROUP_DMEM` is disabled, `cg_unregister` is an empty label th= at falls through to `pdev_unregister`. This compiles but is confusing. Remo= ving the ifdefs (as per point 5) would fix this naturally. **7. Missing `device_release()` call in error/cleanup paths** In `__coherent_heap_register()`, after a successful `rmem->ops->device_init= ()` call, the error paths only call `platform_device_unregister()` but neve= r call `rmem->ops->device_release()` to undo the init: ```c if (rmem->ops && rmem->ops->device_init) { ret =3D rmem->ops->device_init(rmem, &coh_heap->pdev->dev); if (ret) goto pdev_unregister; } /* ... later errors goto cg_unregister or pdev_unregister ... */ /* but never call rmem->ops->device_release() */ ``` If `dma_heap_add()` or `dmem_cgroup_register_region()` fails after a succes= sful `device_init()`, the reserved memory binding is leaked. Need a `device= _release` cleanup step. **8. Deferred registration only on `-ENOMEM` is fragile** ```c int dma_heap_coherent_register(struct reserved_mem *rmem) { int ret; ret =3D __coherent_heap_register(rmem); if (ret =3D=3D -ENOMEM) return coherent_heap_add_deferred(rmem); return ret; } ``` `rmem_dma_setup()` runs from `RESERVEDMEM_OF_DECLARE`, which is during very= early boot before slab is available. The assumption is that `kzalloc()` wi= ll fail with `-ENOMEM` and trigger deferral. But `platform_device_register_= simple()` or other calls could fail with different error codes (e.g., `-ENO= DEV`, `-EPROBE_DEFER`) before the allocator is ready. If the intent is to u= nconditionally defer during early boot, just always defer =E2=80=94 or bett= er yet, don't hook into `rmem_dma_setup()` at all (see point 3). **9. `COHERENT_AREAS_DEFERRED` Kconfig naming is inconsistent** ``` config COHERENT_AREAS_DEFERRED int "Max deferred coherent reserved-memory regions" ``` This name doesn't follow the `DMABUF_HEAPS_` prefix convention used by othe= r options in this Kconfig. Should be something like `DMABUF_HEAPS_COHERENT_= MAX_DEFERRED`. Also, making this a Kconfig integer for what is essentially = an implementation detail of early boot deferral seems like unnecessary user= -facing configuration. **10. `begin_cpu_access`/`end_cpu_access` sync correctness for coherent mem= ory** ```c static int coherent_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf, enum dma_data_direction direction) { ... list_for_each_entry(a, &buffer->attachments, list) { if (!a->mapped) continue; dma_sync_sgtable_for_cpu(a->dev, &a->table, direction); } ``` This is copied from the CMA heap, but the semantics are subtly different. T= he underlying allocation is DMA-coherent (uncached/write-combining), so the= sync on the allocation-side device is unnecessary. The sync on *attaching*= devices' sg_tables may still be needed if those devices' DMA mappings are = not coherent. The code works but a comment explaining this would be helpful= , since the reader may wonder why coherent memory needs sync at all. **11. Dependency on unmerged patches** The `mem_accounting` variable and the dmem cgroup charging pattern are from= Eric's series ([1] in the cover letter) which is not yet merged. This shou= ld be clearly stated as a dependency, and ideally this patch should be part= of or follow that series. #### Minor Issues - `kmalloc_array()` is used for `buffer->pages` while `kzalloc_obj()` is us= ed elsewhere =E2=80=94 inconsistent with the CMA heap which uses `kmalloc_o= bjs()` for the same purpose. - The `pr_warn` in `coherent_heap_register_deferred()` is missing a newline= : `pr_warn("Failed to add coherent heap %s",` =E2=86=92 should have `\n`. - No `MODULE_LICENSE()` (though since it's `bool` not `tristate`, this is c= osmetic =E2=80=94 but `MODULE_DESCRIPTION` without `MODULE_LICENSE` is inco= nsistent). --- Generated by Claude Code Patch Reviewer