| Message ID | 20180621030714.10368-1-baijiaju1990@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 21, 2018 at 11:07:14AM +0800, Jia-Ju Bai wrote: > The kernel may sleep with holding a spinlock. > The function call path (from bottom to top) in Linux-4.16.7 is: > > [FUNC] remove_element(GFP_KERNEL) > mm/mempool.c, 250: remove_element in mempool_resize > mm/mempool.c, 247: _raw_spin_lock_irqsave in mempool_resize > > To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. > > This bug is found by my static analysis tool (DSAC-2) and checked by > my code review. But ... we don't use the flags argument. static void *remove_element(mempool_t *pool, gfp_t flags) { void *element = pool->elements[--pool->curr_nr]; BUG_ON(pool->curr_nr < 0); kasan_unpoison_element(pool, element, flags); check_element(pool, element); return element; } ... static void kasan_unpoison_element(mempool_t *pool, void *element, gfp_t flags) { if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc) kasan_unpoison_slab(element); if (pool->alloc == mempool_alloc_pages) kasan_alloc_pages(element, (unsigned long)pool->pool_data); } So the correct patch would just remove this argument to remove_element() and kasan_unpoison_element()?
On 2018/6/21 11:38, Matthew Wilcox wrote: > On Thu, Jun 21, 2018 at 11:07:14AM +0800, Jia-Ju Bai wrote: >> The kernel may sleep with holding a spinlock. >> The function call path (from bottom to top) in Linux-4.16.7 is: >> >> [FUNC] remove_element(GFP_KERNEL) >> mm/mempool.c, 250: remove_element in mempool_resize >> mm/mempool.c, 247: _raw_spin_lock_irqsave in mempool_resize >> >> To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. >> >> This bug is found by my static analysis tool (DSAC-2) and checked by >> my code review. > But ... we don't use the flags argument. > > static void *remove_element(mempool_t *pool, gfp_t flags) > { > void *element = pool->elements[--pool->curr_nr]; > > BUG_ON(pool->curr_nr < 0); > kasan_unpoison_element(pool, element, flags); > check_element(pool, element); > return element; > } > > ... > > static void kasan_unpoison_element(mempool_t *pool, void *element, gfp_t flags) > { > if (pool->alloc == mempool_alloc_slab || pool->alloc == mempool_kmalloc) > kasan_unpoison_slab(element); > if (pool->alloc == mempool_alloc_pages) > kasan_alloc_pages(element, (unsigned long)pool->pool_data); > } > > So the correct patch would just remove this argument to remove_element() and > kasan_unpoison_element()? Yes, I also find this. I can submit a patch that removes the flag in: Definitions of kasan_unpoison_element() and remove_element() Three calls to remove_element() and one call to kasan_unpoison_element() in mempool.c. Do you think it is okay? Best wishes, Jia-Ju Bai
On Thu, Jun 21, 2018 at 5:46 AM, Jia-Ju Bai <baijiaju1990@gmail.com> wrote: > On 2018/6/21 11:38, Matthew Wilcox wrote: >> >> On Thu, Jun 21, 2018 at 11:07:14AM +0800, Jia-Ju Bai wrote: >>> >>> The kernel may sleep with holding a spinlock. >>> The function call path (from bottom to top) in Linux-4.16.7 is: >>> >>> [FUNC] remove_element(GFP_KERNEL) >>> mm/mempool.c, 250: remove_element in mempool_resize >>> mm/mempool.c, 247: _raw_spin_lock_irqsave in mempool_resize >>> >>> To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. >>> >>> This bug is found by my static analysis tool (DSAC-2) and checked by >>> my code review. >> >> But ... we don't use the flags argument. >> >> static void *remove_element(mempool_t *pool, gfp_t flags) >> { >> void *element = pool->elements[--pool->curr_nr]; >> >> BUG_ON(pool->curr_nr < 0); >> kasan_unpoison_element(pool, element, flags); >> check_element(pool, element); >> return element; >> } >> >> ... >> >> static void kasan_unpoison_element(mempool_t *pool, void *element, gfp_t >> flags) >> { >> if (pool->alloc == mempool_alloc_slab || pool->alloc == >> mempool_kmalloc) >> kasan_unpoison_slab(element); >> if (pool->alloc == mempool_alloc_pages) >> kasan_alloc_pages(element, (unsigned >> long)pool->pool_data); >> } >> >> So the correct patch would just remove this argument to remove_element() >> and >> kasan_unpoison_element()? > > > Yes, I also find this. > I can submit a patch that removes the flag in: > Definitions of kasan_unpoison_element() and remove_element() > Three calls to remove_element() and one call to kasan_unpoison_element() in > mempool.c. > > Do you think it is okay? Hi Jia-Ju, Removing an unused argument within a single file looks good to me.
diff --git a/mm/mempool.c b/mm/mempool.c index 5c9dce34719b..d33bd5d622e7 100644 --- a/mm/mempool.c +++ b/mm/mempool.c @@ -247,7 +247,7 @@ int mempool_resize(mempool_t *pool, int new_min_nr) spin_lock_irqsave(&pool->lock, flags); if (new_min_nr <= pool->min_nr) { while (new_min_nr < pool->curr_nr) { - element = remove_element(pool, GFP_KERNEL); + element = remove_element(pool, GFP_ATOMIC); spin_unlock_irqrestore(&pool->lock, flags); pool->free(element, pool->pool_data); spin_lock_irqsave(&pool->lock, flags);
The kernel may sleep with holding a spinlock. The function call path (from bottom to top) in Linux-4.16.7 is: [FUNC] remove_element(GFP_KERNEL) mm/mempool.c, 250: remove_element in mempool_resize mm/mempool.c, 247: _raw_spin_lock_irqsave in mempool_resize To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC. This bug is found by my static analysis tool (DSAC-2) and checked by my code review. Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com> --- mm/mempool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)