From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 986D62222B7 for ; Sun, 18 Jan 2026 06:12:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768716720; cv=none; b=ErDyjb5ytP88lM7NmXmLgj+5N4LJy8Q52OQGpLK3usZZg3oQI5A730yp97AWyQTpw5PenH5DUEDk322OHlp27p7h5Vl4Tx//J4OkvRlSJDvxYMlJaGemDKqcpozjyA85+gIQPSpCtdMmqm4JuW4Zq0OMwD2Dhs2qOCm5LPECFFE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768716720; c=relaxed/simple; bh=XQZz9Nu+u7TdwrpS5yQTxTW/ej0dkCDpS+WPtv5Iudo=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TupZhkbg/jOIYL7137ToFQLyU0nS3/FHa4L4lgVph1UXE0EyGg1puBoGf3Jb8fRuwgvjHZoUnXc5IX4I5a1tUBW2BQQld5bobBmOgfM+njXWSiQNZ9veniYoV8auS/topYEZGacTv9joxnZoCY4mOpIwTcp5+XhC2Z5HB4QBJgA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=XX41RY4R; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="XX41RY4R" Received: by smtp.kernel.org (Postfix) with ESMTPSA id C5C92C116D0; Sun, 18 Jan 2026 06:11:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1768716720; bh=XQZz9Nu+u7TdwrpS5yQTxTW/ej0dkCDpS+WPtv5Iudo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=XX41RY4RMG0ZGtak6oOJ8ro7GjFQKoOTEGax7hbBjl5C/AdiAvlmAyQWb2RhVkmXL whCApzlt6RghCmL2BW7E4KC8L2C+ANNlMiddN6jW+afgjxv2oQa6WQTZq9WpfJbiu/ SE1IVwdtIMLB4i9RnrD5nCTuUODRXiXw9C/2+s1+ty4rWJJExgOzZjkM3c1aFBRbkP oXGmkvAqB/1+Eor9pzWjrBDHh1DbT1DVnxKgkRPyFqRyWrg8WxK2AIi1HN+CgU+LAt vZWZ3NM7ey7xCLr6yTX7dOtSGIO/xW1VENnaUVRoDjmXacnBYfZjk5QLFeLFLtaaMJ NvFQLWabs2iUw== Date: Sun, 18 Jan 2026 08:11:52 +0200 From: Mike Rapoport To: Pratyush Yadav Cc: Breno Leitao , Pasha Tatashin , akpm@linux-foundation.org, graf@amazon.com, linux-kernel@vger.kernel.org, kexec@lists.infradead.org, linux-mm@kvack.org, ricardo.neri-calderon@linux.intel.com, kernel-team@meta.com Subject: Re: [PATCH v4] kho: validate preserved memory map during population Message-ID: References: <20251223140140.2090337-1-pasha.tatashin@soleen.com> <2vxzo6mt7cl3.fsf@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2vxzo6mt7cl3.fsf@kernel.org> On Fri, Jan 16, 2026 at 04:21:28PM +0000, Pratyush Yadav wrote: > Hi Breno, > > On Thu, Jan 08 2026, Breno Leitao wrote: > > > Hello Pasha, > > > > On Tue, Dec 23, 2025 at 09:01:40AM -0500, Pasha Tatashin wrote: > >> If the previous kernel enabled KHO but did not call kho_finalize() > >> (e.g., CONFIG_LIVEUPDATE=n or userspace skipped the finalization step), > >> the 'preserved-memory-map' property in the FDT remains empty/zero. > >> > >> Previously, kho_populate() would succeed regardless of the memory map's > >> state, reserving the incoming scratch regions in memblock. However, > >> kho_memory_init() would later fail to deserialize the empty map. By that > >> time, the scratch regions were already registered, leading to partial > >> initialization and subsequent list corruption (freeing scratch area > >> twice) during kho_init(). > > > > While trying my new patchset [0] on top of this patch, I got the > > following issue: > > > > [ 0.000000] KHO: disabling KHO revival: -2 > > > > Trying to solve it, I come up with a change in kho_get_mem_map_phys() to > > distinguish no memory and error, see the patch attached later. > > > > This is what I used to test [0] on top of linux-next. Is this useful? > > > > Link: https://lore.kernel.org/all/20260108-kho-v3-1-b1d6b7a89342@debian.org/ [0] > > > > thanks > > --breno > > > > commit 5d7855fede8110d74942e1b67056ba589a1cb54a > > Author: Breno Leitao > > Date: Thu Jan 8 07:44:08 2026 -0800 > > > > kho: allow KHO to work when no memory is preserved > > > > Fix KHO initialization failing when no memory pages were preserved by > > the previous kernel. > > > > Commit eda79a683a0a ("kho: validate preserved memory map during > > population") introduced kho_get_mem_map_phys() which returns the physical > > address of the preserved memory map directly as its return value. The > > caller then validates it with: > > > > mem_map_phys = kho_get_mem_map_phys(fdt); > > if (!mem_map_phys) { > > err = -ENOENT; > > goto out; > > } > > > > This creates an ambiguity: physical address 0 is used both as an error > > indicator (property missing/malformed) and as a valid value (property > > exists with value 0, meaning no memory was preserved). > > > > "No memory preserved" is a legitimate state. KHO provides features beyond > > memory page preservation, such as previous kernel version tracking and > > kexec count tracking. When the previous kernel enables KHO but doesn't > > preserve any memory pages, it sets 'preserved-memory-map' to 0. This is > > semantically different from "KHO not initialized" - it means "KHO is > > active, there's just nothing in the memory map." > > This isn't true. If you hand over _any_ state, you will at least need > the KHO FDT. And the KHO FDT is preserved memory (see the > kho_alloc_preserve() call in kho_init()). So I don't see how you can > ever have valid KHO with no memory. > > mem_map_phys _can_ be 0, but only when KHO was enabled but not used. And > that is of course also a valid use case. > > We want to treat mem_map_phys == 0 the same as the error path, just > without the error print. This lets us discard all previous scratch areas > since they don't have anything useful anyway, and have a fresh start. > > So while you are seeing this error message, I don't think it should > break anything and KHO should still be working fine. You can > double-check this by inspecting /sys/kernel/debug/kho/out. > > So I think the patch is certainly a useful fix, it just needs some > re-wording and fixups. > > Some comments on the code below. > > > > > Before eda79a683a0a, the code handled this gracefully in > > kho_mem_deserialize(): > > > > chunk = mem ? phys_to_virt(mem) : NULL; > > if (!chunk) > > return false; // No pages, but KHO could still work > > > > After eda79a683a0a, the early validation conflated "no property" with > > "property value is 0", causing KHO to be completely disabled in both > > cases. > > > > Fix this by changing kho_get_mem_map_phys() to return an error code and > > pass the physical address via pointer. This allows distinguishing between: > > - Property missing/malformed: return -ENOENT (KHO fails) > > - Property exists with value 0: return 0 (KHO succeeds, no memory to > > restore) > > > > Fixes: eda79a683a0a ("kho: validate preserved memory map during population") > > Signed-off-by: Breno Leitao > > > > diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c > > index 271d90198a08..3cf2dc6840c9 100644 > > --- a/kernel/liveupdate/kexec_handover.c > > +++ b/kernel/liveupdate/kexec_handover.c > > @@ -471,8 +471,8 @@ static void __init deserialize_bitmap(unsigned int order, > > } > > } > > > > -/* Returns physical address of the preserved memory map from FDT */ > > -static phys_addr_t __init kho_get_mem_map_phys(const void *fdt) > > +/* Returns 0 on success and stores physical address in *phys_out */ > > +static int __init kho_get_mem_map_phys(const void *fdt, phys_addr_t *phys_out) > > { > > const void *mem_ptr; > > int len; > > @@ -480,10 +480,11 @@ static phys_addr_t __init kho_get_mem_map_phys(const void *fdt) > > mem_ptr = fdt_getprop(fdt, 0, KHO_FDT_MEMORY_MAP_PROP_NAME, &len); > > if (!mem_ptr || len != sizeof(u64)) { > > pr_err("failed to get preserved memory bitmaps\n"); > > - return 0; > > + return -ENOENT; > > } > > > > - return get_unaligned((const u64 *)mem_ptr); > > + *phys_out = get_unaligned((const u64 *)mem_ptr); > > + return 0; > > } > > > > static void __init kho_mem_deserialize(struct khoser_mem_chunk *chunk) > > @@ -1439,7 +1440,7 @@ void __init kho_populate(phys_addr_t fdt_phys, u64 fdt_len, > > phys_addr_t scratch_phys, u64 scratch_len) > > { > > struct kho_scratch *scratch = NULL; > > - phys_addr_t mem_map_phys; > > + phys_addr_t mem_map_phys = 0; > > void *fdt = NULL; > > int err = 0; > > unsigned int scratch_cnt = scratch_len / sizeof(*kho_scratch); > > @@ -1466,11 +1467,9 @@ void __init kho_populate(phys_addr_t fdt_phys, u64 fdt_len, > > goto out; > > } > > > > - mem_map_phys = kho_get_mem_map_phys(fdt); > > - if (!mem_map_phys) { > > - err = -ENOENT; > > + err = kho_get_mem_map_phys(fdt, &mem_map_phys); > > + if (err) > > This will break when mem_map_phys == 0. As I explained earlier, when > that happens we want to discard all previous scratch info and start with > a clean slate. > > Making this if (err || !mem_map_phys) should do the trick. The if (err) > check before the print should make sure the error message is not printed > when we have a valid property but its value is 0. While we are on it, I'd suggest to change kho_populate() error handling to use goto, (i.e like below) Then a simple if (err) will do and that's much clearer. Another thing I noticed it that assigning err to -EFAULT or -EINVAL after printks is completely redundant, since we anyway report what went wrong, so printing the error value in the end just not needed. diff --git a/kernel/liveupdate/kexec_handover.c b/kernel/liveupdate/kexec_handover.c index feffeafa51b7..2bba111149c4 100644 --- a/kernel/liveupdate/kexec_handover.c +++ b/kernel/liveupdate/kexec_handover.c @@ -1453,27 +1453,27 @@ void __init kho_populate(phys_addr_t fdt_phys, u64 fdt_len, if (!fdt) { pr_warn("setup: failed to memremap FDT (0x%llx)\n", fdt_phys); err = -EFAULT; - goto out; + goto err_report; } err = fdt_check_header(fdt); if (err) { pr_warn("setup: handover FDT (0x%llx) is invalid: %d\n", fdt_phys, err); err = -EINVAL; - goto out; + goto err_unmap_fdt; } err = fdt_node_check_compatible(fdt, 0, KHO_FDT_COMPATIBLE); if (err) { pr_warn("setup: handover FDT (0x%llx) is incompatible with '%s': %d\n", fdt_phys, KHO_FDT_COMPATIBLE, err); err = -EINVAL; - goto out; + goto err_unmap_fdt; } mem_map_phys = kho_get_mem_map_phys(fdt); if (!mem_map_phys) { err = -ENOENT; - goto out; + goto err_unmap_fdt; } scratch = early_memremap(scratch_phys, scratch_len); @@ -1481,7 +1481,7 @@ void __init kho_populate(phys_addr_t fdt_phys, u64 fdt_len, pr_warn("setup: failed to memremap scratch (phys=0x%llx, len=%lld)\n", scratch_phys, scratch_len); err = -EFAULT; - goto out; + goto err_unmap_scratch; } /* @@ -1498,7 +1498,7 @@ void __init kho_populate(phys_addr_t fdt_phys, u64 fdt_len, if (WARN_ON(err)) { pr_warn("failed to mark the scratch region 0x%pa+0x%pa: %pe", &area->addr, &size, ERR_PTR(err)); - goto out; + goto err_unmap_scratch; } pr_debug("Marked 0x%pa+0x%pa as scratch", &area->addr, &size); } @@ -1520,13 +1520,14 @@ void __init kho_populate(phys_addr_t fdt_phys, u64 fdt_len, kho_scratch_cnt = scratch_cnt; pr_info("found kexec handover data.\n"); -out: - if (fdt) - early_memunmap(fdt, fdt_len); - if (scratch) - early_memunmap(scratch, scratch_len); - if (err) - pr_warn("disabling KHO revival: %d\n", err); + return; + +err_unmap_scratch: + early_memunmap(scratch, scratch_len); +err_unmap_fdt: + early_memunmap(fdt, fdt_len); +err_report: + pr_warn("disabling KHO revival: %d\n", err); } /* Helper functions for kexec_file_load */ > > goto out; > > - } > > > > scratch = early_memremap(scratch_phys, scratch_len); > > if (!scratch) { > > -- > Regards, > Pratyush Yadav -- Sincerely yours, Mike.