[v4] mm/vmalloc: replace BUG_ON to a simple if statement

[PATCH v4] mm/vmalloc: replace BUG_ON to a simple if statement

Posted by Hyunmin Lee 2 years, 7 months ago

As per the coding standards, in the event of an abnormal condition that
should not occur under normal circumstances, the kernel should attempt
recovery and proceed with execution, rather than halting the machine.

Specifically, in the alloc_vmap_area() function, use a simple if()
instead of using BUG_ON() halting the machine.

Co-Developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Co-Developed-by: Jeungwoo Yoo <casionwoo@gmail.com>
Co-Developed-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
Signed-off-by: Hyunmin Lee <hn.min.lee@gmail.com>
Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
Signed-off-by: Jeungwoo Yoo <casionwoo@gmail.com>
Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
---
v1->v2 : Add commit description
v2->v3 : Change WARN_ON() to if()
v3->v4 : Use only one if() for three conditions
---
 mm/vmalloc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 74afa2208558..3b7e8856be35 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1587,9 +1587,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
 	int purged = 0;
 	int ret;
 
-	BUG_ON(!size);
-	BUG_ON(offset_in_page(size));
-	BUG_ON(!is_power_of_2(align));
+	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
+		return ERR_PTR(-EINVAL);
 
 	if (unlikely(!vmap_initialized))
 		return ERR_PTR(-EBUSY);
-- 
2.25.1

Re: [PATCH v4] mm/vmalloc: replace BUG_ON to a simple if statement

Posted by Uladzislau Rezki 2 years, 7 months ago

On Wed, Feb 01, 2023 at 08:51:42PM +0900, Hyunmin Lee wrote:
> As per the coding standards, in the event of an abnormal condition that
> should not occur under normal circumstances, the kernel should attempt
> recovery and proceed with execution, rather than halting the machine.
> 
> Specifically, in the alloc_vmap_area() function, use a simple if()
> instead of using BUG_ON() halting the machine.
> 
> Co-Developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Co-Developed-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Co-Developed-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Signed-off-by: Hyunmin Lee <hn.min.lee@gmail.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
> v1->v2 : Add commit description
> v2->v3 : Change WARN_ON() to if()
> v3->v4 : Use only one if() for three conditions
> ---
>  mm/vmalloc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 74afa2208558..3b7e8856be35 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1587,9 +1587,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
>  	int purged = 0;
>  	int ret;
>  
> -	BUG_ON(!size);
> -	BUG_ON(offset_in_page(size));
> -	BUG_ON(!is_power_of_2(align));
> +	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
> +		return ERR_PTR(-EINVAL);
>  
>  	if (unlikely(!vmap_initialized))
>  		return ERR_PTR(-EBUSY);
> -- 
> 2.25.1
> 
Makes sense to me.

Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

--
Uladzislau Rezki

Re: [PATCH v4] mm/vmalloc: replace BUG_ON to a simple if statement

Posted by Mike Rapoport 2 years, 7 months ago

On Wed, Feb 01, 2023 at 08:51:42PM +0900, Hyunmin Lee wrote:
> As per the coding standards, in the event of an abnormal condition that
> should not occur under normal circumstances, the kernel should attempt
> recovery and proceed with execution, rather than halting the machine.
> 
> Specifically, in the alloc_vmap_area() function, use a simple if()
> instead of using BUG_ON() halting the machine.
> 
> Co-Developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Co-Developed-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Co-Developed-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Signed-off-by: Hyunmin Lee <hn.min.lee@gmail.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>

> ---
> v1->v2 : Add commit description
> v2->v3 : Change WARN_ON() to if()
> v3->v4 : Use only one if() for three conditions
> ---
>  mm/vmalloc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 74afa2208558..3b7e8856be35 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1587,9 +1587,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
>  	int purged = 0;
>  	int ret;
>  
> -	BUG_ON(!size);
> -	BUG_ON(offset_in_page(size));
> -	BUG_ON(!is_power_of_2(align));
> +	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
> +		return ERR_PTR(-EINVAL);
>  
>  	if (unlikely(!vmap_initialized))
>  		return ERR_PTR(-EBUSY);
> -- 
> 2.25.1
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v4] mm/vmalloc: replace BUG_ON to a simple if statement

Posted by Hyeonggon Yoo 2 years, 7 months ago

On Wed, Feb 01, 2023 at 08:51:42PM +0900, Hyunmin Lee wrote:
> As per the coding standards, in the event of an abnormal condition that
> should not occur under normal circumstances, the kernel should attempt
> recovery and proceed with execution, rather than halting the machine.
> 
> Specifically, in the alloc_vmap_area() function, use a simple if()
> instead of using BUG_ON() halting the machine.
> 
> Co-Developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Co-Developed-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Co-Developed-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Signed-off-by: Hyunmin Lee <hn.min.lee@gmail.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>
> ---
> v1->v2 : Add commit description
> v2->v3 : Change WARN_ON() to if()
> v3->v4 : Use only one if() for three conditions
> ---
>  mm/vmalloc.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 74afa2208558..3b7e8856be35 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1587,9 +1587,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
>  	int purged = 0;
>  	int ret;
>  
> -	BUG_ON(!size);
> -	BUG_ON(offset_in_page(size));
> -	BUG_ON(!is_power_of_2(align));
> +	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
> +		return ERR_PTR(-EINVAL);
>  
>  	if (unlikely(!vmap_initialized))
>  		return ERR_PTR(-EBUSY);

Reviewed-by: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Thanks!

> -- 
> 2.25.1

Regards,
Hyeonggon

Re: [PATCH v4] mm/vmalloc: replace BUG_ON to a simple if statement

Posted by Christophe Leroy 2 years, 7 months ago


Le 01/02/2023 à 12:51, Hyunmin Lee a écrit :
> As per the coding standards, in the event of an abnormal condition that
> should not occur under normal circumstances, the kernel should attempt
> recovery and proceed with execution, rather than halting the machine.
> 
> Specifically, in the alloc_vmap_area() function, use a simple if()
> instead of using BUG_ON() halting the machine.
> 
> Co-Developed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Co-Developed-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Co-Developed-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Signed-off-by: Hyunmin Lee <hn.min.lee@gmail.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: Jeungwoo Yoo <casionwoo@gmail.com>
> Signed-off-by: Sangyun Kim <sangyun.kim@snu.ac.kr>
> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
> v1->v2 : Add commit description
> v2->v3 : Change WARN_ON() to if()
> v3->v4 : Use only one if() for three conditions
> ---
>   mm/vmalloc.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 74afa2208558..3b7e8856be35 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1587,9 +1587,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
>   	int purged = 0;
>   	int ret;
>   
> -	BUG_ON(!size);
> -	BUG_ON(offset_in_page(size));
> -	BUG_ON(!is_power_of_2(align));
> +	if (unlikely(!size || offset_in_page(size) || !is_power_of_2(align)))
> +		return ERR_PTR(-EINVAL);
>   
>   	if (unlikely(!vmap_initialized))
>   		return ERR_PTR(-EBUSY);