* [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()
@ 2023-08-28 5:22 Anshuman Khandual
2023-08-29 12:51 ` Matthew Wilcox
0 siblings, 1 reply; 4+ messages in thread
From: Anshuman Khandual @ 2023-08-28 5:22 UTC (permalink / raw)
To: linux-mm; +Cc: Anshuman Khandual, Andrew Morton, linux-kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()
2023-08-28 5:22 [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap() Anshuman Khandual
@ 2023-08-29 12:51 ` Matthew Wilcox
2023-08-30 3:17 ` Anshuman Khandual
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2023-08-29 12:51 UTC (permalink / raw)
To: Anshuman Khandual; +Cc: linux-mm, Andrew Morton, linux-kernel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()
2023-08-29 12:51 ` Matthew Wilcox
@ 2023-08-30 3:17 ` Anshuman Khandual
2023-08-30 3:24 ` Matthew Wilcox
0 siblings, 1 reply; 4+ messages in thread
From: Anshuman Khandual @ 2023-08-30 3:17 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-mm, Andrew Morton, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap()
2023-08-30 3:17 ` Anshuman Khandual
@ 2023-08-30 3:24 ` Matthew Wilcox
0 siblings, 0 replies; 4+ messages in thread
From: Matthew Wilcox @ 2023-08-30 3:24 UTC (permalink / raw)
To: Anshuman Khandual; +Cc: linux-mm, Andrew Morton, linux-kernel
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-08-30 3:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-28 5:22 [PATCH] mm/mmap: Tighten up cmdline_parse_stack_guard_gap() Anshuman Khandual
2023-08-29 12:51 ` Matthew Wilcox
2023-08-30 3:17 ` Anshuman Khandual
2023-08-30 3:24 ` Matthew Wilcox
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.