Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [Critical] Missing `list_lru_destroy` in `ttm_pool_type_fini` causes an exploitable Use-After-Free during cgroup reparenting. - [High] Missing `memcg_list_lru_alloc` for DMA32 pools and ignored allocation failures cause continuous `VM_WARN_ON` spam and broken isolation. - [High] Clearing `page->memcg_data` before uncharging causes permanent memcg charge leaks. - [High] `pool_lru_get_page` frees write-combined or uncached pages directly to the buddy allocator without restoring them to write-back, causing cache aliasing. - [High] `ttm_pool_check_objcg` hardcodes `GFP_KERNEL`, overriding caller constraints like `__GFP_RETRY_MAYFAIL`. - [High] Unaccounted allocations from the pool permanently charge the old memcg for active memory, breaking isolation. - [Medium] `ttm_pool_shrinker_count` returns the global pool size rather than the memcg-specific pool size, breaking memcg-aware reclaim heuristics. --
commit 71c02307644b082161406f13e214b752caedbe33 Author: Dave Airlie <[email protected]> Subject: ttm/pool: enable memcg tracking and shrinker. (v3) This commit enables memcg tracking for the TTM pool backend and makes the shrinker memcg aware. It also introduces logic to allow newer memcgs to reparent and reuse pooled pages from parent memcgs. [ ... ] > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -337,20 +340,56 @@ static enum lru_status take_one_from_lru(struct > list_head *item, [ ... ] > +static int pool_lru_get_page(struct ttm_pool_type *pt, int nid, > + struct page **page_out, > + struct obj_cgroup *objcg, > + struct mem_cgroup *memcg) > { > int ret; > struct page *p = NULL; > unsigned long nr_to_walk = 1; > + unsigned int num_pages = 1 << pt->order; > > - ret = list_lru_walk_node(&pt->pages, nid, take_one_from_lru, (void > *)&p, &nr_to_walk); > + ret = list_lru_walk_one(&pt->pages, nid, memcg, take_one_from_lru, > (void *)&p, &nr_to_walk); > if (ret == 1 && p) { > - atomic_long_sub(1 << pt->order, &allocated_pages[nid]); > - mod_lruvec_page_state(p, NR_GPU_ACTIVE, (1 << pt->order)); > - mod_lruvec_page_state(p, NR_GPU_RECLAIM, -(1 << pt->order)); > + atomic_long_sub(num_pages, &allocated_pages[nid]); > + > + if (!mem_cgroup_move_gpu_page_reclaim(objcg, p, pt->order, > false)) { > + __free_pages(p, pt->order); > + p = NULL; > + } [Severity: High] Does this direct call to __free_pages() bypass the required caching restorations? In pool_lru_get_page(), when mem_cgroup_move_gpu_page_reclaim() fails, the page is freed directly to the buddy allocator. If the page came from a write-combined or uncached pool, this skips the set_pages_wb() call normally performed by ttm_pool_free_page(). Could this lead to cache aliasing issues and machine check exceptions on architectures like x86 since the direct map caching attributes are not restored to write-back? [Severity: High] Will unaccounted allocations permanently charge the previous memcg owner? When a process not tracked by a memcg (or the root memcg) allocates memory from the pool, pool_lru_get_page() passes a NULL objcg to mem_cgroup_move_gpu_page_reclaim(). Looking at mm/memcontrol.c:mem_cgroup_move_gpu_page_reclaim(), if the new objcg is NULL, it evaluates the condition (!new_objcg) as true and moves the charge from NR_GPU_RECLAIM to NR_GPU_ACTIVE on the old memcg instead of uncharging it. Doesn't this leave the old memcg permanently charged for pages it no longer owns while the root process uses them, breaking memcg isolation? > } > - return p; > + *page_out = p; > + return ret; > +} [ ... ] > @@ -403,6 +442,31 @@ static void ttm_pool_type_fini(struct ttm_pool_type *pt) > ttm_pool_dispose_list(pt, &dispose); > } [Severity: Critical] Is list_lru_destroy() required here to prevent a use-after-free during cgroup reparenting? When a DRM driver unbinds or the TTM module is unloaded, ttm_pool_type_fini() cleans up the pool type but does not call list_lru_destroy(). The list_lru structures remain registered in the global memcg_list_lrus list. If a memory cgroup is subsequently deleted, memcg_reparent_list_lrus() iterates over this global list and accesses the freed memory. Can we add a call to list_lru_destroy(&pt->pages) to safely unregister it? > +/* > + * This function doesn't currently check dma32, because no driver using this > + * support dma32. This should be added and debugged when that changes. > + */ > +static void ttm_pool_check_objcg(struct obj_cgroup *objcg) > +{ > +#ifdef CONFIG_MEMCG > + int r = 0; > + struct mem_cgroup *memcg; > + if (!objcg) > + return; > + > + memcg = get_mem_cgroup_from_objcg(objcg); > + for (unsigned i = 0; i < NR_PAGE_ORDERS; i++) { > + r = memcg_list_lru_alloc(memcg, > &global_write_combined[i].pages, GFP_KERNEL); > + if (r) > + break; > + r = memcg_list_lru_alloc(memcg, &global_uncached[i].pages, > GFP_KERNEL); > + if (r) > + break; > + } [Severity: High] Can skipping the global_dma32_* pools here cause kernel warning spam and broken tracking? The loop in ttm_pool_check_objcg() explicitly allocates list_lrus for global_write_combined and global_uncached pools, but skips the DMA32 variants. If a driver using DMA32 pools frees a page to the pool, list_lru_add() will attempt to add it to the memcg's list. Because the list wasn't allocated, lock_list_lru_of_memcg() fails to find the list, triggers a VM_WARN_ON(!css_is_dying), and escalates to the parent memcg. Could this be triggered trivially by userspace interacting with DMA32 DRM drivers inside a memory cgroup? [Severity: High] Is it safe to use a hardcoded GFP_KERNEL allocation flag here? The __ttm_pool_alloc() caller might operate under strict constraints, passing a ttm_operation_ctx that sets gfp_retry_mayfail to avoid deadlocks with the GPU shrinker. Because ttm_pool_check_objcg() uses GFP_KERNEL directly instead of propagating the caller's allocation flags, it drops constraints like __GFP_RETRY_MAYFAIL and __GFP_NOWARN. Might this allow the allocator to block indefinitely or invoke the OOM killer while critical DRM locks are held? > + mem_cgroup_put(memcg); > +#endif > +} [ ... ] > @@ -612,6 +682,7 @@ static int ttm_pool_restore_commit(struct > ttm_pool_tt_restore *restore, > */ > ttm_pool_split_for_swap(restore->pool, p); > copy_highpage(restore->alloced_page + i, p); > + p->memcg_data = 0; > __free_pages_gpu_account(p, 0, false); > } [Severity: High] Does clearing p->memcg_data before uncharging cause the memcg charge to leak? By explicitly setting p->memcg_data = 0 here, the subsequent call to __free_pages_gpu_account() -> mem_cgroup_uncharge_gpu_page() cannot identify which memcg to uncharge, since page_objcg(page) will return NULL. Wouldn't this make the uncharge a no-op, permanently leaking the memcg GPU charge counters and the obj_cgroup reference? [ ... ] > @@ -1402,7 +1482,7 @@ int ttm_pool_mgr_init(unsigned long num_pages) > spin_lock_init(&shrinker_lock); > INIT_LIST_HEAD(&shrinker_list); > > - mm_shrinker = shrinker_alloc(SHRINKER_NUMA_AWARE, "drm-ttm_pool"); > + mm_shrinker = shrinker_alloc(SHRINKER_MEMCG_AWARE | > SHRINKER_NUMA_AWARE, "drm-ttm_pool"); [Severity: Medium] How does the shrinker count callback handle specific memory cgroups now that it is memcg aware? This registers the shrinker with SHRINKER_MEMCG_AWARE. The VM subsystem will now call the count_objects callback (ttm_pool_shrinker_count) for specific memory cgroups under pressure, passing sc->memcg. However, ttm_pool_shrinker_count() ignores sc->memcg and always returns the global pool size via atomic_long_read(&allocated_pages[sc->nid]). Could this cause inefficient and disproportionate reclaim pressure, as a memcg with zero TTM pages is falsely reported as holding the global amount? > if (!mm_shrinker) > return -ENOMEM; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
