| Message ID | 20230828052212.748872-1-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/mmap: Tighten up cmdline_parse_stack_guard_gap() | expand |
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.
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
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?
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);
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(-)