Message ID | 20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp3637825wrr; Thu, 24 Nov 2022 14:28:42 -0800 (PST) X-Google-Smtp-Source: AA0mqf6eucNCvGNYevdWb90I7clKlXDsMPlo989D6SjywM6IX1982GSQgM5s3cJCPeG7rBuC8ldn X-Received: by 2002:a63:5fd6:0:b0:477:bdb5:878f with SMTP id t205-20020a635fd6000000b00477bdb5878fmr7676962pgb.61.1669328921830; Thu, 24 Nov 2022 14:28:41 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669328921; cv=none; d=google.com; s=arc-20160816; b=P0q2f2qEkf/IPvNRdsGtsdufwABm19xu4T3dB1OUuBr9lbTdGYlUlzwPcXIC/ZM0Eh 7vha3HTJ6eA6EwOU7V0+wE5QEDDYm8l44jCbxw9ecKC/c8gs0IxXG3XWwkPH3CG8LQMp RbFKR3/K4foE5IyMGei3x0RAok72Fy3LuJ5K5cZGqXhf3XmhR6DRevkFe5aUFOA0vcE0 zhYMYebQBm+50f1Z78yJxch3zYf7TGCqGxVoxiucAoRurBRUXsYUd+KybuZ+bCXjPwwD Nj1+Zbl9GYzv9X4DMUVH1Y4QGhXHwZDRFALt5u742/+50CQZ8kv+FW26kxdvZ8WfCmcA 2BYw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:message-id:content-transfer-encoding :mime-version:subject:date:from:dkim-signature; bh=RDvQaU77JBFIkQ2oUa6F+0NvAeH+qvtlzjePcszuhs4=; b=Oo6yiAVIjO1zDyrJWfJM/cBNC3/9XSvif+9U93tI7nhUeIwUT8/ArZHh2lvtXD4oPQ WMOhDGw867AYdtWtVqwkxUP3y5y1rIphqaCZcnWytO0XlvtaVetD+4t9qsEHHmrkU+0a ZwhNogp+JIvMso3xJ5jk+OrJlV74TtsR5mCrI0lgy/9EuZqn0kSRupv9/VWJwOhjKxeM O1Zj1nSojQLlZ7OTWeKEWoPQK0O3D2CaVxLhleMX5Rvgfumvo8i4NkLDX94od/fGlMcm wgpQ583P/QCee44AHp6sEG7w8cMgIE4JhNiXSWCvw0v0jC1BtSXC3jpkEc/DoWXE5ggb NKRQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=IPMbRodr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i8-20020a17090332c800b00176808b0a3asi1053013plr.441.2022.11.24.14.28.27; Thu, 24 Nov 2022 14:28:41 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=IPMbRodr; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229502AbiKXWYB (ORCPT <rfc822;fengqi706@gmail.com> + 99 others); Thu, 24 Nov 2022 17:24:01 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37772 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229450AbiKXWYA (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 24 Nov 2022 17:24:00 -0500 Received: from mail-ed1-x529.google.com (mail-ed1-x529.google.com [IPv6:2a00:1450:4864:20::529]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 655837CB94 for <linux-kernel@vger.kernel.org>; Thu, 24 Nov 2022 14:23:59 -0800 (PST) Received: by mail-ed1-x529.google.com with SMTP id x2so4136242edd.2 for <linux-kernel@vger.kernel.org>; Thu, 24 Nov 2022 14:23:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=cc:to:message-id:content-transfer-encoding:mime-version:subject :date:from:from:to:cc:subject:date:message-id:reply-to; bh=RDvQaU77JBFIkQ2oUa6F+0NvAeH+qvtlzjePcszuhs4=; b=IPMbRodr2ND7GVEwE0GnAs0d4Zwg1lAlQB9mR56wG2dyMixyC5kqUbEKtCOgX9hML5 ADM9Z/mMyxke38dOCd7jbEPzu1gx9PYogXZ7YDtBm2znQ4dacV7LJC7AT1eSx7ob0jcn 8W5zJharibXpbGTy/H7BM7Mrq7V7jNibfzfLA= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:message-id:content-transfer-encoding:mime-version:subject :date:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=RDvQaU77JBFIkQ2oUa6F+0NvAeH+qvtlzjePcszuhs4=; b=tCYRhq8ZDpxhzPxFWodlHAB9MB0bNcH2sVD5LdU/ZOIQET2DqiiUt4cHD8bVVvPNox EV7yZfilr9RnEQp9EhPVjKS8m4c3JyTo1u/WW1W0XSggV0jtajcT2kPYXjdwMhzEKenh bX0DXBVphk8WkyMwTORiUci57XZKj9auMtm7P8tlzJKvFei3dQAB1UcnP+91OPu6ap3x RhZwkCSGGJZXnHIASWF7Ewb1/hybxwaQHulTLlHlmOt7DPi7w4htKLRoyyzdzxbFR4ZY g5LdTYdXBVSx7Ly/Losx6r+we1ou5uLzBYZR6A1iSqMOjlzYvfYmTSmHTAHx3tbFFl4R dIdg== X-Gm-Message-State: ANoB5pmj2Q2Ef1DYwbYSXdR1pkmNb7jpAkVF9wF04H6HL8N1mleZcaXx oA4jof2tpefKj1FjTt4QP07wSQ== X-Received: by 2002:a05:6402:1f89:b0:458:caec:8f1e with SMTP id c9-20020a0564021f8900b00458caec8f1emr31387244edc.280.1669328637894; Thu, 24 Nov 2022 14:23:57 -0800 (PST) Received: from alco.roam.corp.google.com (80.71.134.83.ipv4.parknet.dk. [80.71.134.83]) by smtp.gmail.com with ESMTPSA id p23-20020a17090653d700b007822196378asm871216ejo.176.2022.11.24.14.23.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 24 Nov 2022 14:23:57 -0800 (PST) From: Ricardo Ribalda <ribalda@chromium.org> Date: Thu, 24 Nov 2022 23:23:36 +0100 Subject: [PATCH] kexec: Enable runtime allocation of crash_image MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20221124-kexec-noalloc-v1-0-d78361e99aec@chromium.org> To: Eric Biederman <ebiederm@xmission.com> Cc: Baoquan He <bhe@redhat.com>, Philipp Rudo <prudo@redhat.com>, Sergey Senozhatsky <senozhatsky@chromium.org>, Ross Zwisler <zwisler@kernel.org>, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, Ricardo Ribalda <ribalda@chromium.org> X-Mailer: b4 0.11.0-dev-696ae X-Developer-Signature: v=1; a=openpgp-sha256; l=4832; i=ribalda@chromium.org; h=from:subject:message-id; bh=4synMW5GYep28EG2om20KSl9OgW8vz6iw1zjyeYDzG4=; b=owEBbQKS/ZANAwAKAdE30T7POsSIAcsmYgBjf+71OVt3J96ieJdQKPp8Bd5A9iPh5yK+hHxrTqn1 tbx1b0+JAjMEAAEKAB0WIQREDzjr+/4oCDLSsx7RN9E+zzrEiAUCY3/u9QAKCRDRN9E+zzrEiH3gEA CWnsTwGWniPrhO8ejZOwYcYBSb4+RNFNGYz59X3eLIuMW0+N+lFCUfVwiLiIXrca0XcEFxu9sVoXgC RvXxRNTssXhBNHTtLSXCz6CeMjBSbeycEzTefvySSIk7unB2N6+WF93oitDDg6zSybsRrTTAjxtWND UW/LiUBd7ghu63XJZbiVqf1TZBWwMyrNIdq4+ybU838nTlmwVSII6CJdRAiePNOtDHtq2MAm+IM5X4 9fnT6afjk1xL87jS4WLcj0u1FeCpsvRHoYTkLt5absIRcOdP5VWkaOglOwEQdemhHWp/XweGaylEAa cwwesUZZH0tTdaYfOE61BAzbWBOMflf8701JqvAECeW5liwMXiXoJsnKAU/s2ffVLXQrJ0rQ2w6upH u05RQSwi3xn0AWHT8ZxxKqYe/S3uNn9+eYzIJq+MuwdwmBTkaEjZnu7qVFLVvZQkCfPe4Rll2nSFyc n0lfdJT2OlhP+H7m4xeV5agNDcKb93joL8X3ckvK7rDMed5xtWD/CfxWHMZG8vUpowexeV5Zfh+piS HrY/ajE/FKdJqkbCMlwQJG8IEMBTwy/LgagYfKU9pmikm/bCBoS07a0EYclfXD8bws6fqWHuXmSBQU 8ziQ1nClcCH20sCtAJ0R3a6dmG0py9UO+bgvvq7foLCKvx+GVJblfmX2S3fg== X-Developer-Key: i=ribalda@chromium.org; a=openpgp; fpr=9EC3BB66E2FC129A6F90B39556A0D81F9F782DA9 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1750418243374325879?= X-GMAIL-MSGID: =?utf-8?q?1750418243374325879?= |
Series |
kexec: Enable runtime allocation of crash_image
|
|
Commit Message
Ricardo Ribalda
Nov. 24, 2022, 10:23 p.m. UTC
Usually crash_image is defined statically via the crashkernel parameter
or DT.
But if the crash kernel is not used, or is smaller than then
area pre-allocated that memory is wasted.
Also, if the crash kernel was not defined at bootime, there is no way to
use the crash kernel.
Enable runtime allocation of the crash_image if the crash_image is not
defined statically. Following the same memory allocation/validation path
that for the reboot kexec kernel.
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
kexec: Enable runtime allocation of crash_image
To: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: linux-kernel@vger.kernel.org
Cc: Ross Zwisler <zwisler@kernel.org>
Cc: Philipp Rudo <prudo@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
---
include/linux/kexec.h | 1 +
kernel/kexec.c | 9 +++++----
kernel/kexec_core.c | 5 +++++
kernel/kexec_file.c | 7 ++++---
4 files changed, 15 insertions(+), 7 deletions(-)
---
base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4
change-id: 20221124-kexec-noalloc-3cab3cbe000f
Best regards,
Comments
On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > Usually crash_image is defined statically via the crashkernel parameter > or DT. > > But if the crash kernel is not used, or is smaller than then > area pre-allocated that memory is wasted. > > Also, if the crash kernel was not defined at bootime, there is no way to > use the crash kernel. > > Enable runtime allocation of the crash_image if the crash_image is not > defined statically. Following the same memory allocation/validation path > that for the reboot kexec kernel. We don't check if the crashkernel memory region is valid in kernel, but we do have done the check in kexec-tools utility. Since both kexec_load and kexec_file_load need go through path of kexec-tools loading, we haven't got problem with lack of the checking in kernel. However, even though we want to do the check, doing like below is much easier and more reasonable. diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index 45637511e0de..4d1339bd2ccf 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, dest_image = &kexec_image; if (flags & KEXEC_FILE_ON_CRASH) { + if (!crash_memory_valid()) + return -EINVAL; dest_image = &kexec_crash_image; if (kexec_crash_image) arch_kexec_unprotect_crashkres(); So, I am wondering if there is an issue encountered if we don't do the check in kernel. Thanks Baoquan > > --- > > To: Eric Biederman <ebiederm@xmission.com> > Cc: kexec@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: linux-kernel@vger.kernel.org > Cc: Ross Zwisler <zwisler@kernel.org> > Cc: Philipp Rudo <prudo@redhat.com> > Cc: Baoquan He <bhe@redhat.com> > --- > include/linux/kexec.h | 1 + > kernel/kexec.c | 9 +++++---- > kernel/kexec_core.c | 5 +++++ > kernel/kexec_file.c | 7 ++++--- > 4 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > index 41a686996aaa..98ca9a32bc8e 100644 > --- a/include/linux/kexec.h > +++ b/include/linux/kexec.h > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > extern bool kexec_in_progress; > > int crash_shrink_memory(unsigned long new_size); > +bool __crash_memory_valid(void); > ssize_t crash_get_memory_size(void); > > #ifndef arch_kexec_protect_crashkres > diff --git a/kernel/kexec.c b/kernel/kexec.c > index cb8e6e6f983c..b5c17db25e88 100644 > --- a/kernel/kexec.c > +++ b/kernel/kexec.c > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > struct kimage *image; > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > - if (kexec_on_panic) { > + if (kexec_on_panic && __crash_memory_valid()) { > /* Verify we have a valid entry point */ > if ((entry < phys_to_boot_phys(crashk_res.start)) || > (entry > phys_to_boot_phys(crashk_res.end))) > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > image->nr_segments = nr_segments; > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > - if (kexec_on_panic) { > + if (kexec_on_panic && __crash_memory_valid()) { > /* Enable special crash kernel control page alloc policy. */ > image->control_page = crashk_res.start; > image->type = KEXEC_TYPE_CRASH; > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > if (flags & KEXEC_ON_CRASH) { > dest_image = &kexec_crash_image; > - if (kexec_crash_image) > + if (kexec_crash_image && __crash_memory_valid()) > arch_kexec_unprotect_crashkres(); > } else { > dest_image = &kexec_image; > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > image = xchg(dest_image, image); > > out: > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > + __crash_memory_valid()) > arch_kexec_protect_crashkres(); > > kimage_free(image); > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > index ca2743f9c634..77083c9760fb 100644 > --- a/kernel/kexec_core.c > +++ b/kernel/kexec_core.c > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > } > } > > +bool __crash_memory_valid(void) > +{ > + return crashk_res.end != crashk_res.start; > +} > + > ssize_t crash_get_memory_size(void) > { > ssize_t size = 0; > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index 45637511e0de..0671f4f370ff 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > image->file_mode = 1; > > - if (kexec_on_panic) { > + if (kexec_on_panic && __crash_memory_valid()) { > /* Enable special crash kernel control page alloc policy. */ > image->control_page = crashk_res.start; > image->type = KEXEC_TYPE_CRASH; > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > dest_image = &kexec_image; > if (flags & KEXEC_FILE_ON_CRASH) { > dest_image = &kexec_crash_image; > - if (kexec_crash_image) > + if (kexec_crash_image && __crash_memory_valid()) > arch_kexec_unprotect_crashkres(); > } > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > exchange: > image = xchg(dest_image, image); > out: > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > + __crash_memory_valid()) > arch_kexec_protect_crashkres(); > > kexec_unlock(); > > --- > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > Best regards, > -- > Ricardo Ribalda <ribalda@chromium.org> >
Hi Baoquan Thanks for your review! On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > Usually crash_image is defined statically via the crashkernel parameter > > or DT. > > > > But if the crash kernel is not used, or is smaller than then > > area pre-allocated that memory is wasted. > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > use the crash kernel. > > > > Enable runtime allocation of the crash_image if the crash_image is not > > defined statically. Following the same memory allocation/validation path > > that for the reboot kexec kernel. > > We don't check if the crashkernel memory region is valid in kernel, but > we do have done the check in kexec-tools utility. Since both kexec_load and > kexec_file_load need go through path of kexec-tools loading, we haven't > got problem with lack of the checking in kernel. Not sure if I follow you. We currently check if the crash kernel is in the right place at sanity_check_segment_list() https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > However, even though we want to do the check, doing like below is much > easier and more reasonable. > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index 45637511e0de..4d1339bd2ccf 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > dest_image = &kexec_image; > if (flags & KEXEC_FILE_ON_CRASH) { > + if (!crash_memory_valid()) > + return -EINVAL; > dest_image = &kexec_crash_image; > if (kexec_crash_image) > arch_kexec_unprotect_crashkres(); > > So, I am wondering if there is an issue encountered if we don't do the > check in kernel. > > Thanks > Baoquan > > > > > --- > > > > To: Eric Biederman <ebiederm@xmission.com> > > Cc: kexec@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > Cc: linux-kernel@vger.kernel.org > > Cc: Ross Zwisler <zwisler@kernel.org> > > Cc: Philipp Rudo <prudo@redhat.com> > > Cc: Baoquan He <bhe@redhat.com> > > --- > > include/linux/kexec.h | 1 + > > kernel/kexec.c | 9 +++++---- > > kernel/kexec_core.c | 5 +++++ > > kernel/kexec_file.c | 7 ++++--- > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > index 41a686996aaa..98ca9a32bc8e 100644 > > --- a/include/linux/kexec.h > > +++ b/include/linux/kexec.h > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > extern bool kexec_in_progress; > > > > int crash_shrink_memory(unsigned long new_size); > > +bool __crash_memory_valid(void); > > ssize_t crash_get_memory_size(void); > > > > #ifndef arch_kexec_protect_crashkres > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > index cb8e6e6f983c..b5c17db25e88 100644 > > --- a/kernel/kexec.c > > +++ b/kernel/kexec.c > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > struct kimage *image; > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > - if (kexec_on_panic) { > > + if (kexec_on_panic && __crash_memory_valid()) { > > /* Verify we have a valid entry point */ > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > (entry > phys_to_boot_phys(crashk_res.end))) > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > image->nr_segments = nr_segments; > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > - if (kexec_on_panic) { > > + if (kexec_on_panic && __crash_memory_valid()) { > > /* Enable special crash kernel control page alloc policy. */ > > image->control_page = crashk_res.start; > > image->type = KEXEC_TYPE_CRASH; > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > if (flags & KEXEC_ON_CRASH) { > > dest_image = &kexec_crash_image; > > - if (kexec_crash_image) > > + if (kexec_crash_image && __crash_memory_valid()) > > arch_kexec_unprotect_crashkres(); > > } else { > > dest_image = &kexec_image; > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > image = xchg(dest_image, image); > > > > out: > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > + __crash_memory_valid()) > > arch_kexec_protect_crashkres(); > > > > kimage_free(image); > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > index ca2743f9c634..77083c9760fb 100644 > > --- a/kernel/kexec_core.c > > +++ b/kernel/kexec_core.c > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > } > > } > > > > +bool __crash_memory_valid(void) > > +{ > > + return crashk_res.end != crashk_res.start; > > +} > > + > > ssize_t crash_get_memory_size(void) > > { > > ssize_t size = 0; > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > index 45637511e0de..0671f4f370ff 100644 > > --- a/kernel/kexec_file.c > > +++ b/kernel/kexec_file.c > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > image->file_mode = 1; > > > > - if (kexec_on_panic) { > > + if (kexec_on_panic && __crash_memory_valid()) { > > /* Enable special crash kernel control page alloc policy. */ > > image->control_page = crashk_res.start; > > image->type = KEXEC_TYPE_CRASH; > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > dest_image = &kexec_image; > > if (flags & KEXEC_FILE_ON_CRASH) { > > dest_image = &kexec_crash_image; > > - if (kexec_crash_image) > > + if (kexec_crash_image && __crash_memory_valid()) > > arch_kexec_unprotect_crashkres(); > > } > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > exchange: > > image = xchg(dest_image, image); > > out: > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > + __crash_memory_valid()) > > arch_kexec_protect_crashkres(); > > > > kexec_unlock(); > > > > --- > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > Best regards, > > -- > > Ricardo Ribalda <ribalda@chromium.org> > > > -- Ricardo Ribalda
On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > Hi Baoquan > > Thanks for your review! > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > Usually crash_image is defined statically via the crashkernel parameter > > > or DT. > > > > > > But if the crash kernel is not used, or is smaller than then > > > area pre-allocated that memory is wasted. > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > use the crash kernel. > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > defined statically. Following the same memory allocation/validation path > > > that for the reboot kexec kernel. > > > > We don't check if the crashkernel memory region is valid in kernel, but > > we do have done the check in kexec-tools utility. Since both kexec_load and > > kexec_file_load need go through path of kexec-tools loading, we haven't > > got problem with lack of the checking in kernel. > > Not sure if I follow you. > > We currently check if the crash kernel is in the right place at > sanity_check_segment_list() > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 Please check below code in kexec-tools utility, currently we have to use kexec -p to enter into kexec_load or kexec_file_load system call. Before entering system call, we have below code: https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c int main(int argc, char *argv[]) { ...... if (do_load && ((kexec_flags & KEXEC_ON_CRASH) || (kexec_file_flags & KEXEC_FILE_ON_CRASH)) && !is_crashkernel_mem_reserved()) { die("Memory for crashkernel is not reserved\n" "Please reserve memory by passing" "\"crashkernel=Y@X\" parameter to kernel\n" "Then try to loading kdump kernel\n"); } ...... } > > > > > > However, even though we want to do the check, doing like below is much > > easier and more reasonable. > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > index 45637511e0de..4d1339bd2ccf 100644 > > --- a/kernel/kexec_file.c > > +++ b/kernel/kexec_file.c > > @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > dest_image = &kexec_image; > > if (flags & KEXEC_FILE_ON_CRASH) { > > + if (!crash_memory_valid()) > > + return -EINVAL; > > dest_image = &kexec_crash_image; > > if (kexec_crash_image) > > arch_kexec_unprotect_crashkres(); > > > > So, I am wondering if there is an issue encountered if we don't do the > > check in kernel. > > > > Thanks > > Baoquan > > > > > > > > --- > > > > > > To: Eric Biederman <ebiederm@xmission.com> > > > Cc: kexec@lists.infradead.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > > Cc: linux-kernel@vger.kernel.org > > > Cc: Ross Zwisler <zwisler@kernel.org> > > > Cc: Philipp Rudo <prudo@redhat.com> > > > Cc: Baoquan He <bhe@redhat.com> > > > --- > > > include/linux/kexec.h | 1 + > > > kernel/kexec.c | 9 +++++---- > > > kernel/kexec_core.c | 5 +++++ > > > kernel/kexec_file.c | 7 ++++--- > > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > > index 41a686996aaa..98ca9a32bc8e 100644 > > > --- a/include/linux/kexec.h > > > +++ b/include/linux/kexec.h > > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > > extern bool kexec_in_progress; > > > > > > int crash_shrink_memory(unsigned long new_size); > > > +bool __crash_memory_valid(void); > > > ssize_t crash_get_memory_size(void); > > > > > > #ifndef arch_kexec_protect_crashkres > > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > > index cb8e6e6f983c..b5c17db25e88 100644 > > > --- a/kernel/kexec.c > > > +++ b/kernel/kexec.c > > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > struct kimage *image; > > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > > > - if (kexec_on_panic) { > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > /* Verify we have a valid entry point */ > > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > > (entry > phys_to_boot_phys(crashk_res.end))) > > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > image->nr_segments = nr_segments; > > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > > > - if (kexec_on_panic) { > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > /* Enable special crash kernel control page alloc policy. */ > > > image->control_page = crashk_res.start; > > > image->type = KEXEC_TYPE_CRASH; > > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > if (flags & KEXEC_ON_CRASH) { > > > dest_image = &kexec_crash_image; > > > - if (kexec_crash_image) > > > + if (kexec_crash_image && __crash_memory_valid()) > > > arch_kexec_unprotect_crashkres(); > > > } else { > > > dest_image = &kexec_image; > > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > image = xchg(dest_image, image); > > > > > > out: > > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > > + __crash_memory_valid()) > > > arch_kexec_protect_crashkres(); > > > > > > kimage_free(image); > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > > index ca2743f9c634..77083c9760fb 100644 > > > --- a/kernel/kexec_core.c > > > +++ b/kernel/kexec_core.c > > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > > } > > > } > > > > > > +bool __crash_memory_valid(void) > > > +{ > > > + return crashk_res.end != crashk_res.start; > > > +} > > > + > > > ssize_t crash_get_memory_size(void) > > > { > > > ssize_t size = 0; > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > index 45637511e0de..0671f4f370ff 100644 > > > --- a/kernel/kexec_file.c > > > +++ b/kernel/kexec_file.c > > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > > > image->file_mode = 1; > > > > > > - if (kexec_on_panic) { > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > /* Enable special crash kernel control page alloc policy. */ > > > image->control_page = crashk_res.start; > > > image->type = KEXEC_TYPE_CRASH; > > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > dest_image = &kexec_image; > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > dest_image = &kexec_crash_image; > > > - if (kexec_crash_image) > > > + if (kexec_crash_image && __crash_memory_valid()) > > > arch_kexec_unprotect_crashkres(); > > > } > > > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > exchange: > > > image = xchg(dest_image, image); > > > out: > > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > > + __crash_memory_valid()) > > > arch_kexec_protect_crashkres(); > > > > > > kexec_unlock(); > > > > > > --- > > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > > > Best regards, > > > -- > > > Ricardo Ribalda <ribalda@chromium.org> > > > > > > > > -- > Ricardo Ribalda >
Hi Baoquan On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote: > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > > Hi Baoquan > > > > Thanks for your review! > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > > Usually crash_image is defined statically via the crashkernel parameter > > > > or DT. > > > > > > > > But if the crash kernel is not used, or is smaller than then > > > > area pre-allocated that memory is wasted. > > > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > > use the crash kernel. > > > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > > defined statically. Following the same memory allocation/validation path > > > > that for the reboot kexec kernel. > > > > > > We don't check if the crashkernel memory region is valid in kernel, but > > > we do have done the check in kexec-tools utility. Since both kexec_load and > > > kexec_file_load need go through path of kexec-tools loading, we haven't > > > got problem with lack of the checking in kernel. > > > > Not sure if I follow you. > > > > We currently check if the crash kernel is in the right place at > > sanity_check_segment_list() > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > Please check below code in kexec-tools utility, currently we have to use > kexec -p to enter into kexec_load or kexec_file_load system call. Before > entering system call, we have below code: So your concern is that the current kexec-tools does not let you pass a crashkernel unless there is memory reserved for it? Once the changes land in the kernel I can make a patch for that. I am currently using this to test the code: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c > > int main(int argc, char *argv[]) > { > ...... > if (do_load && > ((kexec_flags & KEXEC_ON_CRASH) || > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) && > !is_crashkernel_mem_reserved()) { > die("Memory for crashkernel is not reserved\n" > "Please reserve memory by passing" > "\"crashkernel=Y@X\" parameter to kernel\n" > "Then try to loading kdump kernel\n"); > } > > ...... > } > > > > > > > > > > > However, even though we want to do the check, doing like below is much > > > easier and more reasonable. > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > index 45637511e0de..4d1339bd2ccf 100644 > > > --- a/kernel/kexec_file.c > > > +++ b/kernel/kexec_file.c > > > @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > dest_image = &kexec_image; > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > + if (!crash_memory_valid()) > > > + return -EINVAL; > > > dest_image = &kexec_crash_image; > > > if (kexec_crash_image) > > > arch_kexec_unprotect_crashkres(); > > > > > > So, I am wondering if there is an issue encountered if we don't do the > > > check in kernel. > > > > > > Thanks > > > Baoquan > > > > > > > > > > > --- > > > > > > > > To: Eric Biederman <ebiederm@xmission.com> > > > > Cc: kexec@lists.infradead.org > > > > Cc: linux-kernel@vger.kernel.org > > > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > > > Cc: linux-kernel@vger.kernel.org > > > > Cc: Ross Zwisler <zwisler@kernel.org> > > > > Cc: Philipp Rudo <prudo@redhat.com> > > > > Cc: Baoquan He <bhe@redhat.com> > > > > --- > > > > include/linux/kexec.h | 1 + > > > > kernel/kexec.c | 9 +++++---- > > > > kernel/kexec_core.c | 5 +++++ > > > > kernel/kexec_file.c | 7 ++++--- > > > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > > > index 41a686996aaa..98ca9a32bc8e 100644 > > > > --- a/include/linux/kexec.h > > > > +++ b/include/linux/kexec.h > > > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > > > extern bool kexec_in_progress; > > > > > > > > int crash_shrink_memory(unsigned long new_size); > > > > +bool __crash_memory_valid(void); > > > > ssize_t crash_get_memory_size(void); > > > > > > > > #ifndef arch_kexec_protect_crashkres > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > > > index cb8e6e6f983c..b5c17db25e88 100644 > > > > --- a/kernel/kexec.c > > > > +++ b/kernel/kexec.c > > > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > struct kimage *image; > > > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > > > > > - if (kexec_on_panic) { > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > /* Verify we have a valid entry point */ > > > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > > > (entry > phys_to_boot_phys(crashk_res.end))) > > > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > image->nr_segments = nr_segments; > > > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > > > > > - if (kexec_on_panic) { > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > /* Enable special crash kernel control page alloc policy. */ > > > > image->control_page = crashk_res.start; > > > > image->type = KEXEC_TYPE_CRASH; > > > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > > > if (flags & KEXEC_ON_CRASH) { > > > > dest_image = &kexec_crash_image; > > > > - if (kexec_crash_image) > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > arch_kexec_unprotect_crashkres(); > > > > } else { > > > > dest_image = &kexec_image; > > > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > image = xchg(dest_image, image); > > > > > > > > out: > > > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > > > + __crash_memory_valid()) > > > > arch_kexec_protect_crashkres(); > > > > > > > > kimage_free(image); > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > > > index ca2743f9c634..77083c9760fb 100644 > > > > --- a/kernel/kexec_core.c > > > > +++ b/kernel/kexec_core.c > > > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > > > } > > > > } > > > > > > > > +bool __crash_memory_valid(void) > > > > +{ > > > > + return crashk_res.end != crashk_res.start; > > > > +} > > > > + > > > > ssize_t crash_get_memory_size(void) > > > > { > > > > ssize_t size = 0; > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > index 45637511e0de..0671f4f370ff 100644 > > > > --- a/kernel/kexec_file.c > > > > +++ b/kernel/kexec_file.c > > > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > > > > > image->file_mode = 1; > > > > > > > > - if (kexec_on_panic) { > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > /* Enable special crash kernel control page alloc policy. */ > > > > image->control_page = crashk_res.start; > > > > image->type = KEXEC_TYPE_CRASH; > > > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > dest_image = &kexec_image; > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > dest_image = &kexec_crash_image; > > > > - if (kexec_crash_image) > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > arch_kexec_unprotect_crashkres(); > > > > } > > > > > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > exchange: > > > > image = xchg(dest_image, image); > > > > out: > > > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > > > + __crash_memory_valid()) > > > > arch_kexec_protect_crashkres(); > > > > > > > > kexec_unlock(); > > > > > > > > --- > > > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > > > > > Best regards, > > > > -- > > > > Ricardo Ribalda <ribalda@chromium.org> > > > > > > > > > > > > > -- > > Ricardo Ribalda > > >
On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > Hi Baoquan > > Thanks for your review! > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > Usually crash_image is defined statically via the crashkernel parameter > > > or DT. > > > > > > But if the crash kernel is not used, or is smaller than then > > > area pre-allocated that memory is wasted. > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > use the crash kernel. > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > defined statically. Following the same memory allocation/validation path > > > that for the reboot kexec kernel. > > > > We don't check if the crashkernel memory region is valid in kernel, but > > we do have done the check in kexec-tools utility. Since both kexec_load and > > kexec_file_load need go through path of kexec-tools loading, we haven't > > got problem with lack of the checking in kernel. > > Not sure if I follow you. > > We currently check if the crash kernel is in the right place at > sanity_check_segment_list() > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 And it's not checking if crashkernel memory is valid in sanity_check_segment_list(), right? It's checking if the segments are placed correctly.
Hi On Fri, 25 Nov 2022 at 08:27, Baoquan He <bhe@redhat.com> wrote: > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > > Hi Baoquan > > > > Thanks for your review! > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > > Usually crash_image is defined statically via the crashkernel parameter > > > > or DT. > > > > > > > > But if the crash kernel is not used, or is smaller than then > > > > area pre-allocated that memory is wasted. > > > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > > use the crash kernel. > > > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > > defined statically. Following the same memory allocation/validation path > > > > that for the reboot kexec kernel. > > > > > > We don't check if the crashkernel memory region is valid in kernel, but > > > we do have done the check in kexec-tools utility. Since both kexec_load and > > > kexec_file_load need go through path of kexec-tools loading, we haven't > > > got problem with lack of the checking in kernel. > > > > Not sure if I follow you. > > > > We currently check if the crash kernel is in the right place at > > sanity_check_segment_list() > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > And it's not checking if crashkernel memory is valid in > sanity_check_segment_list(), right? It's checking if the segments > are placed correctly. If it is not valid, then this condition is not met. /* Ensure we are within the crash kernel limits */ if ((mstart < phys_to_boot_phys(crashk_res.start)) || (mend > phys_to_boot_phys(crashk_res.end))) return -EADDRNOTAVAIL; >
On 11/25/22 at 08:26am, Ricardo Ribalda wrote: > Hi Baoquan > > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote: > > > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > > > Hi Baoquan > > > > > > Thanks for your review! > > > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > > > Usually crash_image is defined statically via the crashkernel parameter > > > > > or DT. > > > > > > > > > > But if the crash kernel is not used, or is smaller than then > > > > > area pre-allocated that memory is wasted. > > > > > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > > > use the crash kernel. > > > > > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > > > defined statically. Following the same memory allocation/validation path > > > > > that for the reboot kexec kernel. > > > > > > > > We don't check if the crashkernel memory region is valid in kernel, but > > > > we do have done the check in kexec-tools utility. Since both kexec_load and > > > > kexec_file_load need go through path of kexec-tools loading, we haven't > > > > got problem with lack of the checking in kernel. > > > > > > Not sure if I follow you. > > > > > > We currently check if the crash kernel is in the right place at > > > sanity_check_segment_list() > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > > > Please check below code in kexec-tools utility, currently we have to use > > kexec -p to enter into kexec_load or kexec_file_load system call. Before > > entering system call, we have below code: > > So your concern is that the current kexec-tools does not let you pass > a crashkernel unless there is memory reserved for it? No, my concern is why we have to do the check in kernel if we have done that in kexec-tools utility. You didn't say your kexec-lite need this until now. I think it's fine to add the check in kernel if you prefer to do the check in kernel, but not in kexec-lite. The motivation or reason you want to make the change is very important. > > Once the changes land in the kernel I can make a patch for that. I am > currently using this to test the code: > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c > > > > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c > > > > int main(int argc, char *argv[]) > > { > > ...... > > if (do_load && > > ((kexec_flags & KEXEC_ON_CRASH) || > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) && > > !is_crashkernel_mem_reserved()) { > > die("Memory for crashkernel is not reserved\n" > > "Please reserve memory by passing" > > "\"crashkernel=Y@X\" parameter to kernel\n" > > "Then try to loading kdump kernel\n"); > > } > > > > ...... > > } > > > > > > > > > > > > > > > > However, even though we want to do the check, doing like below is much > > > > easier and more reasonable. > > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > index 45637511e0de..4d1339bd2ccf 100644 > > > > --- a/kernel/kexec_file.c > > > > +++ b/kernel/kexec_file.c > > > > @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > > > dest_image = &kexec_image; > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > + if (!crash_memory_valid()) > > > > + return -EINVAL; > > > > dest_image = &kexec_crash_image; > > > > if (kexec_crash_image) > > > > arch_kexec_unprotect_crashkres(); > > > > > > > > So, I am wondering if there is an issue encountered if we don't do the > > > > check in kernel. > > > > > > > > Thanks > > > > Baoquan > > > > > > > > > > > > > > --- > > > > > > > > > > To: Eric Biederman <ebiederm@xmission.com> > > > > > Cc: kexec@lists.infradead.org > > > > > Cc: linux-kernel@vger.kernel.org > > > > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > > > > Cc: linux-kernel@vger.kernel.org > > > > > Cc: Ross Zwisler <zwisler@kernel.org> > > > > > Cc: Philipp Rudo <prudo@redhat.com> > > > > > Cc: Baoquan He <bhe@redhat.com> > > > > > --- > > > > > include/linux/kexec.h | 1 + > > > > > kernel/kexec.c | 9 +++++---- > > > > > kernel/kexec_core.c | 5 +++++ > > > > > kernel/kexec_file.c | 7 ++++--- > > > > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > > > > index 41a686996aaa..98ca9a32bc8e 100644 > > > > > --- a/include/linux/kexec.h > > > > > +++ b/include/linux/kexec.h > > > > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > > > > extern bool kexec_in_progress; > > > > > > > > > > int crash_shrink_memory(unsigned long new_size); > > > > > +bool __crash_memory_valid(void); > > > > > ssize_t crash_get_memory_size(void); > > > > > > > > > > #ifndef arch_kexec_protect_crashkres > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > > > > index cb8e6e6f983c..b5c17db25e88 100644 > > > > > --- a/kernel/kexec.c > > > > > +++ b/kernel/kexec.c > > > > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > > struct kimage *image; > > > > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > > > > > > > - if (kexec_on_panic) { > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > /* Verify we have a valid entry point */ > > > > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > > > > (entry > phys_to_boot_phys(crashk_res.end))) > > > > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > > image->nr_segments = nr_segments; > > > > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > > > > > > > - if (kexec_on_panic) { > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > /* Enable special crash kernel control page alloc policy. */ > > > > > image->control_page = crashk_res.start; > > > > > image->type = KEXEC_TYPE_CRASH; > > > > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > > > > > if (flags & KEXEC_ON_CRASH) { > > > > > dest_image = &kexec_crash_image; > > > > > - if (kexec_crash_image) > > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > > arch_kexec_unprotect_crashkres(); > > > > > } else { > > > > > dest_image = &kexec_image; > > > > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > image = xchg(dest_image, image); > > > > > > > > > > out: > > > > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > > > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > > > > + __crash_memory_valid()) > > > > > arch_kexec_protect_crashkres(); > > > > > > > > > > kimage_free(image); > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > > > > index ca2743f9c634..77083c9760fb 100644 > > > > > --- a/kernel/kexec_core.c > > > > > +++ b/kernel/kexec_core.c > > > > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > > > > } > > > > > } > > > > > > > > > > +bool __crash_memory_valid(void) > > > > > +{ > > > > > + return crashk_res.end != crashk_res.start; > > > > > +} > > > > > + > > > > > ssize_t crash_get_memory_size(void) > > > > > { > > > > > ssize_t size = 0; > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > > index 45637511e0de..0671f4f370ff 100644 > > > > > --- a/kernel/kexec_file.c > > > > > +++ b/kernel/kexec_file.c > > > > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > > > > > > > image->file_mode = 1; > > > > > > > > > > - if (kexec_on_panic) { > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > /* Enable special crash kernel control page alloc policy. */ > > > > > image->control_page = crashk_res.start; > > > > > image->type = KEXEC_TYPE_CRASH; > > > > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > dest_image = &kexec_image; > > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > > dest_image = &kexec_crash_image; > > > > > - if (kexec_crash_image) > > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > > arch_kexec_unprotect_crashkres(); > > > > > } > > > > > > > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > exchange: > > > > > image = xchg(dest_image, image); > > > > > out: > > > > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > > > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > > > > + __crash_memory_valid()) > > > > > arch_kexec_protect_crashkres(); > > > > > > > > > > kexec_unlock(); > > > > > > > > > > --- > > > > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > > > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > > > > > > > Best regards, > > > > > -- > > > > > Ricardo Ribalda <ribalda@chromium.org> > > > > > > > > > > > > > > > > > > -- > > > Ricardo Ribalda > > > > > > > > -- > Ricardo Ribalda >
On 11/25/22 at 08:31am, Ricardo Ribalda wrote: > Hi > > On Fri, 25 Nov 2022 at 08:27, Baoquan He <bhe@redhat.com> wrote: > > > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > > > Hi Baoquan > > > > > > Thanks for your review! > > > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > > > Usually crash_image is defined statically via the crashkernel parameter > > > > > or DT. > > > > > > > > > > But if the crash kernel is not used, or is smaller than then > > > > > area pre-allocated that memory is wasted. > > > > > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > > > use the crash kernel. > > > > > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > > > defined statically. Following the same memory allocation/validation path > > > > > that for the reboot kexec kernel. > > > > > > > > We don't check if the crashkernel memory region is valid in kernel, but > > > > we do have done the check in kexec-tools utility. Since both kexec_load and > > > > kexec_file_load need go through path of kexec-tools loading, we haven't > > > > got problem with lack of the checking in kernel. > > > > > > Not sure if I follow you. > > > > > > We currently check if the crash kernel is in the right place at > > > sanity_check_segment_list() > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > > > And it's not checking if crashkernel memory is valid in > > sanity_check_segment_list(), right? It's checking if the segments > > are placed correctly. > > If it is not valid, then this condition is not met. Yeah. BUT, even though it's valid, below condition could not be met either. They are not the same thing. > > /* Ensure we are within the crash kernel limits */ > if ((mstart < phys_to_boot_phys(crashk_res.start)) || > (mend > phys_to_boot_phys(crashk_res.end))) > return -EADDRNOTAVAIL; > > > > > > > -- > Ricardo Ribalda >
Hi Baoquan On Fri, 25 Nov 2022 at 08:45, Baoquan He <bhe@redhat.com> wrote: > > On 11/25/22 at 08:26am, Ricardo Ribalda wrote: > > Hi Baoquan > > > > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote: > > > > > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > > > > Hi Baoquan > > > > > > > > Thanks for your review! > > > > > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > > > > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > > > > Usually crash_image is defined statically via the crashkernel parameter > > > > > > or DT. > > > > > > > > > > > > But if the crash kernel is not used, or is smaller than then > > > > > > area pre-allocated that memory is wasted. > > > > > > > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > > > > use the crash kernel. > > > > > > > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > > > > defined statically. Following the same memory allocation/validation path > > > > > > that for the reboot kexec kernel. > > > > > > > > > > We don't check if the crashkernel memory region is valid in kernel, but > > > > > we do have done the check in kexec-tools utility. Since both kexec_load and > > > > > kexec_file_load need go through path of kexec-tools loading, we haven't > > > > > got problem with lack of the checking in kernel. > > > > > > > > Not sure if I follow you. > > > > > > > > We currently check if the crash kernel is in the right place at > > > > sanity_check_segment_list() > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > > > > > Please check below code in kexec-tools utility, currently we have to use > > > kexec -p to enter into kexec_load or kexec_file_load system call. Before > > > entering system call, we have below code: > > > > So your concern is that the current kexec-tools does not let you pass > > a crashkernel unless there is memory reserved for it? > > No, my concern is why we have to do the check in kernel if we have done > that in kexec-tools utility. You didn't say your kexec-lite need this > until now. I think it's fine to add the check in kernel if you prefer to > do the check in kernel, but not in kexec-lite. > > The motivation or reason you want to make the change is very important. kexec-lite is just to test the kernel code. It is easier to follow than kexec-utils and supports 32bit userspace on a 64bit kernel. I think it was clear. The motivation is to enable the use of crashkernel when it is not statically predefined. Any suggestions on how I can make it more clear? > > > > > > Once the changes land in the kernel I can make a patch for that. I am > > currently using this to test the code: > > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c > > > > > > > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c > > > > > > int main(int argc, char *argv[]) > > > { > > > ...... > > > if (do_load && > > > ((kexec_flags & KEXEC_ON_CRASH) || > > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) && > > > !is_crashkernel_mem_reserved()) { > > > die("Memory for crashkernel is not reserved\n" > > > "Please reserve memory by passing" > > > "\"crashkernel=Y@X\" parameter to kernel\n" > > > "Then try to loading kdump kernel\n"); Having that check ALSO is unserspace is fine. It lets kexec show a more meaningful error message. But we should not rely on userspace checks. This patch is not about adding an extra check on the kernel, but to enable extra functionaliry. > > > } > > > > > > ...... > > > } > > > > > > > > > > > > > > > > > > > > > However, even though we want to do the check, doing like below is much > > > > > easier and more reasonable. > > > > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > > index 45637511e0de..4d1339bd2ccf 100644 > > > > > --- a/kernel/kexec_file.c > > > > > +++ b/kernel/kexec_file.c > > > > > @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > > > > > dest_image = &kexec_image; > > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > > + if (!crash_memory_valid()) > > > > > + return -EINVAL; > > > > > dest_image = &kexec_crash_image; > > > > > if (kexec_crash_image) > > > > > arch_kexec_unprotect_crashkres(); > > > > > > > > > > So, I am wondering if there is an issue encountered if we don't do the > > > > > check in kernel. > > > > > > > > > > Thanks > > > > > Baoquan > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > To: Eric Biederman <ebiederm@xmission.com> > > > > > > Cc: kexec@lists.infradead.org > > > > > > Cc: linux-kernel@vger.kernel.org > > > > > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > > > > > Cc: linux-kernel@vger.kernel.org > > > > > > Cc: Ross Zwisler <zwisler@kernel.org> > > > > > > Cc: Philipp Rudo <prudo@redhat.com> > > > > > > Cc: Baoquan He <bhe@redhat.com> > > > > > > --- > > > > > > include/linux/kexec.h | 1 + > > > > > > kernel/kexec.c | 9 +++++---- > > > > > > kernel/kexec_core.c | 5 +++++ > > > > > > kernel/kexec_file.c | 7 ++++--- > > > > > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > > > > > index 41a686996aaa..98ca9a32bc8e 100644 > > > > > > --- a/include/linux/kexec.h > > > > > > +++ b/include/linux/kexec.h > > > > > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > > > > > extern bool kexec_in_progress; > > > > > > > > > > > > int crash_shrink_memory(unsigned long new_size); > > > > > > +bool __crash_memory_valid(void); > > > > > > ssize_t crash_get_memory_size(void); > > > > > > > > > > > > #ifndef arch_kexec_protect_crashkres > > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > > > > > index cb8e6e6f983c..b5c17db25e88 100644 > > > > > > --- a/kernel/kexec.c > > > > > > +++ b/kernel/kexec.c > > > > > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > > > struct kimage *image; > > > > > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > > > > > > > > > - if (kexec_on_panic) { > > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > > /* Verify we have a valid entry point */ > > > > > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > > > > > (entry > phys_to_boot_phys(crashk_res.end))) > > > > > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > > > image->nr_segments = nr_segments; > > > > > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > > > > > > > > > - if (kexec_on_panic) { > > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > > /* Enable special crash kernel control page alloc policy. */ > > > > > > image->control_page = crashk_res.start; > > > > > > image->type = KEXEC_TYPE_CRASH; > > > > > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > > > > > > > if (flags & KEXEC_ON_CRASH) { > > > > > > dest_image = &kexec_crash_image; > > > > > > - if (kexec_crash_image) > > > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > > > arch_kexec_unprotect_crashkres(); > > > > > > } else { > > > > > > dest_image = &kexec_image; > > > > > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > image = xchg(dest_image, image); > > > > > > > > > > > > out: > > > > > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > > > > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > > > > > + __crash_memory_valid()) > > > > > > arch_kexec_protect_crashkres(); > > > > > > > > > > > > kimage_free(image); > > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > > > > > index ca2743f9c634..77083c9760fb 100644 > > > > > > --- a/kernel/kexec_core.c > > > > > > +++ b/kernel/kexec_core.c > > > > > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > > > > > } > > > > > > } > > > > > > > > > > > > +bool __crash_memory_valid(void) > > > > > > +{ > > > > > > + return crashk_res.end != crashk_res.start; > > > > > > +} > > > > > > + > > > > > > ssize_t crash_get_memory_size(void) > > > > > > { > > > > > > ssize_t size = 0; > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > > > index 45637511e0de..0671f4f370ff 100644 > > > > > > --- a/kernel/kexec_file.c > > > > > > +++ b/kernel/kexec_file.c > > > > > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > > > > > > > > > image->file_mode = 1; > > > > > > > > > > > > - if (kexec_on_panic) { > > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > > /* Enable special crash kernel control page alloc policy. */ > > > > > > image->control_page = crashk_res.start; > > > > > > image->type = KEXEC_TYPE_CRASH; > > > > > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > dest_image = &kexec_image; > > > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > > > dest_image = &kexec_crash_image; > > > > > > - if (kexec_crash_image) > > > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > > > arch_kexec_unprotect_crashkres(); > > > > > > } > > > > > > > > > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > exchange: > > > > > > image = xchg(dest_image, image); > > > > > > out: > > > > > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > > > > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > > > > > + __crash_memory_valid()) > > > > > > arch_kexec_protect_crashkres(); > > > > > > > > > > > > kexec_unlock(); > > > > > > > > > > > > --- > > > > > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > > > > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > > > > > > > > > Best regards, > > > > > > -- > > > > > > Ricardo Ribalda <ribalda@chromium.org> > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Ricardo Ribalda > > > > > > > > > > > > > -- > > Ricardo Ribalda > > >
On 11/25/22 at 09:10am, Ricardo Ribalda wrote: > Hi Baoquan > > On Fri, 25 Nov 2022 at 08:45, Baoquan He <bhe@redhat.com> wrote: > > > > On 11/25/22 at 08:26am, Ricardo Ribalda wrote: > > > Hi Baoquan > > > > > > On Fri, 25 Nov 2022 at 08:15, Baoquan He <bhe@redhat.com> wrote: > > > > > > > > On 11/25/22 at 06:52am, Ricardo Ribalda wrote: > > > > > Hi Baoquan > > > > > > > > > > Thanks for your review! > > > > > > > > > > On Fri, 25 Nov 2022 at 03:58, Baoquan He <bhe@redhat.com> wrote: > > > > > > > > > > > > On 11/24/22 at 11:23pm, Ricardo Ribalda wrote: > > > > > > > Usually crash_image is defined statically via the crashkernel parameter > > > > > > > or DT. > > > > > > > > > > > > > > But if the crash kernel is not used, or is smaller than then > > > > > > > area pre-allocated that memory is wasted. > > > > > > > > > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > > > > > use the crash kernel. > > > > > > > > > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > > > > > defined statically. Following the same memory allocation/validation path > > > > > > > that for the reboot kexec kernel. > > > > > > > > > > > > We don't check if the crashkernel memory region is valid in kernel, but > > > > > > we do have done the check in kexec-tools utility. Since both kexec_load and > > > > > > kexec_file_load need go through path of kexec-tools loading, we haven't > > > > > > got problem with lack of the checking in kernel. > > > > > > > > > > Not sure if I follow you. > > > > > > > > > > We currently check if the crash kernel is in the right place at > > > > > sanity_check_segment_list() > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/kexec_core.c#n239 > > > > > > > > Please check below code in kexec-tools utility, currently we have to use > > > > kexec -p to enter into kexec_load or kexec_file_load system call. Before > > > > entering system call, we have below code: > > > > > > So your concern is that the current kexec-tools does not let you pass > > > a crashkernel unless there is memory reserved for it? > > > > No, my concern is why we have to do the check in kernel if we have done > > that in kexec-tools utility. You didn't say your kexec-lite need this > > until now. I think it's fine to add the check in kernel if you prefer to > > do the check in kernel, but not in kexec-lite. > > > > The motivation or reason you want to make the change is very important. > > kexec-lite is just to test the kernel code. It is easier to follow > than kexec-utils and supports 32bit userspace on a 64bit kernel. > > > I think it was clear. The motivation is to enable the use of > crashkernel when it is not statically predefined. I thought you are adding a check on crashkernel region validation. Now I am totally lost how this patch can enable the use of crashkernel. I will wait a while to see if other people understand it. > > Any suggestions on how I can make it more clear? > > > > > > > > > > > > > Once the changes land in the kernel I can make a patch for that. I am > > > currently using this to test the code: > > > > > > https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3953579/4/kexec-lite/kexec-lite.c > > > > > > > > > > > https://kernel.googlesource.com/pub/scm/utils/kernel/kexec/kexec-tools.git/+/refs/heads/master/kexec/kexec.c > > > > > > > > int main(int argc, char *argv[]) > > > > { > > > > ...... > > > > if (do_load && > > > > ((kexec_flags & KEXEC_ON_CRASH) || > > > > (kexec_file_flags & KEXEC_FILE_ON_CRASH)) && > > > > !is_crashkernel_mem_reserved()) { > > > > die("Memory for crashkernel is not reserved\n" > > > > "Please reserve memory by passing" > > > > "\"crashkernel=Y@X\" parameter to kernel\n" > > > > "Then try to loading kdump kernel\n"); > > Having that check ALSO is unserspace is fine. It lets kexec show a > more meaningful error message. But we should not rely on userspace > checks. > > This patch is not about adding an extra check on the kernel, but to > enable extra functionaliry. > > > > > } > > > > > > > > ...... > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > However, even though we want to do the check, doing like below is much > > > > > > easier and more reasonable. > > > > > > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > > > index 45637511e0de..4d1339bd2ccf 100644 > > > > > > --- a/kernel/kexec_file.c > > > > > > +++ b/kernel/kexec_file.c > > > > > > @@ -344,6 +344,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > > > > > > > dest_image = &kexec_image; > > > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > > > + if (!crash_memory_valid()) > > > > > > + return -EINVAL; > > > > > > dest_image = &kexec_crash_image; > > > > > > if (kexec_crash_image) > > > > > > arch_kexec_unprotect_crashkres(); > > > > > > > > > > > > So, I am wondering if there is an issue encountered if we don't do the > > > > > > check in kernel. > > > > > > > > > > > > Thanks > > > > > > Baoquan > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > To: Eric Biederman <ebiederm@xmission.com> > > > > > > > Cc: kexec@lists.infradead.org > > > > > > > Cc: linux-kernel@vger.kernel.org > > > > > > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > > > > > > Cc: linux-kernel@vger.kernel.org > > > > > > > Cc: Ross Zwisler <zwisler@kernel.org> > > > > > > > Cc: Philipp Rudo <prudo@redhat.com> > > > > > > > Cc: Baoquan He <bhe@redhat.com> > > > > > > > --- > > > > > > > include/linux/kexec.h | 1 + > > > > > > > kernel/kexec.c | 9 +++++---- > > > > > > > kernel/kexec_core.c | 5 +++++ > > > > > > > kernel/kexec_file.c | 7 ++++--- > > > > > > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > > > > > > index 41a686996aaa..98ca9a32bc8e 100644 > > > > > > > --- a/include/linux/kexec.h > > > > > > > +++ b/include/linux/kexec.h > > > > > > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > > > > > > extern bool kexec_in_progress; > > > > > > > > > > > > > > int crash_shrink_memory(unsigned long new_size); > > > > > > > +bool __crash_memory_valid(void); > > > > > > > ssize_t crash_get_memory_size(void); > > > > > > > > > > > > > > #ifndef arch_kexec_protect_crashkres > > > > > > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > > > > > > index cb8e6e6f983c..b5c17db25e88 100644 > > > > > > > --- a/kernel/kexec.c > > > > > > > +++ b/kernel/kexec.c > > > > > > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > > > > struct kimage *image; > > > > > > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > > > > > > > > > > > - if (kexec_on_panic) { > > > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > > > /* Verify we have a valid entry point */ > > > > > > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > > > > > > (entry > phys_to_boot_phys(crashk_res.end))) > > > > > > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > > > > > image->nr_segments = nr_segments; > > > > > > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > > > > > > > > > > > - if (kexec_on_panic) { > > > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > > > /* Enable special crash kernel control page alloc policy. */ > > > > > > > image->control_page = crashk_res.start; > > > > > > > image->type = KEXEC_TYPE_CRASH; > > > > > > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > > > > > > > > > if (flags & KEXEC_ON_CRASH) { > > > > > > > dest_image = &kexec_crash_image; > > > > > > > - if (kexec_crash_image) > > > > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > > > > arch_kexec_unprotect_crashkres(); > > > > > > > } else { > > > > > > > dest_image = &kexec_image; > > > > > > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > > image = xchg(dest_image, image); > > > > > > > > > > > > > > out: > > > > > > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > > > > > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > > > > > > + __crash_memory_valid()) > > > > > > > arch_kexec_protect_crashkres(); > > > > > > > > > > > > > > kimage_free(image); > > > > > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > > > > > > index ca2743f9c634..77083c9760fb 100644 > > > > > > > --- a/kernel/kexec_core.c > > > > > > > +++ b/kernel/kexec_core.c > > > > > > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +bool __crash_memory_valid(void) > > > > > > > +{ > > > > > > > + return crashk_res.end != crashk_res.start; > > > > > > > +} > > > > > > > + > > > > > > > ssize_t crash_get_memory_size(void) > > > > > > > { > > > > > > > ssize_t size = 0; > > > > > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > > > > > index 45637511e0de..0671f4f370ff 100644 > > > > > > > --- a/kernel/kexec_file.c > > > > > > > +++ b/kernel/kexec_file.c > > > > > > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > > > > > > > > > > > image->file_mode = 1; > > > > > > > > > > > > > > - if (kexec_on_panic) { > > > > > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > > > > > /* Enable special crash kernel control page alloc policy. */ > > > > > > > image->control_page = crashk_res.start; > > > > > > > image->type = KEXEC_TYPE_CRASH; > > > > > > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > > dest_image = &kexec_image; > > > > > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > > > > > dest_image = &kexec_crash_image; > > > > > > > - if (kexec_crash_image) > > > > > > > + if (kexec_crash_image && __crash_memory_valid()) > > > > > > > arch_kexec_unprotect_crashkres(); > > > > > > > } > > > > > > > > > > > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > > > > > exchange: > > > > > > > image = xchg(dest_image, image); > > > > > > > out: > > > > > > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > > > > > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > > > > > > + __crash_memory_valid()) > > > > > > > arch_kexec_protect_crashkres(); > > > > > > > > > > > > > > kexec_unlock(); > > > > > > > > > > > > > > --- > > > > > > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > > > > > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > > > > > > > > > > > Best regards, > > > > > > > -- > > > > > > > Ricardo Ribalda <ribalda@chromium.org> > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Ricardo Ribalda > > > > > > > > > > > > > > > > > > -- > > > Ricardo Ribalda > > > > > > > > -- > Ricardo Ribalda >
Hi Ricardo, On Thu, 24 Nov 2022 23:23:36 +0100 Ricardo Ribalda <ribalda@chromium.org> wrote: > Usually crash_image is defined statically via the crashkernel parameter > or DT. > > But if the crash kernel is not used, or is smaller than then > area pre-allocated that memory is wasted. > > Also, if the crash kernel was not defined at bootime, there is no way to > use the crash kernel. > > Enable runtime allocation of the crash_image if the crash_image is not > defined statically. Following the same memory allocation/validation path > that for the reboot kexec kernel. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> I don't think this patch will work as intended. For one you omit setting the image->type to KEXEC_TYPE_CRASH. But when you grep for that type you will find that there is a lot of special handling done for it. I don't believe that this can simply be skipped without causing problems. Furthermore I think you have missed one important detail. The memory reserved for the crash kernel is not just a buffer for the image but the memory it runs in! For that it has to be a continuous piece of physical memory with usually some additional arch specific limitations. When allocated dynamically all those limitations need to be considered. But a standard kexec doesn't care about those limitations as it doesn't care about the os running before itself. It can simply overwrite the memory when booting. But if the crash kernel does the same it will corrupt the dump it is supposed to generate. Thanks Philipp > --- > kexec: Enable runtime allocation of crash_image > > To: Eric Biederman <ebiederm@xmission.com> > Cc: kexec@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > Cc: linux-kernel@vger.kernel.org > Cc: Ross Zwisler <zwisler@kernel.org> > Cc: Philipp Rudo <prudo@redhat.com> > Cc: Baoquan He <bhe@redhat.com> > --- > include/linux/kexec.h | 1 + > kernel/kexec.c | 9 +++++---- > kernel/kexec_core.c | 5 +++++ > kernel/kexec_file.c | 7 ++++--- > 4 files changed, 15 insertions(+), 7 deletions(-) > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > index 41a686996aaa..98ca9a32bc8e 100644 > --- a/include/linux/kexec.h > +++ b/include/linux/kexec.h > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > extern bool kexec_in_progress; > > int crash_shrink_memory(unsigned long new_size); > +bool __crash_memory_valid(void); > ssize_t crash_get_memory_size(void); > > #ifndef arch_kexec_protect_crashkres > diff --git a/kernel/kexec.c b/kernel/kexec.c > index cb8e6e6f983c..b5c17db25e88 100644 > --- a/kernel/kexec.c > +++ b/kernel/kexec.c > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > struct kimage *image; > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > - if (kexec_on_panic) { > + if (kexec_on_panic && __crash_memory_valid()) { > /* Verify we have a valid entry point */ > if ((entry < phys_to_boot_phys(crashk_res.start)) || > (entry > phys_to_boot_phys(crashk_res.end))) > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > image->nr_segments = nr_segments; > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > - if (kexec_on_panic) { > + if (kexec_on_panic && __crash_memory_valid()) { > /* Enable special crash kernel control page alloc policy. */ > image->control_page = crashk_res.start; > image->type = KEXEC_TYPE_CRASH; > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > if (flags & KEXEC_ON_CRASH) { > dest_image = &kexec_crash_image; > - if (kexec_crash_image) > + if (kexec_crash_image && __crash_memory_valid()) > arch_kexec_unprotect_crashkres(); > } else { > dest_image = &kexec_image; > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > image = xchg(dest_image, image); > > out: > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > + __crash_memory_valid()) > arch_kexec_protect_crashkres(); > > kimage_free(image); > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > index ca2743f9c634..77083c9760fb 100644 > --- a/kernel/kexec_core.c > +++ b/kernel/kexec_core.c > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > } > } > > +bool __crash_memory_valid(void) > +{ > + return crashk_res.end != crashk_res.start; > +} > + > ssize_t crash_get_memory_size(void) > { > ssize_t size = 0; > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index 45637511e0de..0671f4f370ff 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > image->file_mode = 1; > > - if (kexec_on_panic) { > + if (kexec_on_panic && __crash_memory_valid()) { > /* Enable special crash kernel control page alloc policy. */ > image->control_page = crashk_res.start; > image->type = KEXEC_TYPE_CRASH; > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > dest_image = &kexec_image; > if (flags & KEXEC_FILE_ON_CRASH) { > dest_image = &kexec_crash_image; > - if (kexec_crash_image) > + if (kexec_crash_image && __crash_memory_valid()) > arch_kexec_unprotect_crashkres(); > } > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > exchange: > image = xchg(dest_image, image); > out: > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > + __crash_memory_valid()) > arch_kexec_protect_crashkres(); > > kexec_unlock(); > > --- > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > Best regards,
Hi Philipp Thanks for your review. On Mon, 28 Nov 2022 at 18:00, Philipp Rudo <prudo@redhat.com> wrote: > > Hi Ricardo, > > On Thu, 24 Nov 2022 23:23:36 +0100 > Ricardo Ribalda <ribalda@chromium.org> wrote: > > > Usually crash_image is defined statically via the crashkernel parameter > > or DT. > > > > But if the crash kernel is not used, or is smaller than then > > area pre-allocated that memory is wasted. > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > use the crash kernel. > > > > Enable runtime allocation of the crash_image if the crash_image is not > > defined statically. Following the same memory allocation/validation path > > that for the reboot kexec kernel. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > I don't think this patch will work as intended. For one you omit > setting the image->type to KEXEC_TYPE_CRASH. But when you grep for that > type you will find that there is a lot of special handling done for it. > I don't believe that this can simply be skipped without causing > problems. > > Furthermore I think you have missed one important detail. The memory > reserved for the crash kernel is not just a buffer for the image but > the memory it runs in! For that it has to be a continuous piece of > physical memory with usually some additional arch specific limitations. > When allocated dynamically all those limitations need to be considered. > But a standard kexec doesn't care about those limitations as it doesn't > care about the os running before itself. It can simply overwrite the > memory when booting. But if the crash kernel does the same it will > corrupt the dump it is supposed to generate. Right now, I do not intend to use it to fetch a kdump, I am using it as the image that will run when the system crashes. It seems to work fine on the two devices that I am using for tests. > > Thanks > Philipp > > > --- > > kexec: Enable runtime allocation of crash_image > > > > To: Eric Biederman <ebiederm@xmission.com> > > Cc: kexec@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > Cc: linux-kernel@vger.kernel.org > > Cc: Ross Zwisler <zwisler@kernel.org> > > Cc: Philipp Rudo <prudo@redhat.com> > > Cc: Baoquan He <bhe@redhat.com> > > --- > > include/linux/kexec.h | 1 + > > kernel/kexec.c | 9 +++++---- > > kernel/kexec_core.c | 5 +++++ > > kernel/kexec_file.c | 7 ++++--- > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > index 41a686996aaa..98ca9a32bc8e 100644 > > --- a/include/linux/kexec.h > > +++ b/include/linux/kexec.h > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > extern bool kexec_in_progress; > > > > int crash_shrink_memory(unsigned long new_size); > > +bool __crash_memory_valid(void); > > ssize_t crash_get_memory_size(void); > > > > #ifndef arch_kexec_protect_crashkres > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > index cb8e6e6f983c..b5c17db25e88 100644 > > --- a/kernel/kexec.c > > +++ b/kernel/kexec.c > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > struct kimage *image; > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > - if (kexec_on_panic) { > > + if (kexec_on_panic && __crash_memory_valid()) { > > /* Verify we have a valid entry point */ > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > (entry > phys_to_boot_phys(crashk_res.end))) > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > image->nr_segments = nr_segments; > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > - if (kexec_on_panic) { > > + if (kexec_on_panic && __crash_memory_valid()) { > > /* Enable special crash kernel control page alloc policy. */ > > image->control_page = crashk_res.start; > > image->type = KEXEC_TYPE_CRASH; > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > if (flags & KEXEC_ON_CRASH) { > > dest_image = &kexec_crash_image; > > - if (kexec_crash_image) > > + if (kexec_crash_image && __crash_memory_valid()) > > arch_kexec_unprotect_crashkres(); > > } else { > > dest_image = &kexec_image; > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > image = xchg(dest_image, image); > > > > out: > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > + __crash_memory_valid()) > > arch_kexec_protect_crashkres(); > > > > kimage_free(image); > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > index ca2743f9c634..77083c9760fb 100644 > > --- a/kernel/kexec_core.c > > +++ b/kernel/kexec_core.c > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > } > > } > > > > +bool __crash_memory_valid(void) > > +{ > > + return crashk_res.end != crashk_res.start; > > +} > > + > > ssize_t crash_get_memory_size(void) > > { > > ssize_t size = 0; > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > index 45637511e0de..0671f4f370ff 100644 > > --- a/kernel/kexec_file.c > > +++ b/kernel/kexec_file.c > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > image->file_mode = 1; > > > > - if (kexec_on_panic) { > > + if (kexec_on_panic && __crash_memory_valid()) { > > /* Enable special crash kernel control page alloc policy. */ > > image->control_page = crashk_res.start; > > image->type = KEXEC_TYPE_CRASH; > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > dest_image = &kexec_image; > > if (flags & KEXEC_FILE_ON_CRASH) { > > dest_image = &kexec_crash_image; > > - if (kexec_crash_image) > > + if (kexec_crash_image && __crash_memory_valid()) > > arch_kexec_unprotect_crashkres(); > > } > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > exchange: > > image = xchg(dest_image, image); > > out: > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > + __crash_memory_valid()) > > arch_kexec_protect_crashkres(); > > > > kexec_unlock(); > > > > --- > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > Best regards, >
Ricardo, On Mon, 28 Nov 2022 18:07:06 +0100 Ricardo Ribalda <ribalda@chromium.org> wrote: > Hi Philipp > > > Thanks for your review. > > > On Mon, 28 Nov 2022 at 18:00, Philipp Rudo <prudo@redhat.com> wrote: > > > > Hi Ricardo, > > > > On Thu, 24 Nov 2022 23:23:36 +0100 > > Ricardo Ribalda <ribalda@chromium.org> wrote: > > > > > Usually crash_image is defined statically via the crashkernel parameter > > > or DT. > > > > > > But if the crash kernel is not used, or is smaller than then > > > area pre-allocated that memory is wasted. > > > > > > Also, if the crash kernel was not defined at bootime, there is no way to > > > use the crash kernel. > > > > > > Enable runtime allocation of the crash_image if the crash_image is not > > > defined statically. Following the same memory allocation/validation path > > > that for the reboot kexec kernel. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > I don't think this patch will work as intended. For one you omit > > setting the image->type to KEXEC_TYPE_CRASH. But when you grep for that > > type you will find that there is a lot of special handling done for it. > > I don't believe that this can simply be skipped without causing > > problems. > > > > Furthermore I think you have missed one important detail. The memory > > reserved for the crash kernel is not just a buffer for the image but > > the memory it runs in! For that it has to be a continuous piece of > > physical memory with usually some additional arch specific limitations. > > When allocated dynamically all those limitations need to be considered. > > But a standard kexec doesn't care about those limitations as it doesn't > > care about the os running before itself. It can simply overwrite the > > memory when booting. But if the crash kernel does the same it will > > corrupt the dump it is supposed to generate. > > Right now, I do not intend to use it to fetch a kdump, I am using it > as the image that will run when the system crashes. the crash_image is currently all about creating a dump. If you want to change that you need to discuss the new behavior in the commit message! Please update the commit message. Thanks Philipp > > It seems to work fine on the two devices that I am using for tests. > > > > > Thanks > > Philipp > > > > > --- > > > kexec: Enable runtime allocation of crash_image > > > > > > To: Eric Biederman <ebiederm@xmission.com> > > > Cc: kexec@lists.infradead.org > > > Cc: linux-kernel@vger.kernel.org > > > Cc: Sergey Senozhatsky <senozhatsky@chromium.org> > > > Cc: linux-kernel@vger.kernel.org > > > Cc: Ross Zwisler <zwisler@kernel.org> > > > Cc: Philipp Rudo <prudo@redhat.com> > > > Cc: Baoquan He <bhe@redhat.com> > > > --- > > > include/linux/kexec.h | 1 + > > > kernel/kexec.c | 9 +++++---- > > > kernel/kexec_core.c | 5 +++++ > > > kernel/kexec_file.c | 7 ++++--- > > > 4 files changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h > > > index 41a686996aaa..98ca9a32bc8e 100644 > > > --- a/include/linux/kexec.h > > > +++ b/include/linux/kexec.h > > > @@ -427,6 +427,7 @@ extern int kexec_load_disabled; > > > extern bool kexec_in_progress; > > > > > > int crash_shrink_memory(unsigned long new_size); > > > +bool __crash_memory_valid(void); > > > ssize_t crash_get_memory_size(void); > > > > > > #ifndef arch_kexec_protect_crashkres > > > diff --git a/kernel/kexec.c b/kernel/kexec.c > > > index cb8e6e6f983c..b5c17db25e88 100644 > > > --- a/kernel/kexec.c > > > +++ b/kernel/kexec.c > > > @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > struct kimage *image; > > > bool kexec_on_panic = flags & KEXEC_ON_CRASH; > > > > > > - if (kexec_on_panic) { > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > /* Verify we have a valid entry point */ > > > if ((entry < phys_to_boot_phys(crashk_res.start)) || > > > (entry > phys_to_boot_phys(crashk_res.end))) > > > @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, > > > image->nr_segments = nr_segments; > > > memcpy(image->segment, segments, nr_segments * sizeof(*segments)); > > > > > > - if (kexec_on_panic) { > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > /* Enable special crash kernel control page alloc policy. */ > > > image->control_page = crashk_res.start; > > > image->type = KEXEC_TYPE_CRASH; > > > @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > > > > if (flags & KEXEC_ON_CRASH) { > > > dest_image = &kexec_crash_image; > > > - if (kexec_crash_image) > > > + if (kexec_crash_image && __crash_memory_valid()) > > > arch_kexec_unprotect_crashkres(); > > > } else { > > > dest_image = &kexec_image; > > > @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, > > > image = xchg(dest_image, image); > > > > > > out: > > > - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) > > > + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && > > > + __crash_memory_valid()) > > > arch_kexec_protect_crashkres(); > > > > > > kimage_free(image); > > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > > index ca2743f9c634..77083c9760fb 100644 > > > --- a/kernel/kexec_core.c > > > +++ b/kernel/kexec_core.c > > > @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) > > > } > > > } > > > > > > +bool __crash_memory_valid(void) > > > +{ > > > + return crashk_res.end != crashk_res.start; > > > +} > > > + > > > ssize_t crash_get_memory_size(void) > > > { > > > ssize_t size = 0; > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > > > index 45637511e0de..0671f4f370ff 100644 > > > --- a/kernel/kexec_file.c > > > +++ b/kernel/kexec_file.c > > > @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, > > > > > > image->file_mode = 1; > > > > > > - if (kexec_on_panic) { > > > + if (kexec_on_panic && __crash_memory_valid()) { > > > /* Enable special crash kernel control page alloc policy. */ > > > image->control_page = crashk_res.start; > > > image->type = KEXEC_TYPE_CRASH; > > > @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > dest_image = &kexec_image; > > > if (flags & KEXEC_FILE_ON_CRASH) { > > > dest_image = &kexec_crash_image; > > > - if (kexec_crash_image) > > > + if (kexec_crash_image && __crash_memory_valid()) > > > arch_kexec_unprotect_crashkres(); > > > } > > > > > > @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, > > > exchange: > > > image = xchg(dest_image, image); > > > out: > > > - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) > > > + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && > > > + __crash_memory_valid()) > > > arch_kexec_protect_crashkres(); > > > > > > kexec_unlock(); > > > > > > --- > > > base-commit: 4312098baf37ee17a8350725e6e0d0e8590252d4 > > > change-id: 20221124-kexec-noalloc-3cab3cbe000f > > > > > > Best regards, > > > >
diff --git a/include/linux/kexec.h b/include/linux/kexec.h index 41a686996aaa..98ca9a32bc8e 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -427,6 +427,7 @@ extern int kexec_load_disabled; extern bool kexec_in_progress; int crash_shrink_memory(unsigned long new_size); +bool __crash_memory_valid(void); ssize_t crash_get_memory_size(void); #ifndef arch_kexec_protect_crashkres diff --git a/kernel/kexec.c b/kernel/kexec.c index cb8e6e6f983c..b5c17db25e88 100644 --- a/kernel/kexec.c +++ b/kernel/kexec.c @@ -28,7 +28,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, struct kimage *image; bool kexec_on_panic = flags & KEXEC_ON_CRASH; - if (kexec_on_panic) { + if (kexec_on_panic && __crash_memory_valid()) { /* Verify we have a valid entry point */ if ((entry < phys_to_boot_phys(crashk_res.start)) || (entry > phys_to_boot_phys(crashk_res.end))) @@ -44,7 +44,7 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, image->nr_segments = nr_segments; memcpy(image->segment, segments, nr_segments * sizeof(*segments)); - if (kexec_on_panic) { + if (kexec_on_panic && __crash_memory_valid()) { /* Enable special crash kernel control page alloc policy. */ image->control_page = crashk_res.start; image->type = KEXEC_TYPE_CRASH; @@ -101,7 +101,7 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, if (flags & KEXEC_ON_CRASH) { dest_image = &kexec_crash_image; - if (kexec_crash_image) + if (kexec_crash_image && __crash_memory_valid()) arch_kexec_unprotect_crashkres(); } else { dest_image = &kexec_image; @@ -157,7 +157,8 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments, image = xchg(dest_image, image); out: - if ((flags & KEXEC_ON_CRASH) && kexec_crash_image) + if ((flags & KEXEC_ON_CRASH) && kexec_crash_image && + __crash_memory_valid()) arch_kexec_protect_crashkres(); kimage_free(image); diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index ca2743f9c634..77083c9760fb 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -1004,6 +1004,11 @@ void crash_kexec(struct pt_regs *regs) } } +bool __crash_memory_valid(void) +{ + return crashk_res.end != crashk_res.start; +} + ssize_t crash_get_memory_size(void) { ssize_t size = 0; diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index 45637511e0de..0671f4f370ff 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -280,7 +280,7 @@ kimage_file_alloc_init(struct kimage **rimage, int kernel_fd, image->file_mode = 1; - if (kexec_on_panic) { + if (kexec_on_panic && __crash_memory_valid()) { /* Enable special crash kernel control page alloc policy. */ image->control_page = crashk_res.start; image->type = KEXEC_TYPE_CRASH; @@ -345,7 +345,7 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, dest_image = &kexec_image; if (flags & KEXEC_FILE_ON_CRASH) { dest_image = &kexec_crash_image; - if (kexec_crash_image) + if (kexec_crash_image && __crash_memory_valid()) arch_kexec_unprotect_crashkres(); } @@ -408,7 +408,8 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, initrd_fd, exchange: image = xchg(dest_image, image); out: - if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image) + if ((flags & KEXEC_FILE_ON_CRASH) && kexec_crash_image && + __crash_memory_valid()) arch_kexec_protect_crashkres(); kexec_unlock();