From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: cgroup/dmem: add dmem.memcg control file for double-charging to memcg Date: Mon, 25 May 2026 22:42:50 +1000 Message-ID: In-Reply-To: <20260519-cgroup-dmem-memcg-double-charge-v2-2-db4d1407062b@redhat.com> References: <20260519-cgroup-dmem-memcg-double-charge-v2-0-db4d1407062b@redhat.com> <20260519-cgroup-dmem-memcg-double-charge-v2-2-db4d1407062b@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 **1. Charge/uncharge size inconsistency (potential accounting leak)** In `dmem_cgroup_try_charge()`: ```c unsigned long nr_pages =3D PAGE_ALIGN(size) >> PAGE_SHIFT; ... if (charge_memcg) { ... mem_cgroup_dmem_charge(pool->cs->css.cgroup, nr_pages, GFP_KERNEL); } ``` But in `dmem_cgroup_uncharge()`: ```c if (atomic_read(&pool->region->memcg_status) =3D=3D DMEM_MEMCG_LOCKED_ON && !WARN_ON_ONCE(size > (u64)UINT_MAX << PAGE_SHIFT)) mem_cgroup_dmem_uncharge(pool->cs->css.cgroup, PAGE_ALIGN(size) >> PAGE_SHIFT); ``` The charge path checks `size > (u64)UINT_MAX << PAGE_SHIFT` and bails out e= arly with `-EINVAL` *before* charging memcg. But the uncharge path uses `WA= RN_ON_ONCE` on the same condition and *skips the uncharge* if the size is t= oo large. If somehow a large allocation was charged (e.g., due to a code ch= ange or different path), the uncharge would be silently skipped, leaking th= e memcg charge. The WARN_ON_ONCE is also inverted =E2=80=94 it triggers whe= n the condition is true (size is too large) and then the `!` causes the unc= harge to be skipped. The logic is correct but confusing to read. **2. `apply_memcg_charge` is called on every charge attempt, even if it's a= lready locked** ```c charge_memcg =3D apply_memcg_charge(®ion->memcg_status); ``` This function does an `atomic_cmpxchg` on every allocation to lock the stat= e, even after the first successful lock. For `DMEM_MEMCG_LOCKED_OFF` and `D= MEM_MEMCG_LOCKED_ON`, it just reads and returns, which is fine. But for the= unlocked states, it'll keep attempting cmpxchg until it succeeds. This is = a minor hot-path concern =E2=80=94 once locked, the `atomic_read` in the sw= itch is cheap. No bug, just noting the fast path is already O(1) after the = first allocation. **3. Write handler silently ignores locked state** ```c atomic_cmpxchg(®ion->memcg_status, flag ? DMEM_MEMCG_OFF : DMEM_MEMCG_ON, flag ? DMEM_MEMCG_ON : DMEM_MEMCG_OFF); /* Continue if a region is already locked. */ ``` When a write to `dmem.memcg` tries to change a region that is already locke= d, the `atomic_cmpxchg` silently fails (the old value doesn't match) and th= e write returns success (`nbytes`). The admin gets no indication that the c= hange was not applied. It would be more user-friendly to return `-EBUSY` or= `-EPERM` when the region is locked, rather than silently discarding the wr= ite. At minimum, the admin could check by reading the file, but silent succ= ess on a no-op write is a usability issue. **4. Write handler returns `nbytes` on partial processing** The write handler processes lines in a loop. If parsing fails mid-way (e.g.= , second line is invalid), it returns `-EINVAL` having already processed th= e first line and released its region ref. But there's no rollback of the fi= rst line's state change. While cgroup write operations are typically single= -line, the parser supports multi-line input, so this is a potential issue. **5. Missing `CFTYPE_NS_DELEGATABLE` check** The `dmem.memcg` file is `CFTYPE_ONLY_ON_ROOT` but has no `.access` flags t= o restrict writes. The commit message says "root-only cgroupfs file that le= ts an administrator configure" =E2=80=94 but the write permission is just s= tandard file permissions. Any process with write access to the cgroupfs roo= t can modify the setting, not just uid 0. For a security-sensitive setting = that affects accounting, this might want explicit access control. However, = this matches the pattern of other root-only cgroup files, so it may be inte= ntional. **6. `depends_on` declaration** ```c #ifdef CONFIG_MEMCG .depends_on =3D 1 << memory_cgrp_id, #endif ``` This ensures the memory controller is enabled whenever dmem is enabled. Thi= s is correct for the feature, but it changes behavior even when `dmem.memcg= ` is not used =E2=80=94 any system with both CONFIG_MEMCG and CONFIG_CGROUP= _DMEM will now force the memory controller to be active whenever dmem is. T= his could be surprising for users who want dmem without memcg overhead. The= tradeoff makes the implementation simpler (no need to handle the case wher= e memcg is disabled at runtime), but should be documented. **7. `atomic_read` in uncharge path has no acquire barrier** ```c if (atomic_read(&pool->region->memcg_status) =3D=3D DMEM_MEMCG_LOCKED_ON && ``` The uncharge path uses a plain `atomic_read` to check the locked-on state. = The corresponding `atomic_cmpxchg` in `apply_memcg_charge` provides full me= mory ordering, so the transition to LOCKED_ON is properly visible. However,= there's a window between when `apply_memcg_charge` returns `true` (transit= ioning to LOCKED_ON) and when the charge actually completes. If the charge = fails after `apply_memcg_charge` locks the state, the state is permanently = locked ON but no charge was made. The error path in `dmem_cgroup_try_charge= ` handles this correctly (it uncharges memcg on `page_counter_try_charge` f= ailure), but it's worth verifying that all error paths are covered. **8. Documentation placement** The documentation is well-written and clear. The placement after the existi= ng dmem documentation section is correct. One minor note: the doc says "Thi= s file is only available when the kernel is built with CONFIG_MEMCG" =E2=80= =94 this should probably also mention CONFIG_CGROUP_DMEM since both are req= uired. --- Generated by Claude Code Patch Reviewer