Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [Critical] Direct map caching attributes (WC/UC) are not restored to Write-Back (WB) when a page is freed due to a memcg charging failure. - [High] Missing `list_lru_destroy()` causes global list corruption and Use-After-Free when TTM pools are destroyed. - [High] ttm_pool_check_objcg fails to allocate memcg LRU entries for device-specific pools, breaking memcg-aware shrinking. - [High] ttm_pool_shrinker_count returns the global pool count instead of the memcg-specific count, breaking memcg reclaim proportionality. - [High] A race condition in `ttm_pool_type_give()` allows the shrinker to free a page before local metadata updates finish. - [High] Uncharged pages stolen from the root pool by accounted BOs are not charged to the requesting memcg. - [Medium] Ignoring `memcg_list_lru_alloc()` failure causes a `VM_WARN_ON` when freeing pages to the pool. - [Medium] ttm_pool_check_objcg hardcodes GFP_KERNEL, ignoring the caller's allocation context flags. - [Medium] Missing `up_read(&pool_shrink_rwsem)` when `shrinker_list` is empty in `ttm_pool_shrink`. - [Low] Off-by-two loop bounds error in `ttm_pool_shrinker_scan()` causes redundant scanning iterations. --
commit 3d1a95525ccb3182a802e2681e222aef27971cef Author: Dave Airlie <[email protected]> Subject: ttm/pool: enable memcg tracking and shrinker. (v3) This patch introduces memory cgroup (memcg) tracking and memcg-aware shrinking for the TTM page pool. It updates the LRU lists to operate in memcg mode and allows pooled pages reparented to a higher memcg to be reused. [ ... ] > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c [ ... ] > @@ -317,12 +321,11 @@ static void ttm_pool_type_give(struct ttm_pool_type > *pt, struct page *p) > > INIT_LIST_HEAD(&p->lru); > rcu_read_lock(); > - list_lru_add(&pt->pages, &p->lru, nid, NULL); > + list_lru_add(&pt->pages, &p->lru, nid, page_memcg_check(p)); > rcu_read_unlock(); > > atomic_long_add(num_pages, &allocated_pages[nid]); > - mod_lruvec_page_state(p, NR_GPU_ACTIVE, -num_pages); > - mod_lruvec_page_state(p, NR_GPU_RECLAIM, num_pages); > + mem_cgroup_move_gpu_page_reclaim(NULL, p, pt->order, true); [Severity: High] Can a background shrinker isolate and free this page before local metadata updates finish? Calling list_lru_add() exposes the page to the global LRU. If a shrinker frees it concurrently, the subsequent call to mem_cgroup_move_gpu_page_reclaim() will dereference page_objcg(p) on a freed page, causing a use-after-free regression. > } > > static enum lru_status take_one_from_lru(struct list_head *item, [ ... ] > @@ -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: Critical] When mem_cgroup_move_gpu_page_reclaim() fails, calling __free_pages() directly bypasses ttm_pool_free_page(). Does this skip restoring x86 direct map cache attributes to Write-Back via set_pages_wb? Returning WC or UC pages to the kernel allocator could cause Machine Check Exceptions or data corruption. Additionally, if the page was allocated via the DMA API, this would leak the DMA mapping by skipping dma_free_attrs(). > } > - return p; > + *page_out = p; > + return ret; > +} > + > +/* Take pages from a specific pool_type, return NULL when nothing available > */ > +static struct page *ttm_pool_type_take(struct ttm_pool_type *pt, int nid, > + struct obj_cgroup *orig_objcg) > +{ [ ... ] > + /* > + * Attempt to get a page from the current memcg, but if it hasn't got > any in it's level, > + * go up to the parent and check there. This helps the scenario where > multiple apps get > + * started into their own cgroup from a common parent and want to reuse > the pools. > + */ > + while (!page_out) { > + ret = pool_lru_get_page(pt, nid, &page_out, orig_objcg, memcg); > + if (ret == 1) > + break; > + if (!memcg) > + break; > + memcg = parent_mem_cgroup(memcg); > + if (!memcg) > + break; > + } [Severity: High] When taking an uncharged page from the root pool, does this loop pass it downstream to be incorrectly handled as uncharged? In mem_cgroup_move_gpu_page_reclaim(), an early exit for missing objcg skips charging the new_objcg. This would allow an accounted buffer object to steal uncharged pages from the root pool without being charged for them, bypassing container memory limits. > + > + mem_cgroup_put(orig_memcg); > + return page_out; > } > > /* Initialize and add a pool type to the global shrinker list */ > @@ -360,7 +399,7 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, > struct ttm_pool *pool, > pt->pool = pool; > pt->caching = caching; > pt->order = order; > - list_lru_init(&pt->pages); > + list_lru_init_memcg(&pt->pages, mm_shrinker); [Severity: High] Since list_lru_init_memcg() registers the LRU lists to the global memcg_list_lrus list, do we need a corresponding list_lru_destroy() in ttm_pool_type_fini()? Without it, unloading a TTM device and freeing the pool memory could leave the LRU list linked in the global list, leading to a use-after-free regression during subsequent memcg operations. > > spin_lock(&shrinker_lock); > list_add_tail(&pt->shrinker_list, &shrinker_list); [ ... ] > +/* > + * 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); [Severity: Medium] Could using GFP_KERNEL here unconditionally be unsafe? The caller __ttm_pool_alloc() determines safe allocation flags (such as handling gfp_retry_mayfail), but they are ignored here. This might trigger the OOM killer unexpectedly or cause deadlocks if called from contexts where FS/IO operations are unsafe. > + if (r) > + break; [Severity: Medium] Does ignoring the memcg_list_lru_alloc() failure here lead to a VM_WARN_ON later? If the allocation fails due to memory pressure and we break early, freeing pages to this LRU might cause list_lru_add() to fail finding the memcg-specific list. This would fall back to the parent memcg and potentially break accurate tracking. > + r = memcg_list_lru_alloc(memcg, &global_uncached[i].pages, > GFP_KERNEL); > + if (r) > + break; > + } [Severity: High] Is it intentional to only pre-allocate global_write_combined and global_uncached here? The global_dma32_* pools and any device-specific caching pools appear to be missing. Pages added to these uninitialized pools might hit a VM_WARN_ON and fall back to the parent memcg, completely breaking memcg-aware shrinking for them. > + mem_cgroup_put(memcg); > +#endif > +} [ ... ] > /* Free pages using the per-node shrinker list */ > -static unsigned int ttm_pool_shrink(int nid, unsigned long num_to_free) > +static unsigned int ttm_pool_shrink(int nid, > + struct mem_cgroup *memcg, > + unsigned long num_to_free) > { > LIST_HEAD(dispose); > struct ttm_pool_type *pt; > @@ -448,7 +514,11 @@ static unsigned int ttm_pool_shrink(int nid, unsigned > long num_to_free) > if (!pt) > return 0; [Severity: Medium] If shrinker_list is empty and we return 0 early, does this leak the pool_shrink_rwsem read lock? A missing up_read() here could eventually lead to deadlocks when a writer tries to acquire the lock. > > - num_pages = list_lru_walk_node(&pt->pages, nid, > pool_move_to_dispose_list, &dispose, &num_to_free); > + if (!memcg) { > + num_pages = list_lru_walk_node(&pt->pages, nid, > pool_move_to_dispose_list, &dispose, &num_to_free); > + } else { > + num_pages = list_lru_walk_one(&pt->pages, nid, memcg, > pool_move_to_dispose_list, &dispose, &num_to_free); > + } > num_pages *= 1 << pt->order; [ ... ] > @@ -1214,10 +1288,14 @@ static unsigned long ttm_pool_shrinker_scan(struct > shrinker *shrink, > struct shrink_control *sc) > { > unsigned long num_freed = 0; > + int num_pools; > + spin_lock(&shrinker_lock); > + num_pools = list_count_nodes(&shrinker_list); > + spin_unlock(&shrinker_lock); > > do > - num_freed += ttm_pool_shrink(sc->nid, sc->nr_to_scan); > - while (num_freed < sc->nr_to_scan && > + num_freed += ttm_pool_shrink(sc->nid, sc->memcg, > sc->nr_to_scan); > + while (num_pools-- >= 0 && num_freed < sc->nr_to_scan && [Severity: Low] Does the num_pools-- >= 0 condition cause redundant scanning iterations? If num_pools is 1, the loop executes for 1, 0, and -1, resulting in 3 iterations instead of 1. This would force redundant scanning of the same pools in the LRU and waste CPU cycles. > atomic_long_read(&allocated_pages[sc->nid])); > > sc->nr_scanned = num_freed; > @@ -1406,7 +1484,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: High] With SHRINKER_MEMCG_AWARE enabled, the shrinker is expected to report the number of objects owned by sc->memcg. However, ttm_pool_shrinker_count() unconditionally returns the total number of pages in the global pool (allocated_pages[sc->nid]). Could this falsely inflate the target memcg's apparent footprint, tricking the VM into applying massive, disproportionate reclaim pressure against it? > if (!mm_shrinker) > return -ENOMEM; > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
