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-heap: charge dma-buf memory via explicit memcg Date: Sat, 16 May 2026 13:56:21 +1000 Message-ID: In-Reply-To: <20260512-v2_20230123_tjmercier_google_com-v1-2-6326701c3691@redhat.com> References: <20260512-v2_20230123_tjmercier_google_com-v1-0-6326701c3691@redhat.com> <20260512-v2_20230123_tjmercier_google_com-v1-2-6326701c3691@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 This is the core patch. It moves charging from `dma_buf_export()` to `dma_h= eap_buffer_alloc()`, adds the `charge_pid_fd` field to the UAPI struct, and= uses pidfd to resolve the target memcg. **Issue 1 (Bug) - Unsafe `task->mm` access:** ```c + task =3D pidfd_get_task(heap_allocation->charge_pid_fd, &pidfd_flags); + if (IS_ERR(task)) + return PTR_ERR(task); + ... + memcg =3D get_mem_cgroup_from_mm(task->mm); + put_task_struct(task); ``` `task->mm` can be NULL if the pidfd points to a kernel thread or a process = that has already called `exit_mm()`. While `get_mem_cgroup_from_mm(NULL)` w= on't crash (it falls back to root/current memcg), it would silently charge = the wrong cgroup. The correct pattern is: ```c struct mm_struct *mm =3D get_task_mm(task); if (!mm) { put_task_struct(task); return -ESRCH; } memcg =3D get_mem_cgroup_from_mm(mm); mmput(mm); put_task_struct(task); ``` This also closes a potential TOCTOU race where the task exits between `pidf= d_get_task()` and the mm access. **Issue 2 - Charge/release ordering:** The charge happens after `heap->ops-= >allocate()` succeeds: ```c + dmabuf =3D heap->ops->allocate(heap, len, fd_flags, heap_flags); + if (IS_ERR(dmabuf)) + return PTR_ERR(dmabuf); + + nr_pages =3D len / PAGE_SIZE; + ... + if (memcg) { + if (!mem_cgroup_charge_dmabuf(memcg, nr_pages, GFP_KERNEL)) { ``` The pages are already allocated before the charge check. If the cgroup is o= ver its limit, we've allocated pages we immediately need to free. Ideally t= he charge should happen before the allocation to serve as a gate, or the al= location should use `__GFP_ACCOUNT` to let the allocator handle cgroup limi= ts during allocation itself. **Issue 3 - `__GFP_ACCOUNT` removal:** ```c - if (mem_accounting) - flags |=3D __GFP_ACCOUNT; ``` Removing `__GFP_ACCOUNT` means system heap pages are no longer tracked unde= r kmem in the cgroup. The cover letter says this avoids double-counting, bu= t the two mechanisms track different things: `__GFP_ACCOUNT` charges to the= cgroup's kmem counter, while `MEMCG_DMABUF` is a separate stat. They're no= t truly double-counted in a harmful way =E2=80=94 they track different acco= unting dimensions. This design decision should be justified more carefully:= are there real systems where both paths being active causes problems (e.g.= , triggering OOM too early)? **Issue 4 - Refcount correctness:** The refcounting is actually correct, bu= t subtle enough to deserve a comment. When `charge_to` is non-NULL: - `css_get()` inside `dma_heap_buffer_alloc()` takes a new reference - The caller's reference from `get_mem_cgroup_from_mm()` is dropped by `mem= _cgroup_put(memcg)` after `dma_heap_buffer_alloc()` returns - The buffer_alloc reference transfers to `dmabuf->memcg` and is dropped on= release When `charge_to` is NULL and `mem_accounting` is true, the reference from t= he internal `get_mem_cgroup_from_mm(current->mm)` transfers directly to `dm= abuf->memcg`. This all works but is non-obvious =E2=80=94 consider adding a= brief comment. --- Generated by Claude Code Patch Reviewer