LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/2] staging: android: Remove BUG/BUG_ONs
@ 2020-08-19 19:37 Tomer Samara
  2020-08-19 19:38 ` [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c Tomer Samara
  2020-08-19 19:39 ` [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c Tomer Samara
  0 siblings, 2 replies; 5+ messages in thread
From: Tomer Samara @ 2020-08-19 19:37 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Arve Hjønnevåg, Riley Andrews, Laura Abbott,
	Sumit Semwal, Todd Kjos, Martijn Coenen, Joel Fernandes,
	Christian Brauner, Hridya Valsaraju, Suren Baghdasaryan, devel,
	dri-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c
  2020-08-19 19:37 [PATCH v3 0/2] staging: android: Remove BUG/BUG_ONs Tomer Samara
@ 2020-08-19 19:38 ` Tomer Samara
  2020-08-21 13:15   ` Dan Carpenter
  2020-08-19 19:39 ` [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c Tomer Samara
  1 sibling, 1 reply; 5+ messages in thread
From: Tomer Samara @ 2020-08-19 19:38 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Arve Hjønnevåg, Riley Andrews, Laura Abbott,
	Sumit Semwal, Todd Kjos, Martijn Coenen, Joel Fernandes,
	Christian Brauner, Hridya Valsaraju, Suren Baghdasaryan, devel,
	dri-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c
  2020-08-19 19:37 [PATCH v3 0/2] staging: android: Remove BUG/BUG_ONs Tomer Samara
  2020-08-19 19:38 ` [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c Tomer Samara
@ 2020-08-19 19:39 ` Tomer Samara
  2020-08-21 13:32   ` Dan Carpenter
  1 sibling, 1 reply; 5+ messages in thread
From: Tomer Samara @ 2020-08-19 19:39 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Arve Hjønnevåg, Riley Andrews, Laura Abbott,
	Sumit Semwal, Todd Kjos, Martijn Coenen, Joel Fernandes,
	Christian Brauner, Hridya Valsaraju, Suren Baghdasaryan, devel,
	dri-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c
  2020-08-19 19:38 ` [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c Tomer Samara
@ 2020-08-21 13:15   ` Dan Carpenter
  0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-08-21 13:15 UTC (permalink / raw)
  To: Tomer Samara
  Cc: Greg Kroah-Hartman, devel, Todd Kjos, Suren Baghdasaryan,
	Riley Andrews, dri-devel, linux-kernel, Hridya Valsaraju,
	Arve Hjønnevåg, Joel Fernandes, Laura Abbott,
	Martijn Coenen, Sumit Semwal, Christian Brauner

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c
  2020-08-19 19:39 ` [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c Tomer Samara
@ 2020-08-21 13:32   ` Dan Carpenter
  0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2020-08-21 13:32 UTC (permalink / raw)
  To: Tomer Samara
  Cc: Greg Kroah-Hartman, devel, Todd Kjos, Suren Baghdasaryan,
	Riley Andrews, dri-devel, linux-kernel, Hridya Valsaraju,
	Arve Hjønnevåg, Joel Fernandes, Laura Abbott,
	Martijn Coenen, Sumit Semwal, Christian Brauner

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19 19:37 [PATCH v3 0/2] staging: android: Remove BUG/BUG_ONs Tomer Samara
2020-08-19 19:38 ` [PATCH v3 1/2] staging: android: Remove BUG_ON from ion_page_pool.c Tomer Samara
2020-08-21 13:15   ` Dan Carpenter
2020-08-19 19:39 ` [PATCH v3 2/2] staging: android: Remove BUG from ion_system_heap.c Tomer Samara
2020-08-21 13:32   ` Dan Carpenter

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lkml.kernel.org/lkml/9 lkml/git/9.git
	git clone --mirror https://lkml.kernel.org/lkml/10 lkml/git/10.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git