Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp332409yba; Thu, 16 May 2019 01:20:40 -0700 (PDT) X-Google-Smtp-Source: APXvYqy6+tbHPwJcllNeuZHFoFC+8VnZi5GNIiOIW7vcKfAJxXmxZxD3PNfpCq+Tt4N1m3w6qilq X-Received: by 2002:a63:61cf:: with SMTP id v198mr49240300pgb.29.1557994840094; Thu, 16 May 2019 01:20:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1557994840; cv=none; d=google.com; s=arc-20160816; b=rVMl/c3GxXGMni494z9Enz45LKuymptKrC6GfxeNZzcsAwYPj/Ue9yBAkLL9UwGLB2 dkNR0lodmS+DCQ5SJg8ORNeI3ldBZqdU5XJcmwknzJgALMHgwXSIeqssILJ8q0am272C q0+KipVtJTIkin+nXL7IxExYkAyQi/VjoJx6SXsJxr3U22r0QteBON57dx8OkDM7jkWC yLx9tvH6bTlmPdY8Bglw9FZ/kibYTtfa/y2feRRGlraiY/9+q9wlkbY4uoq98OJYN4wE A0nDZvVCFYjI/zZwTHKbBNwDTfQhi2RUxJYH+PQbp827VOGrX92XnLfCwUMfpfuViMhK knzg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version; bh=s68yQo+QQvCZajbWc1oevyw/oT7lrvS4cXS+T8Bmo2M=; b=wOTJgYkAy8kq7tDTFFDR3I5vSl4/ZrZpwOB/tX13F2KyRZy6rGlVOv2EuJ2a017e4X BOjEHg14pQ/OlhLwzyKenjHnNI1c2eBzGXDKeBIFB/G4NeHUTMvwd07iyL+sLzRvJTL5 jyWzouH/l1S93u+4vL1Cg7JKo0f9yDqiGH8sNEfRjOGovWRn5k3CpX337wqiOeUbSoSn HkRxR3iyHgAblSrgANRy2mao825jyA+JArNujJeO/+bDTgnPlzL1RKNgSXmIzFXttxPA 1fdKrwoyH2HMjsd3cjUBWz9dfHawzj69r79/FL2sSpw1e1vQDrNn+9SPhzoL+c8/ND9G XAtA== 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 j23si4640927pff.159.2019.05.16.01.20.24; Thu, 16 May 2019 01:20:40 -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 S1726486AbfEPITT (ORCPT + 99 others); Thu, 16 May 2019 04:19:19 -0400 Received: from mail-it1-f193.google.com ([209.85.166.193]:37624 "EHLO mail-it1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726272AbfEPITT (ORCPT ); Thu, 16 May 2019 04:19:19 -0400 Received: by mail-it1-f193.google.com with SMTP id m140so4786968itg.2 for ; Thu, 16 May 2019 01:19:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=s68yQo+QQvCZajbWc1oevyw/oT7lrvS4cXS+T8Bmo2M=; b=EwphtlJ63VR9Yn8Ov0HSG/7H1y1l97MkP6vXNWnzJiuPCaIE2FkVZVjDYnoozd0IaM Fg6/PHMKy2T4d4D4F0lTywdb2S4BHDk9aaifwJoW8TwZ7oL6N2i8Juf+bOrmw4eHGXYf PmQQlJG854i1FWeCaHZ7QQFahHzvxp4bDqDgm5MipZ3onnJoeib6jfJg+E4oMcw6a74K UXHNRUFYC/6nhayqnDk4tK3ej3kBdbiWNkrJeyCJPyVTH5aqbIOSCCgpKCkXauLxLAmu yq0G8LqZtpyyM2DZFmIGKuIdSMXUkbPBvkCzI9kCzP2ukvN1iCDQdnH3OBRRfJB+9sz8 6pew== X-Gm-Message-State: APjAAAXGDvTcpYZLyKFrdOS2HMyZ48eUaQZ5oGNu8UZ69q2e6c7q4nhK 6zFVO7doD2LBrqoZyjzrbBvplisRu5vKc1zX3pKlSw== X-Received: by 2002:a02:37d7:: with SMTP id r206mr32206828jar.127.1557994757853; Thu, 16 May 2019 01:19:17 -0700 (PDT) MIME-Version: 1.0 References: <20190510102051.25647-1-kasong@redhat.com> <4f453ec6-67a6-2c8f-2aab-acb54ae55645@redhat.com> In-Reply-To: <4f453ec6-67a6-2c8f-2aab-acb54ae55645@redhat.com> From: Kairui Song Date: Thu, 16 May 2019 16:19:06 +0800 Message-ID: Subject: Re: [RFC PATCH] vmcore: Add a kernel cmdline device_dump_limit To: Bhupesh Sharma Cc: Linux Kernel Mailing List , "kexec@lists.infradead.org" , "David S . Miller" , Rahul Lakkireddy , Eric Biederman , Andrew Morton , Dave Young , Alexey Dobriyan , Ganesh Goudar Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 10, 2019 at 7:17 PM Bhupesh Sharma wrote: > > 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 Will fix this typo. > > > 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: I think no need to move it to expert mode, just leave it disabled by default should be better, that should be enough to make sure driver won't append that much memory and cause OOM, while it could still be enabled without changing the kernel, so this feature won't bring extra risk, and could be enabled anytime easily. > > 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. Problem is previous commit seems not broken, just bring extra memory stress. Is "Fixes:" tag suitable for this commit? > > > 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? Yes, that could be very helpful. How about pr_err_once? WARN is too noise, just give a hint to the user that device dump is disabled should be enough, so user will know why device dump data is not present and will just enable it. > > > + 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' Good suggestion, will update the document. > > > #endif /* CONFIG_PROC_VMCORE_DEVICE_DUMP */ > > > > /* Free all dumps in vmcore device dump list */ > > > > Thanks, > Bhupesh Thanks for the review! -- Best Regards, Kairui Song