| Message ID | 2e6c71ad168f92170ef856922b9a0c8dd0f85e11.1597865771.git.tomersamara98@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: android: Remove BUG/BUG_ONs | expand |
On Wed, Aug 19, 2020 at 10:38:47PM +0300, Tomer Samara wrote: > BUG_ON() is removed at ion_page_pool.c and add error handleing to > ion_page_pool_shrink > > Fixes the following issue: > Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON(). > > Signed-off-by: Tomer Samara <tomersamara98@gmail.com> > --- > drivers/staging/android/ion/ion_page_pool.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c > index 0198b886d906..ae2bc57bcbe8 100644 > --- a/drivers/staging/android/ion/ion_page_pool.c > +++ b/drivers/staging/android/ion/ion_page_pool.c > @@ -46,11 +46,13 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) > struct page *page; > > if (high) { > - BUG_ON(!pool->high_count); > + if (!pool->high_count) > + return NULL; I looked at the callers and it's trivial to verify that these conditions are impossible. Just delete the BUG_ON() checks. > page = list_first_entry(&pool->high_items, struct page, lru); > pool->high_count--; > } else { > - BUG_ON(!pool->low_count); > + if (!pool->low_count) > + return NULL; > page = list_first_entry(&pool->low_items, struct page, lru); > pool->low_count--; > } > @@ -65,7 +67,8 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) > { > struct page *page = NULL; > > - BUG_ON(!pool); > + if (!pool) > + return NULL; This one is slightly harder to verify... But really I would prefer that we just deleted it as well. If we had a NULL dereference here then that would give a pretty straight forward stack trace to debug. > > mutex_lock(&pool->mutex); > if (pool->high_count) > @@ -82,7 +85,8 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) > > void ion_page_pool_free(struct ion_page_pool *pool, struct page *page) > { > - BUG_ON(pool->order != compound_order(page)); > + if (pool->order != compound_order(page)) > + return; Is returning really the correct way to handle this bug? I suggest, just change BUG_ON() to a WARN_ON(). > > ion_page_pool_add(pool, page); > } > @@ -124,6 +128,8 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask, > break; > } > mutex_unlock(&pool->mutex); > + if (!page) > + break; This change is no longer required if we delete the changes earlier as I suggest. This change illustrates how when we start handling impossible conditions then we just have to keep on imagining more and more impossible conditions. When we start trying to write code for situations which we know are impossible that is an unending task. > ion_page_pool_free_pages(pool, page); > freed += (1 << pool->order); > } regards, dan carpenter
diff --git a/drivers/staging/android/ion/ion_page_pool.c b/drivers/staging/android/ion/ion_page_pool.c index 0198b886d906..ae2bc57bcbe8 100644 --- a/drivers/staging/android/ion/ion_page_pool.c +++ b/drivers/staging/android/ion/ion_page_pool.c @@ -46,11 +46,13 @@ static struct page *ion_page_pool_remove(struct ion_page_pool *pool, bool high) struct page *page; if (high) { - BUG_ON(!pool->high_count); + if (!pool->high_count) + return NULL; page = list_first_entry(&pool->high_items, struct page, lru); pool->high_count--; } else { - BUG_ON(!pool->low_count); + if (!pool->low_count) + return NULL; page = list_first_entry(&pool->low_items, struct page, lru); pool->low_count--; } @@ -65,7 +67,8 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) { struct page *page = NULL; - BUG_ON(!pool); + if (!pool) + return NULL; mutex_lock(&pool->mutex); if (pool->high_count) @@ -82,7 +85,8 @@ struct page *ion_page_pool_alloc(struct ion_page_pool *pool) void ion_page_pool_free(struct ion_page_pool *pool, struct page *page) { - BUG_ON(pool->order != compound_order(page)); + if (pool->order != compound_order(page)) + return; ion_page_pool_add(pool, page); } @@ -124,6 +128,8 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask, break; } mutex_unlock(&pool->mutex); + if (!page) + break; ion_page_pool_free_pages(pool, page); freed += (1 << pool->order); }
BUG_ON() is removed at ion_page_pool.c and add error handleing to ion_page_pool_shrink Fixes the following issue: Avoid crashing the kernel - try using WARN_ON & recovery code ratherthan BUG() or BUG_ON(). Signed-off-by: Tomer Samara <tomersamara98@gmail.com> --- drivers/staging/android/ion/ion_page_pool.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)