From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1Ya2Y0-0005NR-7T for kexec@lists.infradead.org; Mon, 23 Mar 2015 13:37:41 +0000 Date: Mon, 23 Mar 2015 09:37:10 -0400 From: Vivek Goyal Subject: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path Message-ID: <20150323133710.GA3172@redhat.com> References: <54F9D645.2050008@jp.fujitsu.com> <20150323034752.GD2068@dhcp-16-105.nay.redhat.com> <20150323071943.GA22765@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150323071943.GA22765@gmail.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Ingo Molnar Cc: Baoquan He , kexec@lists.infradead.org, linux-kernel@vger.kernel.org, hidehiro.kawai.ez@hitachi.com, =?utf-8?B?IkhhdGF5YW1hLCBEYWlzdWtlL+eVkeWxsSDlpKfovJQi?= , mingo@redhat.com, ebiederm@xmission.com, masami.hiramatsu.pt@hitachi.com, akpm@linux-foundation.org, bp@suse.de T24gTW9uLCBNYXIgMjMsIDIwMTUgYXQgMDg6MTk6NDNBTSArMDEwMCwgSW5nbyBNb2xuYXIgd3Jv dGU6Cj4gCj4gKiBCYW9xdWFuIEhlIDxiaGVAcmVkaGF0LmNvbT4gd3JvdGU6Cj4gCj4gPiBDQyBt b3JlIHBlb3BsZSAuLi4KPiA+IAo+ID4gT24gMDMvMDcvMTUgYXQgMDE6MzFhbSwgIkhhdGF5YW1h LCBEYWlzdWtlL+eVkeWxsSDlpKfovJQiIHdyb3RlOgo+ID4gPiBUaGUgY29tbWl0IGYwNmU1MTUz ZjRhZTJlMmYzYjAzMDBmMGUyNjBlNDBjYjdmZWZkNDUgaW50cm9kdWNlZAo+ID4gPiAiY3Jhc2hf a2V4ZWNfcG9zdF9ub3RpZmllcnMiIGtlcm5lbCBib290IG9wdGlvbiwgd2hpY2ggdG9nZ2xlcwo+ ID4gPiB3aGVhdGhlciBwYW5pYygpIGNhbGxzIGNyYXNoX2tleGVjKCkgYmVmb3JlIHBhbmljX25v dGlmaWVycyBhbmQgZHVtcAo+ID4gPiBrbXNnIG9yIGFmdGVyLgo+ID4gPiAKPiA+ID4gVGhlIHBy b2JsZW0gaXMgdGhhdCB0aGUgY29tbWl0IG92ZXJsb29rcyBwYW5pY19vbl9vb3BzIGtlcm5lbCBi b290Cj4gPiA+IG9wdGlvbi4gSWYgaXQgaXMgZW5hYmxlZCwgY3Jhc2hfa2V4ZWMoKSBpcyBjYWxs ZWQgZGlyZWN0bHkgd2l0aG91dAo+ID4gPiBnb2luZyB0aHJvdWdoIHBhbmljKCkgaW4gb29wcyBw YXRoLgo+ID4gPiAKPiA+ID4gVG8gZml4IHRoaXMgaXNzdWUsIHRoaXMgcGF0Y2ggYWRkcyBhIGNo ZWNrIHRvCj4gPiA+ICJjcmFzaF9rZXhlY19wb3N0X25vdGlmaWVycyIgaW4gdGhlIGNvbmRpdGlv biBvZiBrZXhlY19zaG91bGRfY3Jhc2goKS4KPiA+ID4gCj4gPiA+IEFsc28sIHB1dCBhIGNvbW1l bnQgaW4ga2V4ZWNfc2hvdWxkX2NyYXNoKCkgdG8gZXhwbGFpbiBub3Qgb2J2aW91cwo+ID4gPiB0 aGluZ3Mgb24gdGhpcyBwYXRjaC4KPiA+ID4gCj4gPiA+IFNpZ25lZC1vZmYtYnk6IEhBVEFZQU1B IERhaXN1a2UgPGQuaGF0YXlhbWFAanAuZnVqaXRzdS5jb20+Cj4gPiA+IEFja2VkLWJ5OiBCYW9x dWFuIEhlIDxiaGVAcmVkaGF0LmNvbT4KPiA+ID4gVGVzdGVkLWJ5OiBIaWRlaGlybyBLYXdhaSA8 aGlkZWhpcm8ua2F3YWkuZXpAaGl0YWNoaS5jb20+Cj4gPiA+IFJldmlld2VkLWJ5OiBNYXNhbWkg SGlyYW1hdHN1IDxtYXNhbWkuaGlyYW1hdHN1LnB0QGhpdGFjaGkuY29tPgo+ID4gPiAtLS0KPiA+ ID4gIGluY2x1ZGUvbGludXgva2VybmVsLmggfCAgMyArKysKPiA+ID4gIGtlcm5lbC9rZXhlYy5j ICAgICAgICAgfCAxMSArKysrKysrKysrKwo+ID4gPiAga2VybmVsL3BhbmljLmMgICAgICAgICB8 ICAyICstCj4gPiA+ICAzIGZpbGVzIGNoYW5nZWQsIDE1IGluc2VydGlvbnMoKyksIDEgZGVsZXRp b24oLSkKPiAKPiBUaGlzIGlzIGhhY2sgdXBvbiBoYWNrLCBidXQgd2h5IHdhcyB0aGlzIGNyYXAg bWVyZ2VkIGluIHRoZSBmaXJzdCAKPiBwbGFjZT8KPiAKPiBJIHNlZSB0d28gcHJvYmxlbXMganVz dCBieSBjdXJzb3J5IHJldmlldzoKPiAKPiAxKQo+IAo+IEZpcnN0bHksIHRoZSByZWFsIGJ1ZyBp bjoKPiAKPiAgIGYwNmU1MTUzZjRhZSAoImtlcm5lbC9wYW5pYy5jOiBhZGQgImNyYXNoX2tleGVj X3Bvc3Rfbm90aWZpZXJzIiBvcHRpb24gZm9yIGtkdW1wIGFmdGVyIHBhbmljX25vdGlmZXJzIikK PiAKPiBXYXMgdGhhdCBjcmFzaF9rZXhlYygpIHdhcyBjYWxsZWQgdW5jb25kaXRpb25hbGx5IGFm dGVyIG5vdGlmaWVycyB3ZXJlIAo+IGNhbGxlZCwgd2hpY2ggc2hvdWxkIGJlIGZpeGVkIHZpYSB0 aGUgc2ltcGxlIHBhdGNoIGJlbG93ICh1bnRlc3RlZCkuIAo+IExvb2tzIG11Y2ggc2ltcGxlciB0 aGFuIHlvdXIgZml4Lgo+IAoKSGkgSW5nbywKCkFncmVlZC4gWW91ciBwYXRjaCBsb29rcyBnb29k LgoKPiAyKQo+IAo+IFNlY29uZGx5LCBhbmQgbW9yZSBpbXBvcnRhbnRseSwgdGhlIHdob2xlIHBy ZW1pc2Ugb2YgY29tbWl0IAo+IGYwNmU1MTUzZjRhZSBpcyBicm9rZW4gSU1ITzoKPiAKPiAgIlRo aXMgY2FuIGhlbHAgcmFyZSBzaXR1YXRpb25zIHdoZXJlIGtkdW1wIGZhaWxzIGJlY2F1c2Ugb2Yg dW5zdGFibGUKPiAgIGNyYXNoZWQga2VybmVsIG9yIGhhcmR3YXJlIGZhaWx1cmUgKG1lbW9yeSBj b3JydXB0aW9uIG9uIGNyaXRpY2FsCj4gICBkYXRhL2NvZGUpIgo+IAo+IHd0Zj8KPiAKPiBJZiB0 aGUga2VybmVsIGNyYXNoZWQgZHVlIHRvIGEga2VybmVsIGNyYXNoLCB0aGVuIHRoZSBrZXJuZWwg Ym9vdGluZyAKPiB1cCBpbiB3aGF0ZXZlciBoYXJkd2FyZSBzdGF0ZSBzaG91bGQgYmUgYWJsZSB0 byBkbyBhIGNsZWFuIGJvb3R1cC4gVGhlIAo+IGZpeCBmb3IgdGhvc2UgJ3JhcmUgc2l0dWF0aW9u cycgc2hvdWxkIGJlIHRvIGZpeCB0aGUgcmVhbCBidWcgKGZvciAKPiBleGFtcGxlIGJ5IG1ha2lu ZyBoYXJkd2FyZSBkcml2ZXIgaW5pdCAob3IgZGVpbml0KSBzZXF1ZW5jZXMgbW9yZSAKPiByb2J1 c3QpLCBub3QgdG8gcGFwZXIgaXQgb3ZlciBieSBvcmRlcmluZyBhcm91bmQgY3Jhc2gtdGltZSBz ZXF1ZW5jZXMgCj4gLi4uCj4gCj4gSWYgaXQgY3Jhc2hlZCBkdWUgdG8gc29tZSBoYXJkd2FyZSBm YWlsdXJlLCB0aGVyZSdzIGxpdGVyYWxseSBhbiAKPiBpbmZpbml0ZSBhbW91bnQgb2YgZmFpbHVy ZSBtb2RlcyB0aGF0IG1heSBvciBtYXkgbm90IGJlIGltcGFjdGVkIGJ5IAo+IGtleGVjIGNyYXNo LXRpbWUgaGFuZGxpbmcgb3JkZXJpbmcuIFdlIGRvbid0IHdhbnQgdG8gcHV0IGEgemlsbGlvbiAK PiBzdWNoIGZsYWdzIGludG8gdGhlIGtlcm5lbCBwcm9wZXIganVzdCB0byBhbGxvdyB0aGUgcGVy dHVyYmF0aW9uIG9mIAo+IHRoZSBrZXJuZWwuCgpJIHRoaW5rIG9uZSBvZiB0aGUgbW90aXZhdGlv bnMgYmVoaW5kIHRoaXMgcGF0Y2ggd2FzIGNhbGwgdG8ga21zZ19kdW1wKCkuClNvbWUgdmVuZG9y cyBoYXZlIGJlZW4gd2FudGluZyB0byBoYXZlIHRoZSBjYXBhYmlsaXR5IHRvIHNhdmUga2VybmVs IGxvZ3MKdG8gc29tZSBOVlJBTSBiZWZvcmUgdHJhbnNpdGlvbiB0byBzZWNvbmQga2VybmVsIGhh cHBlbnMuIFRoZWlyIGFyZ3VtZW50CmlzIHRoYXQga2R1bXAgZG9lcyBub3Qgc3VjY2VlZCBhbGwg dGhlIHRpbWUgYW5kIGlmIGtkdW1wIGRvZXMgbm90IHN1Y2NlZWQKdGhlbiBhdGxlYXN0IHRoZXkg aGF2ZSBzb21ldGhpbmcgdG8gd29yayB3aXRoIChrZXJuZWwgbG9ncyByZXRyaWV2ZWQKZnJvbSBw c3RvcmUgaW50ZXJmYWNlKS4KCk5vdCB0aGF0IEkgYWdyZWUgZnVsbHkgd2l0aCB0aGlzIGFzIHBy b2JsZW0gbWlnaHQgaGFwcGVuIHdoaWxlIHdlIHRyeQp0byBydW4gcGFuaWNfbm90aWZpZXJzIG9y IGttc2dfZHVtcCBob29rcyBhbmQgbmV2ZXIgdHJhbnNpdGlvbiBpbnRvCmtkdW1wIGtlcm5lbC4K CkFuZCBpdCBoYXMgYmVlbiBsaXRlcmFsbHkgeWVhcnMgc2luY2Ugc29tZSBkZXZlbG9wZXJzIGhh dmUgYmVlbiBwdXNoaW5nIGZvcgphbGxvd2luZyB0byBydW4gcGFuaWMgbm90aWZpZXJzIGJlZm9y ZSBjcmFzaF9rZXhlYygpLiBFcmljIEJpZWRlcm1hbiBoYXMgYmVlbgpwdXNoaW5nIGJhY2sgc2F5 aW5nIGl0IHJlZHVjZXMgdGhlIHJlbGlhYmlsaXR5IG9mIGtkdW1wIG9wZXJhdGlvbiBzbyB0aGlz CmlzIG5vdCBhY2NlcHRhYmxlLgoKU28gd2hpbGUgaXQgaXMgdmVyeSBoYWNreSwgdGhpcyBjb21t YW5kIGxpbmUgb3B0aW9uIHdhcyBpbnRvcmR1Y2VkIHdoaWNoCmFsbG93ZWQgdG8gb3ZlcnJpZGUg ZGVmYXVsdCBjcmFzaF9rZXhlYygpIGJlaGF2aW9yIGFuZCB0aG9zZSB3aG8gd2FudAp0byBkbyBh ZGRpdGlvbmFsIHRoaW5ncyAoYXQgdGhlaXIgb3duIHJpc2spIGJlZm9yZSB0cmFuc2l0aW9uIHRv IHNlY29uZAprZXJuZWwsIGNhbiBzcGVjaWZ5IHRoaXMgcGFyYW1ldGVyLgoKVGhhbmtzClZpdmVr Cgo+IAo+IGRpZmYgLS1naXQgYS9rZXJuZWwvcGFuaWMuYyBiL2tlcm5lbC9wYW5pYy5jCj4gaW5k ZXggODEzNmFkNzZlNWZkLi43NzQ2MTRmNzJjYmQgMTAwNjQ0Cj4gLS0tIGEva2VybmVsL3Bhbmlj LmMKPiArKysgYi9rZXJuZWwvcGFuaWMuYwo+IEBAIC0xNDIsNyArMTQyLDggQEAgdm9pZCBwYW5p Yyhjb25zdCBjaGFyICpmbXQsIC4uLikKPiAgCSAqIE5vdGU6IHNpbmNlIHNvbWUgcGFuaWNfbm90 aWZpZXJzIGNhbiBtYWtlIGNyYXNoZWQga2VybmVsCj4gIAkgKiBtb3JlIHVuc3RhYmxlLCBpdCBj YW4gaW5jcmVhc2Ugcmlza3Mgb2YgdGhlIGtkdW1wIGZhaWx1cmUgdG9vLgo+ICAJICovCj4gLQlj cmFzaF9rZXhlYyhOVUxMKTsKPiArCWlmIChjcmFzaF9rZXhlY19wb3N0X25vdGlmaWVycykKPiAr CQljcmFzaF9rZXhlYyhOVUxMKTsKPiAgCj4gIAlidXN0X3NwaW5sb2NrcygwKTsKPiAgCgpfX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwprZXhlYyBtYWlsaW5n IGxpc3QKa2V4ZWNAbGlzdHMuaW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9y Zy9tYWlsbWFuL2xpc3RpbmZvL2tleGVjCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752446AbbCWNhp (ORCPT ); Mon, 23 Mar 2015 09:37:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35887 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752205AbbCWNhn (ORCPT ); Mon, 23 Mar 2015 09:37:43 -0400 Date: Mon, 23 Mar 2015 09:37:10 -0400 From: Vivek Goyal To: Ingo Molnar Cc: Baoquan He , =?utf-8?B?IkhhdGF5YW1hLCBEYWlzdWtlL+eVkeWxsSDlpKfovJQi?= , ebiederm@xmission.com, masami.hiramatsu.pt@hitachi.com, hidehiro.kawai.ez@hitachi.com, linux-kernel@vger.kernel.org, kexec@lists.infradead.org, akpm@linux-foundation.org, mingo@redhat.com, bp@suse.de Subject: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path Message-ID: <20150323133710.GA3172@redhat.com> References: <54F9D645.2050008@jp.fujitsu.com> <20150323034752.GD2068@dhcp-16-105.nay.redhat.com> <20150323071943.GA22765@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150323071943.GA22765@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 23, 2015 at 08:19:43AM +0100, Ingo Molnar wrote: > > * Baoquan He wrote: > > > CC more people ... > > > > On 03/07/15 at 01:31am, "Hatayama, Daisuke/畑山 大輔" wrote: > > > The commit f06e5153f4ae2e2f3b0300f0e260e40cb7fefd45 introduced > > > "crash_kexec_post_notifiers" kernel boot option, which toggles > > > wheather panic() calls crash_kexec() before panic_notifiers and dump > > > kmsg or after. > > > > > > The problem is that the commit overlooks panic_on_oops kernel boot > > > option. If it is enabled, crash_kexec() is called directly without > > > going through panic() in oops path. > > > > > > To fix this issue, this patch adds a check to > > > "crash_kexec_post_notifiers" in the condition of kexec_should_crash(). > > > > > > Also, put a comment in kexec_should_crash() to explain not obvious > > > things on this patch. > > > > > > Signed-off-by: HATAYAMA Daisuke > > > Acked-by: Baoquan He > > > Tested-by: Hidehiro Kawai > > > Reviewed-by: Masami Hiramatsu > > > --- > > > include/linux/kernel.h | 3 +++ > > > kernel/kexec.c | 11 +++++++++++ > > > kernel/panic.c | 2 +- > > > 3 files changed, 15 insertions(+), 1 deletion(-) > > This is hack upon hack, but why was this crap merged in the first > place? > > I see two problems just by cursory review: > > 1) > > Firstly, the real bug in: > > f06e5153f4ae ("kernel/panic.c: add "crash_kexec_post_notifiers" option for kdump after panic_notifers") > > Was that crash_kexec() was called unconditionally after notifiers were > called, which should be fixed via the simple patch below (untested). > Looks much simpler than your fix. > Hi Ingo, Agreed. Your patch looks good. > 2) > > Secondly, and more importantly, the whole premise of commit > f06e5153f4ae is broken IMHO: > > "This can help rare situations where kdump fails because of unstable > crashed kernel or hardware failure (memory corruption on critical > data/code)" > > wtf? > > If the kernel crashed due to a kernel crash, then the kernel booting > up in whatever hardware state should be able to do a clean bootup. The > fix for those 'rare situations' should be to fix the real bug (for > example by making hardware driver init (or deinit) sequences more > robust), not to paper it over by ordering around crash-time sequences > ... > > If it crashed due to some hardware failure, there's literally an > infinite amount of failure modes that may or may not be impacted by > kexec crash-time handling ordering. We don't want to put a zillion > such flags into the kernel proper just to allow the perturbation of > the kernel. I think one of the motivations behind this patch was call to kmsg_dump(). Some vendors have been wanting to have the capability to save kernel logs to some NVRAM before transition to second kernel happens. Their argument is that kdump does not succeed all the time and if kdump does not succeed then atleast they have something to work with (kernel logs retrieved from pstore interface). Not that I agree fully with this as problem might happen while we try to run panic_notifiers or kmsg_dump hooks and never transition into kdump kernel. And it has been literally years since some developers have been pushing for allowing to run panic notifiers before crash_kexec(). Eric Biederman has been pushing back saying it reduces the reliability of kdump operation so this is not acceptable. So while it is very hacky, this command line option was intorduced which allowed to override default crash_kexec() behavior and those who want to do additional things (at their own risk) before transition to second kernel, can specify this parameter. Thanks Vivek > > diff --git a/kernel/panic.c b/kernel/panic.c > index 8136ad76e5fd..774614f72cbd 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -142,7 +142,8 @@ void panic(const char *fmt, ...) > * Note: since some panic_notifiers can make crashed kernel > * more unstable, it can increase risks of the kdump failure too. > */ > - crash_kexec(NULL); > + if (crash_kexec_post_notifiers) > + crash_kexec(NULL); > > bust_spinlocks(0); >