Bug in the error handling in TTMs pool implementation

[Karolina Stolarek]

On 24.11.2023 16:53, Christian König wrote:
> @Karolina do you of hand know a way how we could exercise this in a TTM 
> unit test? Basically we would need to redirect the alloc_pages_node() 
> symbol to an unit test internal function and let it return an error (or 
> something like that).

Do I understand correctly that you want to mock NULL from 
alloc_pages_node() in ttm_pool_alloc_page()? I had a quick look at it, 
and found a(n ugly) way to do it.

KUnit provides an API to redirect calls using KUNIT_STATIC_STUB_REDIRECT 
that can be registered in the original function and then overridden in 
the test. You can read more about the mechanism in [1].

The problem is that we'd need to introduce a wrapper call for 
alloc_pages_node() in TTM and expose it, so the function can be 
substituted during the test. We can't directly modify alloc_pages_node() 
due to cyclic dependencies caused by kunit/static_stub.h.

For example, in ttm_pool.c, we'd have:

+struct page *ttm_alloc_pages_node(int nid, gfp_t gfp_mask,
+                                 unsigned int order)
+{
+       KUNIT_STATIC_STUB_REDIRECT(ttm_alloc_pages_node, nid, gfp_mask,
order);
+       return alloc_pages_node(nid, gfp_mask, order);
+}
+EXPORT_SYMBOL(ttm_alloc_pages_node);
+
  /* Allocate pages of size 1 << order with the given gfp_flags */
  static struct page *ttm_pool_alloc_page(struct ttm_pool *pool, gfp_t
gfp_flags,
                                         unsigned int order)
(...)
         if (!pool->use_dma_alloc) {
-               p = alloc_pages_node(pool->nid, gfp_flags, order);
+               p = ttm_alloc_pages_node(pool->nid, gfp_flags, order);
                 if (p)
                         p->private = order;
                 return p;

And in the test we would say:

+static struct page *fake_ttm_alloc_pages_node(int nid, gfp_t gfp_mask,
+                                             unsigned int order)
+{
+       return NULL;
+}
+
+static void ttm_pool_alloc_failed(struct kunit *test)
+{
(...)
+       kunit_activate_static_stub(test, ttm_alloc_pages_node,
+                                  fake_ttm_alloc_pages_node);
+       err = ttm_pool_alloc(pool, tt, &simple_ctx);
+       kunit_deactivate_static_stub(test, ttm_alloc_pages_node);

Christian, what do you think?

All the best,
Karolina

---------------------------------------------------------
[1] - 
https://lore.kernel.org/all/20230128074918.1180523-1-davidgow@google.com/