Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp1972147yba; Fri, 10 May 2019 04:20:10 -0700 (PDT) X-Google-Smtp-Source: APXvYqxnfoX2B1padLSLCZXhE/wZ1PJXT82A2uJOfP6Y70Q2sPu8vofY+m5BHkV1ZLUaqtsF/4bx X-Received: by 2002:a63:fc55:: with SMTP id r21mr12607523pgk.441.1557487210628; Fri, 10 May 2019 04:20:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1557487210; cv=none; d=google.com; s=arc-20160816; b=ih0iqbls0u+C6+IIZ1+2X2xZhN7UkNysOLOVovbhptCbrw+qdc9bhRCg0AmttFjDxJ hh8NGS9tekP2Wi71uLmBANZSg2lqo6pINSH676aKD/OOUzUpil5PVLm3YGPxnHbnb8Yf PpLWUv9gQepxRPkKsWvcRnsYb8aRFlAN8V81zAH67qmo7ywLv+jybermXD5yek3dDcSB GczIHDSuvDT/eBclMzxCGZvyOMPGjkydvubSDbeuLIFNg8kYBTI1Fdx69BR4/NrZVSrX R3XC0oRzOYK2P7ERf56DReupmClUdpFRarhC4cHulhfZv8W5EAd7UFJlneKE0LScQDjf /C0Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=qU0tD7CT9Jm6uTpFuovdKUn0C14kT62XFFwJuIH/foI=; b=LlftbXLMMsk3GaVIIGK6iIwGHwrNzwXTFO3hoy8lmkj2UWYIvet44KDdKS2B6EcdOM gOfRj2BvfdUc+sxURvvpG8qaHitB9LrYoJ0McNDTNtZROCBaNkPvGoBpgFmwxW4yY+sY hzNVV4ZgPDDr/P4hK0cAFTGKywkPSiVa+njCNq7Kxq0ZPAeZzxAzrfWiqmcL9yuD8oA8 eEhyfvCsvITD5FmmF0diOt9Kqwb34PxDSs33jxyPxMLRPUmSUyiVQUFhTsLjtA2MSKZl JnFLdQ1Qy37wEFNouSXfMTRFjWyguRYI0NFtH8qncWMJTi7WL4LkSpfxqfZfzDYNda7l SGJA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id b9si6940854pgm.510.2019.05.10.04.19.53; Fri, 10 May 2019 04:20:10 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727352AbfEJLRV (ORCPT + 99 others); Fri, 10 May 2019 07:17:21 -0400 Received: from mail-pg1-f195.google.com ([209.85.215.195]:46150 "EHLO mail-pg1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727052AbfEJLRU (ORCPT ); Fri, 10 May 2019 07:17:20 -0400 Received: by mail-pg1-f195.google.com with SMTP id t187so2864365pgb.13 for ; Fri, 10 May 2019 04:17:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=qU0tD7CT9Jm6uTpFuovdKUn0C14kT62XFFwJuIH/foI=; b=kY0Owjt4IP0FW7PkfXhYGVua1AhZsADMH1vHshvJwhsNT5kot5+3bi+ix6qeSTYzly /ykDUXIyuzO3hzsdkD9SRZrOrQurBCbAaDZgYcVjtT3DT+R0sx8r6y8L1pY4RwHHYcCj yqrKpVb25HfNO98SDMimSOTsbPXwySpz6uzpPs6hb2m6r/atFWZDxEDCn12YfuzhzInB BmusOOWGeFJVuDE+lQxUss427jDASGui1sshSIFhCyMp9XiMqbDRpE5eRy5R4lHe52HE nOFVWII09MWYmqLgZrTA30qm6SEv2Al6p3EMLoe2exRhIf8ORR7Lz1jYJcPsE0Slvekz kxew== X-Gm-Message-State: APjAAAVyVe3oPYs1T0OV15uuk2PsllKS/x4oKiSUqui+eYl+3mCuhvnW SlC5oSyl68qsrUkmoWLKvTP7CX958lM= X-Received: by 2002:aa7:800e:: with SMTP id j14mr12906339pfi.157.1557487040094; Fri, 10 May 2019 04:17:20 -0700 (PDT) Received: from localhost.localdomain ([110.227.183.79]) by smtp.gmail.com with ESMTPSA id g2sm8819491pfd.134.2019.05.10.04.17.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 May 2019 04:17:18 -0700 (PDT) Subject: Re: [RFC PATCH] vmcore: Add a kernel cmdline device_dump_limit To: Kairui Song , linux-kernel@vger.kernel.org Cc: "kexec@lists.infradead.org" , "David S . Miller" , Rahul Lakkireddy , Eric Biederman , Andrew Morton , Dave Young , Alexey Dobriyan , Ganesh Goudar References: <20190510102051.25647-1-kasong@redhat.com> From: Bhupesh Sharma Message-ID: <4f453ec6-67a6-2c8f-2aab-acb54ae55645@redhat.com> Date: Fri, 10 May 2019 16:47:13 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20190510102051.25647-1-kasong@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Kairui, Thanks for the patch. Please see my comments in-line: On 05/10/2019 03:50 PM, Kairui Song wrote: > Device dump allow drivers to add device related dump data to vmcore as > they want. This have a potential issue, the data is stored in memory, > drivers may append too much data and use too much memory. The vmcore is > typically used in a kdump kernel which runs in a pre-reserved small > chunk of memory. So as a result it will make kdump unusable at all due > to OOM issues. > > So introduce new device_dump_limit= kernel parameter, and set the > default limit to 0, so device dump is not enabled unless user specify > the accetable maxiam ^^^^ acceptable maximum > memory usage for device dump data. In this way user > will also have the chance to adjust the kdump reserved memory > accordingly. Hmmm., this doesn't give much confidence with the PROC_VMCORE_DEVICE_DUMP feature in its current shape. Rather shouldn't we be enabling config PROC_VMCORE_DEVICE_DUMP only under EXPERT mode for now, considering that this feature needs further thrashing and testing with real setups including platforms where drivers append large amounts of data to vmcore: diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig index 817c02b13b1d..c47a12cf7fc0 100644 --- a/fs/proc/Kconfig +++ b/fs/proc/Kconfig @@ -45,7 +45,7 @@ config PROC_VMCORE Exports the dump image of crashed kernel in ELF format. config PROC_VMCORE_DEVICE_DUMP - bool "Device Hardware/Firmware Log Collection" + bool "Device Hardware/Firmware Log Collection" if EXPERT depends on PROC_VMCORE default n help @@ -59,6 +59,12 @@ config PROC_VMCORE_DEVICE_DUMP If you say Y here, the collected device dumps will be added as ELF notes to /proc/vmcore. + Considering that there can be device drivers which append + large amounts of data to vmcore, you should say N here unless + you are reserving a large chunk of memory for crashdump + kernel, because otherwise the crashdump kernel might become + unusable due to OOM issues. + May be you can add a 'Fixes:' tag here. > Signed-off-by: Kairui Song > --- > fs/proc/vmcore.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c > index 3fe90443c1bb..e28695ef2439 100644 > --- a/fs/proc/vmcore.c > +++ b/fs/proc/vmcore.c > @@ -53,6 +53,9 @@ static struct proc_dir_entry *proc_vmcore; > /* Device Dump list and mutex to synchronize access to list */ > static LIST_HEAD(vmcoredd_list); > static DEFINE_MUTEX(vmcoredd_mutex); > + > +/* Device Dump Limit */ > +static size_t vmcoredd_limit; > #endif /* CONFIG_PROC_VMCORE_DEVICE_DUMP */ > > /* Device Dump Size */ > @@ -1465,6 +1468,11 @@ int vmcore_add_device_dump(struct vmcoredd_data *data) > data_size = roundup(sizeof(struct vmcoredd_header) + data->size, > PAGE_SIZE); > > + if (vmcoredd_orig_sz + data_size >= vmcoredd_limit) { > + ret = -ENOMEM; Should we be adding a WARN() here to let the user know that the device dump data will not be available in vmcore? > + goto out_err; > + } > + > /* Allocate buffer for driver's to write their dumps */ > buf = vmcore_alloc_buf(data_size); > if (!buf) { > @@ -1502,6 +1510,18 @@ int vmcore_add_device_dump(struct vmcoredd_data *data) > return ret; > } > EXPORT_SYMBOL(vmcore_add_device_dump); > + > +static int __init parse_vmcoredd_limit(char *arg) > +{ > + char *end; > + > + if (!arg) > + return -EINVAL; > + vmcoredd_limit = memparse(arg, &end); > + return end > arg ? 0 : -EINVAL; > + > +} > +__setup("device_dump_limit=", parse_vmcoredd_limit); We should be adding this boot argument and its description to 'Documentation/admin-guide/kernel-parameters.txt' > #endif /* CONFIG_PROC_VMCORE_DEVICE_DUMP */ > > /* Free all dumps in vmcore device dump list */ > Thanks, Bhupesh