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 853F144E03A for ; Tue, 20 Jan 2026 15:11:44 +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=1768921904; cv=none; b=nQtP0/tU3w9lOx28qaQ2Ga29QwusH7YYNLYlUyYV/wKiTshIqu5sRrPWjQ3odasMapjtVI86wTBxX0qcdH8WhRnRlEcWkP8+CtiC/FnL5T50ls/C9kEDAEZmflQzud5KJ07Pdr1XOR8/qb3rtqs22Ub/8TXv+DUS577GuwDBIb8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768921904; c=relaxed/simple; bh=L0kijvKmmnnVJts9P035SvIiJZGDcTy18QVATluzDTk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=U/DTBEBadOe8b0MMSyQAY0pEV+M4eFEMQXf42iPazTj03VzJcIi4zIjbP2rj0QVq0YbQFKNFEwxUC/nVXnx7clZg0lnpAYjj3AeU+wspUQrt5K0P7kCM+CvcUk5DOPSx8yJ8v9otBt1jfKefZISk63+crVIZyM20uw0GrRBNFTY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=pqNBI0K4; 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="pqNBI0K4" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 79283C16AAE; Tue, 20 Jan 2026 15:11:42 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1768921904; bh=L0kijvKmmnnVJts9P035SvIiJZGDcTy18QVATluzDTk=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=pqNBI0K4mNIsrekd8AQKjzjwr7TpaE02BvkUwCFkvouaKATK39xHpsYYUwhlDz/F8 xvMXfZuXW10ipkYaIXa6VJnj+Xrb7tHqCH6m8QGJd7+1BY3+qrnKOO9f2tv38MyMeP R3XumRvZvWK4kZjucabXR6uQLPsS/cG/QNj5PrGRqV8pw4gT86SVseb42WeazYWfE9 6rdi1BynKj+Iht7MSeFAG8sPuI6zzH4srLDTx51YsQ78T8l3+pqdZDUUP6nqwDmLq1 JJgZ9YoaaExJx7qmFJqPXm5+pD6iy9ak7d8jOIgP7BYIdDhU2edmZDtbx0TTA8E12o k1hkwtG2XrgEg== From: Pratyush Yadav To: Mike Rapoport Cc: Pratyush Yadav , 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 In-Reply-To: (Mike Rapoport's message of "Sun, 18 Jan 2026 08:11:52 +0200") References: <20251223140140.2090337-1-pasha.tatashin@soleen.com> <2vxzo6mt7cl3.fsf@kernel.org> Date: Tue, 20 Jan 2026 15:11:41 +0000 Message-ID: <2vxzv7gwe2tu.fsf@kernel.org> User-Agent: Gnus/5.13 (Gnus v5.13) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Sun, Jan 18 2026, Mike Rapoport wrote: > 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. Yeah, looks like a good cleanup. > > 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 -- Regards, Pratyush Yadav