[PATCH v3 0/2] staging: android: Remove BUG/BUG_ONs

[PATCH v3 0/2] staging: android: Remove BUG/BUG_ONs

[Tomer Samara]

Remove BUG/BUG_ONs from androind/ion allocator and add error handling to
calling functions

Tomer Samara (2):
  staging: android: Remove BUG_ON from ion_page_pool.c
  staging: android: Remove BUG from ion_system_heap.c

 drivers/staging/android/ion/ion_page_pool.c   | 14 ++++++++++----
 drivers/staging/android/ion/ion_system_heap.c | 15 ++++++++++++---
 2 files changed, 22 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c

[Tomer Samara]

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;
 		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);
 	}
-- 
2.25.1

Re: [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c

[Dan Carpenter]

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

[PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c

[Tomer Samara]

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;
 }
 
@@ -48,8 +48,13 @@ static struct page *alloc_buffer_page(struct ion_system_heap *heap,
 				      struct ion_buffer *buffer,
 				      unsigned long order)
 {
-	struct ion_page_pool *pool = heap->pools[order_to_index(order)];
+	struct ion_page_pool *pool;
+	int index = order_to_index(order);
 
+	if (index < 0)
+		return NULL;
+
+	pool = heap->pools[index];
 	return ion_page_pool_alloc(pool);
 }
 
@@ -58,6 +63,7 @@ static void free_buffer_page(struct ion_system_heap *heap,
 {
 	struct ion_page_pool *pool;
 	unsigned int order = compound_order(page);
+	int index;
 
 	/* go to system */
 	if (buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE) {
@@ -65,8 +71,11 @@ static void free_buffer_page(struct ion_system_heap *heap,
 		return;
 	}
 
-	pool = heap->pools[order_to_index(order)];
+	index = order_to_index(order);
+	if (index < 0)
+		return;
 
+	pool = heap->pools[index];
 	ion_page_pool_free(pool, page);
 }
 
-- 
2.25.1

Re: [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c

[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