[PATCH] sched/isolation: don't do unbounded chomp on bootarg string

[PATCH] sched/isolation: don't do unbounded chomp on bootarg string

[Paul Gortmaker]

After commit 3662daf023500dc084fa3b96f68a6f46179ddc73
("sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters")
the isolcpus= string is walked to skip over what might be future flag
comma separated additions.

However, there is a logic error, and so as can clearly be seen below, it
will ignore its own arg len and search to the end of the bootarg string.

 $ dmesg|grep isol
 Command line: BOOT_IMAGE=/boot/bzImage isolcpus=xyz pleasedontparseme=1 root=/dev/sda1 ro
 Kernel command line: BOOT_IMAGE=/boot/bzImage isolcpus=xyz pleasedontparseme=1 root=/dev/sda1 ro
 isolcpus: Skipped unknown flag xyz
 isolcpus: Invalid flag pleasedontparseme=1 root=/dev/sda1 ro

This happens because the flag "skip" code does an unconditional
increment, which skips over the '\0' check the loop body looks for. If
the isolcpus= happens to be the last bootarg, then you'd never notice?

So we only increment if the skipped flag is followed by a comma, as per
what the existing "continue" flag matching code does.

Note that isolcpus= was declared deprecated as of v4.15 (b0d40d2b22fe),
so we might want to revisit that if we are trying to future-proof it
as recently as a year ago for as yet unseen new flags.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Fixes: 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters")
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 5a6ea03f9882..9652dba7e938 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -188,7 +188,8 @@ static int __init housekeeping_isolcpus_setup(char *str)
 		}
 
 		pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
-		str++;
+		if (str[1] == ',')	/* above continue; match on "flag," */
+			str++;
 	}
 
 	/* Default behaviour for isolcpus without flags */
-- 
2.25.1

Re: [PATCH] sched/isolation: don't do unbounded chomp on bootarg string

[Paul Gortmaker]

[Re: [PATCH] sched/isolation: don't do unbounded chomp on bootarg string] On 19/04/2021 (Mon 16:38) Peter Xu wrote:

> On Sun, Apr 18, 2021 at 05:54:26PM -0400, Paul Gortmaker wrote:
> > After commit 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip
> > unknown sub-parameters") the isolcpus= string is walked to skip over what
> > might be any future flag comma separated additions.
> > 
> > However, there is a logic error, and so as can clearly be seen below, it
> > will ignore its own arg len and search to the end of the bootarg string.
> > 
> >  $ dmesg|grep isol
> >  Command line: BOOT_IMAGE=/boot/bzImage isolcpus=xyz pleasedontparseme=1 root=/dev/sda1 ro
> >  isolcpus: Skipped unknown flag xyz
> >  isolcpus: Invalid flag pleasedontparseme=1 root=/dev/sda1 ro
> > 
> > This happens because the flag "skip" code does an unconditional
> > increment, which skips over the '\0' check the loop body looks for. If
> > the isolcpus= happens to be the last bootarg, then you'd never notice?
> > 
> > So we only increment if the skipped flag is followed by a comma, as per
> > what the existing "continue" flag matching code does.
> > 
> > Note that isolcpus= was declared deprecated as of v4.15 (b0d40d2b22fe),
> > so we might want to revisit that if we are trying to future-proof it
> > as recently as a year ago for as yet unseen new flags.
> 
> Thanks for report the issue.
> 
> Is cpuset going to totally replace "isolcpus="?  It seems most hk_flags will be
> handled by nohz_full=, and HK_FLAG_DOMAIN can be done by cpuset.  However it
> seems still the only place to set the new flag HK_FLAG_MANAGED_IRQ.  If one day
> we'll finally obsolete isolcpus= we may need to think about where to put it?

It is probably overly optimistic that we'll ever get to retire it, based
on past history of trying to remove old use cases out of the kernel.

> 
> When I looked at it, I also noticed I see no caller to set HK_FLAG_SCHED at
> all.  Is it really used anywhere?

I guess that would be a Frederic question.  In the commit log of
de201559df (back in 2017) he did note flags might be too fine grained...

> 
> Regarding this patch...
> 
> > 
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Frederic Weisbecker <frederic@kernel.org>
> > Fixes: 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters")
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > 
> > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > index 5a6ea03f9882..9652dba7e938 100644
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -188,7 +188,8 @@ static int __init housekeeping_isolcpus_setup(char *str)
> >  		}
> >  
> >  		pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
> > -		str++;
> > +		if (str[1] == ',')	/* above continue; match on "flag," */
> 
> .. wondering why it is not "str[0] == ','" instead?

Apparently I need to add some more convenient self tests, so I don't
manage to hurt myself with string handling.  Thanks for spotting it.

Paul.
--

> 
> Thanks,
> 
> > +			str++;
> >  	}
> >  
> >  	/* Default behaviour for isolcpus without flags */
> > -- 
> > 2.25.1
> > 
> 
> -- 
> Peter Xu
> 

Re: [PATCH] sched/isolation: don't do unbounded chomp on bootarg string

[Peter Xu]

On Sun, Apr 18, 2021 at 05:54:26PM -0400, Paul Gortmaker wrote:
> After commit 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip
> unknown sub-parameters") the isolcpus= string is walked to skip over what
> might be any future flag comma separated additions.
> 
> However, there is a logic error, and so as can clearly be seen below, it
> will ignore its own arg len and search to the end of the bootarg string.
> 
>  $ dmesg|grep isol
>  Command line: BOOT_IMAGE=/boot/bzImage isolcpus=xyz pleasedontparseme=1 root=/dev/sda1 ro
>  isolcpus: Skipped unknown flag xyz
>  isolcpus: Invalid flag pleasedontparseme=1 root=/dev/sda1 ro
> 
> This happens because the flag "skip" code does an unconditional
> increment, which skips over the '\0' check the loop body looks for. If
> the isolcpus= happens to be the last bootarg, then you'd never notice?
> 
> So we only increment if the skipped flag is followed by a comma, as per
> what the existing "continue" flag matching code does.
> 
> Note that isolcpus= was declared deprecated as of v4.15 (b0d40d2b22fe),
> so we might want to revisit that if we are trying to future-proof it
> as recently as a year ago for as yet unseen new flags.

Thanks for report the issue.

Is cpuset going to totally replace "isolcpus="?  It seems most hk_flags will be
handled by nohz_full=, and HK_FLAG_DOMAIN can be done by cpuset.  However it
seems still the only place to set the new flag HK_FLAG_MANAGED_IRQ.  If one day
we'll finally obsolete isolcpus= we may need to think about where to put it?

When I looked at it, I also noticed I see no caller to set HK_FLAG_SCHED at
all.  Is it really used anywhere?

Regarding this patch...

> 
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Frederic Weisbecker <frederic@kernel.org>
> Fixes: 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters")
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> 
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index 5a6ea03f9882..9652dba7e938 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -188,7 +188,8 @@ static int __init housekeeping_isolcpus_setup(char *str)
>  		}
>  
>  		pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
> -		str++;
> +		if (str[1] == ',')	/* above continue; match on "flag," */

.. wondering why it is not "str[0] == ','" instead?

Thanks,

> +			str++;
>  	}
>  
>  	/* Default behaviour for isolcpus without flags */
> -- 
> 2.25.1
> 

-- 
Peter Xu

[PATCH] sched/isolation: don't do unbounded chomp on bootarg string

[Paul Gortmaker]

After commit 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip
unknown sub-parameters") the isolcpus= string is walked to skip over what
might be any future flag comma separated additions.

However, there is a logic error, and so as can clearly be seen below, it
will ignore its own arg len and search to the end of the bootarg string.

 $ dmesg|grep isol
 Command line: BOOT_IMAGE=/boot/bzImage isolcpus=xyz pleasedontparseme=1 root=/dev/sda1 ro
 isolcpus: Skipped unknown flag xyz
 isolcpus: Invalid flag pleasedontparseme=1 root=/dev/sda1 ro

This happens because the flag "skip" code does an unconditional
increment, which skips over the '\0' check the loop body looks for. If
the isolcpus= happens to be the last bootarg, then you'd never notice?

So we only increment if the skipped flag is followed by a comma, as per
what the existing "continue" flag matching code does.

Note that isolcpus= was declared deprecated as of v4.15 (b0d40d2b22fe),
so we might want to revisit that if we are trying to future-proof it
as recently as a year ago for as yet unseen new flags.

Cc: Peter Xu <peterx@redhat.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Frederic Weisbecker <frederic@kernel.org>
Fixes: 3662daf02350 ("sched/isolation: Allow "isolcpus=" to skip unknown sub-parameters")
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index 5a6ea03f9882..9652dba7e938 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -188,7 +188,8 @@ static int __init housekeeping_isolcpus_setup(char *str)
 		}
 
 		pr_info("isolcpus: Skipped unknown flag %.*s\n", len, par);
-		str++;
+		if (str[1] == ',')	/* above continue; match on "flag," */
+			str++;
 	}
 
 	/* Default behaviour for isolcpus without flags */
-- 
2.25.1