[PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()

[PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()

[Anshuman Khandual]

Currently kernel command line 'stack_guard_gap=', which does not provide an
explicit pages gap value, still assigns 0 to 'stack_guard_gap', which would
not have been expected. In such cases it should just retain the default gap
value (DEFAULT_STACK_GUARD_GAP). Instead let's assert a positive value for
input gap pages before proceeding any further. While here, this tightens up
cmdline_parse_stack_guard_gap() for other scenarios, where the command line
parameter 'stack_guard_gap' is not successfully handled as expected.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-mm@kvack.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Depends on the following patch.

https://lore.kernel.org/all/20230828035248.678960-1-anshuman.khandual@arm.com/

 mm/mmap.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 8679750333bb..adaa81d95518 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2122,16 +2122,19 @@ int expand_downwards(struct vm_area_struct *vma, unsigned long address)
 /* enforced gap between the expanding stack and other mappings. */
 unsigned long stack_guard_gap = DEFAULT_STACK_GUARD_GAP;
 
-static int __init cmdline_parse_stack_guard_gap(char *p)
+static int __init cmdline_parse_stack_guard_gap(char *str)
 {
 	unsigned long val;
-	char *endptr;
 
-	val = simple_strtoul(p, &endptr, 10);
-	if (!*endptr)
-		stack_guard_gap = val << PAGE_SHIFT;
+	if (!str)
+		return 0;
 
-	return 1;
+	val = simple_strtoul(str, &str, 10);
+	if (!*str && val) {
+		stack_guard_gap = val << PAGE_SHIFT;
+		return 1;
+	}
+	return 0;
 }
 __setup("stack_guard_gap=", cmdline_parse_stack_guard_gap);
 
-- 
2.30.2

Re: [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()

[Matthew Wilcox]

On Mon, Aug 28, 2023 at 10:52:12AM +0530, Anshuman Khandual wrote:
> -static int __init cmdline_parse_stack_guard_gap(char *p)
> +static int __init cmdline_parse_stack_guard_gap(char *str)
>  {
>  	unsigned long val;
> -	char *endptr;
>  
> -	val = simple_strtoul(p, &endptr, 10);
> -	if (!*endptr)
> -		stack_guard_gap = val << PAGE_SHIFT;
> +	if (!str)
> +		return 0;

Please explain how this function can be called with a NULL pointer.

> -	return 1;
> +	val = simple_strtoul(str, &str, 10);
> +	if (!*str && val) {
> +		stack_guard_gap = val << PAGE_SHIFT;
> +		return 1;
> +	}
> +	return 0;
>  }

Now you've removed the abillity for someone to say stack_guard_gap=0,
which seems potentially useful.

Re: [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()

[Anshuman Khandual]

On 8/29/23 18:21, Matthew Wilcox wrote:
> On Mon, Aug 28, 2023 at 10:52:12AM +0530, Anshuman Khandual wrote:
>> -static int __init cmdline_parse_stack_guard_gap(char *p)
>> +static int __init cmdline_parse_stack_guard_gap(char *str)
>>  {
>>  	unsigned long val;
>> -	char *endptr;
>>  
>> -	val = simple_strtoul(p, &endptr, 10);
>> -	if (!*endptr)
>> -		stack_guard_gap = val << PAGE_SHIFT;
>> +	if (!str)
>> +		return 0;
> 
> Please explain how this function can be called with a NULL pointer.

This is an additional check just in case. We have similar constructs
in the following __setup() functions as well.

__setup("hashdist=", set_hashdist)
__setup("numa_balancing=", setup_numabalancing)
__setup("transparent_hugepage=", setup_transparent_hugepage)

Also it might be a better to warn, when returning unhandled with 0
like in those scenarios.

> 
>> -	return 1;
>> +	val = simple_strtoul(str, &str, 10);
>> +	if (!*str && val) {
>> +		stack_guard_gap = val << PAGE_SHIFT;
>> +		return 1;
>> +	}
>> +	return 0;
>>  }
> 
> Now you've removed the abillity for someone to say stack_guard_gap=0,
> which seems potentially useful.

In that case, should the following two scenarios be differentiated ?

* stack_guard_gap=	- Retains DEFAULT_STACK_GUARD_GAP
* stack_guard_gap=0	- Changes to 0 pages

Re: [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()

[Matthew Wilcox]

On Wed, Aug 30, 2023 at 08:47:12AM +0530, Anshuman Khandual wrote:
> 
> 
> On 8/29/23 18:21, Matthew Wilcox wrote:
> > On Mon, Aug 28, 2023 at 10:52:12AM +0530, Anshuman Khandual wrote:
> >> -static int __init cmdline_parse_stack_guard_gap(char *p)
> >> +static int __init cmdline_parse_stack_guard_gap(char *str)
> >>  {
> >>  	unsigned long val;
> >> -	char *endptr;
> >>  
> >> -	val = simple_strtoul(p, &endptr, 10);
> >> -	if (!*endptr)
> >> -		stack_guard_gap = val << PAGE_SHIFT;
> >> +	if (!str)
> >> +		return 0;
> > 
> > Please explain how this function can be called with a NULL pointer.
> 
> This is an additional check just in case. We have similar constructs
> in the following __setup() functions as well.

In case of _what_?  Somebody goes insane and decides to start calling
__setup functions with NULL pointers?  We don't test "Did the VFS call
this filesystem with a NULL inode pointer" because that would make
ZERO sense.  Defensive programming doesn't need to defend against an
insane kernel core.

> __setup("hashdist=", set_hashdist)
> __setup("numa_balancing=", setup_numabalancing)
> __setup("transparent_hugepage=", setup_transparent_hugepage)

Those should have this stupid NULL check removed.

> > Now you've removed the abillity for someone to say stack_guard_gap=0,
> > which seems potentially useful.
> 
> In that case, should the following two scenarios be differentiated ?
> 
> * stack_guard_gap=	- Retains DEFAULT_STACK_GUARD_GAP
> * stack_guard_gap=0	- Changes to 0 pages

I don't know.  You appear to have run into the scenario where
'stack_guard_gap=' was specified.  What did you expect it to do?