Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The patch introduces a use-after-free race condition between `ttm_pool_type_fini()` and the active memory shrinker `ttm_pool_shrink()` by calling `list_lru_destroy()` prematurely. Pre-existing issues: - [High] `ttm_pool_type_init()` ignores the return value of `list_lru_init()`, leading to a NULL pointer dereference if allocation fails. - [High] `ttm_pool_shrink()` assumes `shrinker_list` is never empty, causing memory corruption and crashes during module unload if triggered. -- commit 2d02cce6fc533c55922ee9772ecc91925df8846e Author: Óscar Megía López <[email protected]> Subject: Memory leak error in qxl unbind This commit fixes a 64 bytes memory leak discovered when qxl unbind is executed. > diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c > index 278bbe7a11add..15191efe9f7e8 100644 > --- a/drivers/gpu/drm/ttm/ttm_pool.c > +++ b/drivers/gpu/drm/ttm/ttm_pool.c > @@ -401,6 +401,7 @@ static void ttm_pool_type_fini(struct ttm_pool_type *pt) > > list_lru_walk(&pt->pages, pool_move_to_dispose_list, &dispose, > LONG_MAX); > ttm_pool_dispose_list(pt, &dispose); > + list_lru_destroy(&pt->pages); [Severity: High] Can this list_lru_destroy() call cause a use-after-free race with the active memory shrinker? If ttm_pool_shrink() executes concurrently, it takes a read lock on pool_shrink_rwsem and iterates over pt->pages: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_shrink() { ... num_pages = list_lru_walk_node(&pt->pages, nid, pool_move_to_dispose_list, &dispose, &num_to_free); ... } Because list_lru_destroy() is called here before ttm_pool_synchronize_shrinkers() (which is called later in ttm_pool_fini()), the shrinker might access the LRU after its memory has been freed. Should this destruction be deferred until after the shrinkers are synchronized? > } > > /* Return the pool_type to use for the given caching and order */ [Severity: High] This isn't a bug introduced by this patch, but I noticed that ttm_pool_type_init() ignores the return value of list_lru_init(): drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_type_init() { ... list_lru_init(&pt->pages); ... } If the internal allocation in list_lru_init() fails under memory pressure, it returns -ENOMEM and leaves pt->pages uninitialized. Does this lead to a NULL pointer dereference when the shrinker later iterates or when a page is added? [Severity: High] This is a pre-existing issue, but I noticed that ttm_pool_shrink() assumes shrinker_list is never empty: drivers/gpu/drm/ttm/ttm_pool.c:ttm_pool_shrink() { ... pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list); ... } During module unload in ttm_pool_mgr_fini(), all pool types are unlinked from shrinker_list. Before shrinker_free() resolves, there is a window where the list is empty. If the shrinker triggers during this window, will list_first_entry() return a bogus pointer and cause memory corruption? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
