On Fri, Aug 18, 2023 at 8:11 AM Paul Gortmaker
<paul.gortma...@windriver.com> 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 <frede...@kernel.org>
> 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 <adriancin...@gmail.com>
> > ---
> > 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]
-=-=-=-=-=-=-=-=-=-=-=-
