Re: [linux-yocto] [PATCH] kernel/sched: Fix double free on invalid isolcpus/nohz_full params
On Fri, Aug 18, 2023 at 8:11 AM Paul Gortmaker
wrote:
>
> [[linux-yocto] [PATCH] kernel/sched: Fix double free on invalid
> isolcpus/nohz_full params] On 17/08/2023 (Thu 10:56) Adrian Cinal via
> lists.yoctoproject.org wrote:
>
> > A previous patch left behind a redundant call to free_bootmem_cpumask_var
> > possibly leading to a double free (once in the if-branch and once in the
> > unwind code at the end of the function) if the isolcpus= or nohz_full=
> > kernel command line parameters failed validation, cf.:
> >
> > https://lists.yoctoproject.org/g/linux-yocto/message/12797
>
> Once again, I wanted to know exactly how we got here. So here is how.
>
> The key to this issue is this commit from v5.18:
>
> commit 0cd3e59de1f53978873669c7c8225ec13e88c3ae
> Author: Frederic Weisbecker
> Date: Mon Feb 7 16:59:08 2022 +0100
>
> sched/isolation: Consolidate error handling
>
> Centralize the mask freeing and return value for the error path. This
> makes potential leaks more visible.
>
> Simple and common enough; it contains multiple instances of:
>
> - free_bootmem_cpumask_var(non_housekeeping_mask);
> - return 0;
> + goto free_non_housekeeping_mask;
>
> But from a kernel version persepective, it means we have an "old style" free
> and the "new style" free.
>
> The patch sent to lkml was for post 5.18 kernels, and used the new style,
> and was even documented as such in the 0/N (see asteriks):
>
> -
> This is a rebase and retest of two fixes I'd sent earlier[1].
>
> The rebase is required due to conflicts in my patch #1 and where Frederic
> updated the unwind code in housekeeping_setup in his series[2] and that series
> is now in sched/core of tip[3].
>
> So this update is against a baseline of ed3b362d54f0 found in sched/core as
> "sched/isolation: Split housekeeping cpumask per isolation features" in tip.
>
>**
> Changes amount to "return 0" ---> "goto out_free" and adding a nod to PaulM's
> observation that nohz_full w/o a cpuset is coming someday into the commit log.
>**
>
> [1]
> https://lore.kernel.org/all/20211206145950.10927-1-paul.gortma...@windriver.com/
> [2] https://lore.kernel.org/all/20220207155910.527133-1-frede...@kernel.org/
> [3] git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
> -
>
> https://lore.kernel.org/lkml/20220221182009.1283-1-paul.gortma...@windriver.com/
>
> Similarly, the submission for yocto for the v5.15 kernel (which was current
> at the time) used the old style and everything was fine.
>
> When yocto moved to v6.1, the uprev generally carries forward commits that
> have not been merged to mainline or declared obsolete.
>
> So the v5.15 old style commit was carried forward to v6.1, resulting in a
> mix of old and new style free. Not ideal, but still functionally correct.
>
> The double free risk comes from your change which deleted the "return 0"
> underneath the old free:
>
> https://lists.yoctoproject.org/g/linux-yocto/topic/99772129
>
> It shouldn't have done that, and I missed it in review.
>
> Bruce: the executive summary is that this delta fix is correct, and should
> be placed on the 6.1/6.4 branches that got the previous commit from Adrian.
>
> The yocto-kernel-cache can and should ignore all this churn, and simply
> contain the post v5.18 "new style" version of the commit sent to lkml:
>
> https://lore.kernel.org/lkml/20220221182009.1283-2-paul.gortma...@windriver.com/
>
Thanks Paul!
Everything should be sorted now, I've pushed changes and SRCREV bumps
will follow in a few days.
Bruce
> Thanks,
> Paul.
>
>
>
>
> >
> > Signed-off-by: Adrian Cinal
> > ---
> > kernel/sched/isolation.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> > index b97d6e05013d..7bebfdc42486 100644
> > --- a/kernel/sched/isolation.c
> > +++ b/kernel/sched/isolation.c
> > @@ -133,7 +133,6 @@ static int __init housekeeping_setup(char *str,
> > unsigned long flags)
> >
> > if (cpumask_empty(non_housekeeping_mask)) {
> > pr_info("housekeeping: kernel parameter 'nohz_full=' or
> > 'isolcpus=' has no valid CPUs.\n");
> > - free_bootmem_cpumask_var(non_housekeeping_mask);
> > goto free_non_housekeeping_mask;
> > }
> >
> > --
> > 2.41.0
> >
>
> >
> >
> >
>
--
- Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end
- "Use the force Harry" - Gandalf, Star Trek II
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#13003):
https://lists.yoctoproject.org/g/linux-yocto/message/13003
Mute This Topic: https://lists.yoctoproject.org/mt/100796885/21656
Group Owner: linux-yocto+ow...@lists.yoctoproject.org
Unsubscribe:
https://lists.yoctoproject.org/g/linux-yocto/leave/6687884/21656/624485779/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [linux-yocto] [PATCH] kernel/sched: Fix double free on invalid isolcpus/nohz_full params
[[linux-yocto] [PATCH] kernel/sched: Fix double free on invalid
isolcpus/nohz_full params] On 17/08/2023 (Thu 10:56) Adrian Cinal via
lists.yoctoproject.org wrote:
> A previous patch left behind a redundant call to free_bootmem_cpumask_var
> possibly leading to a double free (once in the if-branch and once in the
> unwind code at the end of the function) if the isolcpus= or nohz_full=
> kernel command line parameters failed validation, cf.:
>
> https://lists.yoctoproject.org/g/linux-yocto/message/12797
Once again, I wanted to know exactly how we got here. So here is how.
The key to this issue is this commit from v5.18:
commit 0cd3e59de1f53978873669c7c8225ec13e88c3ae
Author: Frederic Weisbecker
Date: Mon Feb 7 16:59:08 2022 +0100
sched/isolation: Consolidate error handling
Centralize the mask freeing and return value for the error path. This
makes potential leaks more visible.
Simple and common enough; it contains multiple instances of:
- free_bootmem_cpumask_var(non_housekeeping_mask);
- return 0;
+ goto free_non_housekeeping_mask;
But from a kernel version persepective, it means we have an "old style" free
and the "new style" free.
The patch sent to lkml was for post 5.18 kernels, and used the new style,
and was even documented as such in the 0/N (see asteriks):
-
This is a rebase and retest of two fixes I'd sent earlier[1].
The rebase is required due to conflicts in my patch #1 and where Frederic
updated the unwind code in housekeeping_setup in his series[2] and that series
is now in sched/core of tip[3].
So this update is against a baseline of ed3b362d54f0 found in sched/core as
"sched/isolation: Split housekeeping cpumask per isolation features" in tip.
**
Changes amount to "return 0" ---> "goto out_free" and adding a nod to PaulM's
observation that nohz_full w/o a cpuset is coming someday into the commit log.
**
[1]
https://lore.kernel.org/all/20211206145950.10927-1-paul.gortma...@windriver.com/
[2] https://lore.kernel.org/all/20220207155910.527133-1-frede...@kernel.org/
[3] git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
-
https://lore.kernel.org/lkml/20220221182009.1283-1-paul.gortma...@windriver.com/
Similarly, the submission for yocto for the v5.15 kernel (which was current
at the time) used the old style and everything was fine.
When yocto moved to v6.1, the uprev generally carries forward commits that
have not been merged to mainline or declared obsolete.
So the v5.15 old style commit was carried forward to v6.1, resulting in a
mix of old and new style free. Not ideal, but still functionally correct.
The double free risk comes from your change which deleted the "return 0"
underneath the old free:
https://lists.yoctoproject.org/g/linux-yocto/topic/99772129
It shouldn't have done that, and I missed it in review.
Bruce: the executive summary is that this delta fix is correct, and should
be placed on the 6.1/6.4 branches that got the previous commit from Adrian.
The yocto-kernel-cache can and should ignore all this churn, and simply
contain the post v5.18 "new style" version of the commit sent to lkml:
https://lore.kernel.org/lkml/20220221182009.1283-2-paul.gortma...@windriver.com/
Thanks,
Paul.
>
> Signed-off-by: Adrian Cinal
> ---
> kernel/sched/isolation.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
> index b97d6e05013d..7bebfdc42486 100644
> --- a/kernel/sched/isolation.c
> +++ b/kernel/sched/isolation.c
> @@ -133,7 +133,6 @@ static int __init housekeeping_setup(char *str, unsigned
> long flags)
>
> if (cpumask_empty(non_housekeeping_mask)) {
> pr_info("housekeeping: kernel parameter 'nohz_full=' or
> 'isolcpus=' has no valid CPUs.\n");
> - free_bootmem_cpumask_var(non_housekeeping_mask);
> goto free_non_housekeeping_mask;
> }
>
> --
> 2.41.0
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#12998):
https://lists.yoctoproject.org/g/linux-yocto/message/12998
Mute This Topic: https://lists.yoctoproject.org/mt/100796885/21656
Group Owner: linux-yocto+ow...@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/linux-yocto/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[linux-yocto] [PATCH] kernel/sched: Fix double free on invalid isolcpus/nohz_full params
A previous patch left behind a redundant call to free_bootmem_cpumask_var
possibly leading to a double free (once in the if-branch and once in the
unwind code at the end of the function) if the isolcpus= or nohz_full=
kernel command line parameters failed validation, cf.:
https://lists.yoctoproject.org/g/linux-yocto/message/12797
Signed-off-by: Adrian Cinal
---
kernel/sched/isolation.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/sched/isolation.c b/kernel/sched/isolation.c
index b97d6e05013d..7bebfdc42486 100644
--- a/kernel/sched/isolation.c
+++ b/kernel/sched/isolation.c
@@ -133,7 +133,6 @@ static int __init housekeeping_setup(char *str, unsigned
long flags)
if (cpumask_empty(non_housekeeping_mask)) {
pr_info("housekeeping: kernel parameter 'nohz_full=' or
'isolcpus=' has no valid CPUs.\n");
- free_bootmem_cpumask_var(non_housekeeping_mask);
goto free_non_housekeeping_mask;
}
--
2.41.0
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#12995):
https://lists.yoctoproject.org/g/linux-yocto/message/12995
Mute This Topic: https://lists.yoctoproject.org/mt/100796885/21656
Group Owner: linux-yocto+ow...@lists.yoctoproject.org
Unsubscribe: https://lists.yoctoproject.org/g/linux-yocto/unsub
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
