Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Deadlock due to leaked `pool_shrink_rwsem` read lock on early return in `ttm_pool_shrink()`. - [High] Fatal NULL pointer dereference caused by bypassed global pool initialization upon `shrinker_alloc` failure. --
commit a5f142f7cded6383539224542edc3c44efc5294c Author: Dave Airlie <[email protected]> ttm/pool: initialise the shrinker earlier (v2) This commit moves the shrinker initialization earlier in the module load sequence to accommodate future memcg enablement. It also adds a check in the shrinker callback to handle uninitialized lists. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index e4dbf4c93091c..f12b68812081f 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -440,10 +440,14 @@ static unsigned int ttm_pool_shrink(int nid, unsigned > long num_to_free) > > down_read(&pool_shrink_rwsem); > spin_lock(&shrinker_lock); > - pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list); > - list_move_tail(&pt->shrinker_list, &shrinker_list); > + pt = list_first_entry_or_null(&shrinker_list, typeof(*pt), > shrinker_list); > + if (pt) > + list_move_tail(&pt->shrinker_list, &shrinker_list); > spin_unlock(&shrinker_lock); > > + if (!pt) > + return 0; > + [Severity: High] Does this early return leak the pool_shrink_rwsem read lock? We acquire the lock just a few lines above via down_read(&pool_shrink_rwsem). If the shrinker list is empty and pt is NULL, returning here skips the up_read() call at the end of the function, which could lead to a deadlock when another writer or reader attempts to acquire it. [ ... ] > @@ -1402,6 +1406,17 @@ 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"); > + if (!mm_shrinker) > + return -ENOMEM; > + [Severity: High] If shrinker_alloc() fails and returns early here, does it leave the global pool structures completely uninitialized? Because the caller ttm_global_init() ignores the return value of ttm_pool_mgr_init(), driver initialization will continue. The newly moved return bypasses the initialization loop for structures like global_write_combined. When the driver later attempts to allocate from these pools, or when ttm_pool_mgr_fini() calls list_del() on the zero-filled uninitialized structures, can this cause a NULL pointer dereference? > + mm_shrinker->count_objects = ttm_pool_shrinker_count; > + mm_shrinker->scan_objects = ttm_pool_shrinker_scan; > + mm_shrinker->batch = TTM_SHRINKER_BATCH; > + mm_shrinker->seeks = 1; > + > + shrinker_register(mm_shrinker); > + > for (i = 0; i < NR_PAGE_ORDERS; ++i) { > ttm_pool_type_init(&global_write_combined[i], NULL, > ttm_write_combined, i); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
