On Fri, Aug 18, 2023 at 8:11 AM Paul Gortmaker <[email protected]> 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 <[email protected]> > 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/[email protected]/ > [2] https://lore.kernel.org/all/[email protected]/ > [3] git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git > ------------- > > https://lore.kernel.org/lkml/[email protected]/ > > 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/[email protected]/ >
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 <[email protected]> > > --- > > 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: [email protected] Unsubscribe: https://lists.yoctoproject.org/g/linux-yocto/leave/6687884/21656/624485779/xyzzy [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
