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

On Wed, Aug 19, 2020 at 10:39:34PM +0300, Tomer Samara wrote:

Remove BUG() at ion_sytem_heap.c and error handling to:

• free_buffer_page
• alloc_buffer_page

this fix the following checkpatch 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_system_heap.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c index eac0632ab4e8..56d53268b82c 100644 --- a/drivers/staging/android/ion/ion_system_heap.c +++ b/drivers/staging/android/ion/ion_system_heap.c @@ -30,7 +30,7 @@ static int order_to_index(unsigned int order) for (i = 0; i < NUM_ORDERS; i++) if (order == orders[i]) return i;

• BUG();

• return -1;

}

Just delete the BUG() and put a comment that /* This is impossible. */ so that reviewers know that we never return -1.

I suspect that there are some static analysis tools which might complain about this -1 return. But those tools are pretty crap. Never change code just to make the tools happy.

regards, dan carpenter