From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail7.hitachi.co.jp ([133.145.228.42]) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1YaFY9-0002mx-KK for kexec@lists.infradead.org; Tue, 24 Mar 2015 03:30:43 +0000 Message-ID: <5510DA42.6040708@hitachi.com> Date: Tue, 24 Mar 2015 12:30:10 +0900 From: Masami Hiramatsu MIME-Version: 1.0 Subject: Re: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path References: <54F9D645.2050008@jp.fujitsu.com> <20150323034752.GD2068@dhcp-16-105.nay.redhat.com> <20150323071943.GA22765@gmail.com> 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, =?UTF-8?B?IkhhdGF5YW1hLCBEYWlzdWtlL+eVkeWxsQ==?= =?UTF-8?B?IOWkp+i8lCI=?= , mingo@redhat.com, ebiederm@xmission.com, hidehiro.kawai.ez@hitachi.com, akpm@linux-foundation.org, bp@suse.de, Vivek Goyal KDIwMTUvMDMvMjMgMTY6MTkpLCBJbmdvIE1vbG5hciB3cm90ZToKPiAKPiAqIEJhb3F1YW4gSGUg PGJoZUByZWRoYXQuY29tPiB3cm90ZToKPiAKPj4gQ0MgbW9yZSBwZW9wbGUgLi4uCj4+Cj4+IE9u IDAzLzA3LzE1IGF0IDAxOjMxYW0sICJIYXRheWFtYSwgRGFpc3VrZS/nlZHlsbEg5aSn6LyUIiB3 cm90ZToKPj4+IFRoZSBjb21taXQgZjA2ZTUxNTNmNGFlMmUyZjNiMDMwMGYwZTI2MGU0MGNiN2Zl ZmQ0NSBpbnRyb2R1Y2VkCj4+PiAiY3Jhc2hfa2V4ZWNfcG9zdF9ub3RpZmllcnMiIGtlcm5lbCBi b290IG9wdGlvbiwgd2hpY2ggdG9nZ2xlcwo+Pj4gd2hlYXRoZXIgcGFuaWMoKSBjYWxscyBjcmFz aF9rZXhlYygpIGJlZm9yZSBwYW5pY19ub3RpZmllcnMgYW5kIGR1bXAKPj4+IGttc2cgb3IgYWZ0 ZXIuCj4+Pgo+Pj4gVGhlIHByb2JsZW0gaXMgdGhhdCB0aGUgY29tbWl0IG92ZXJsb29rcyBwYW5p Y19vbl9vb3BzIGtlcm5lbCBib290Cj4+PiBvcHRpb24uIElmIGl0IGlzIGVuYWJsZWQsIGNyYXNo X2tleGVjKCkgaXMgY2FsbGVkIGRpcmVjdGx5IHdpdGhvdXQKPj4+IGdvaW5nIHRocm91Z2ggcGFu aWMoKSBpbiBvb3BzIHBhdGguCj4+Pgo+Pj4gVG8gZml4IHRoaXMgaXNzdWUsIHRoaXMgcGF0Y2gg YWRkcyBhIGNoZWNrIHRvCj4+PiAiY3Jhc2hfa2V4ZWNfcG9zdF9ub3RpZmllcnMiIGluIHRoZSBj b25kaXRpb24gb2Yga2V4ZWNfc2hvdWxkX2NyYXNoKCkuCj4+Pgo+Pj4gQWxzbywgcHV0IGEgY29t bWVudCBpbiBrZXhlY19zaG91bGRfY3Jhc2goKSB0byBleHBsYWluIG5vdCBvYnZpb3VzCj4+PiB0 aGluZ3Mgb24gdGhpcyBwYXRjaC4KPj4+Cj4+PiBTaWduZWQtb2ZmLWJ5OiBIQVRBWUFNQSBEYWlz dWtlIDxkLmhhdGF5YW1hQGpwLmZ1aml0c3UuY29tPgo+Pj4gQWNrZWQtYnk6IEJhb3F1YW4gSGUg PGJoZUByZWRoYXQuY29tPgo+Pj4gVGVzdGVkLWJ5OiBIaWRlaGlybyBLYXdhaSA8aGlkZWhpcm8u a2F3YWkuZXpAaGl0YWNoaS5jb20+Cj4+PiBSZXZpZXdlZC1ieTogTWFzYW1pIEhpcmFtYXRzdSA8 bWFzYW1pLmhpcmFtYXRzdS5wdEBoaXRhY2hpLmNvbT4KPj4+IC0tLQo+Pj4gIGluY2x1ZGUvbGlu dXgva2VybmVsLmggfCAgMyArKysKPj4+ICBrZXJuZWwva2V4ZWMuYyAgICAgICAgIHwgMTEgKysr KysrKysrKysKPj4+ICBrZXJuZWwvcGFuaWMuYyAgICAgICAgIHwgIDIgKy0KPj4+ICAzIGZpbGVz IGNoYW5nZWQsIDE1IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkKPiAKPiBUaGlzIGlzIGhh Y2sgdXBvbiBoYWNrLCBidXQgd2h5IHdhcyB0aGlzIGNyYXAgbWVyZ2VkIGluIHRoZSBmaXJzdCAK PiBwbGFjZT8KPiAKPiBJIHNlZSB0d28gcHJvYmxlbXMganVzdCBieSBjdXJzb3J5IHJldmlldzoK PiAKPiAxKQo+IAo+IEZpcnN0bHksIHRoZSByZWFsIGJ1ZyBpbjoKPiAKPiAgIGYwNmU1MTUzZjRh ZSAoImtlcm5lbC9wYW5pYy5jOiBhZGQgImNyYXNoX2tleGVjX3Bvc3Rfbm90aWZpZXJzIiBvcHRp b24gZm9yIGtkdW1wIGFmdGVyIHBhbmljX25vdGlmZXJzIikKPiAKPiBXYXMgdGhhdCBjcmFzaF9r ZXhlYygpIHdhcyBjYWxsZWQgdW5jb25kaXRpb25hbGx5IGFmdGVyIG5vdGlmaWVycyB3ZXJlIAo+ IGNhbGxlZCwgd2hpY2ggc2hvdWxkIGJlIGZpeGVkIHZpYSB0aGUgc2ltcGxlIHBhdGNoIGJlbG93 ICh1bnRlc3RlZCkuIAo+IExvb2tzIG11Y2ggc2ltcGxlciB0aGFuIHlvdXIgZml4LgoKTm8sIERh aXN1a2UncyBwYXRjaCBpcyBub3QgZm9yIHRoYXQgY2FzZS4gU2luY2UgdGhlIGtkdW1wIGhhcyBh IHNwZWNpYWwgaG9vayBpbgprZXJuZWwgb29wcywgd2hlbiBib3RoIG9mIHBhbmljX29uX29vcHMg YW5kIGNyYXNoX2tlcm5lbCBhcmUgc2V0LCBwYW5pYygpIGlzCm5ldmVyIGNhbGxlZC4gUGxlYXNl IHNlZSBvb3BzX2VuZEBhcmNoL3g4Ni9rZXJuZWwvZHVtcHN0YWNrLmMKCi0tLS0Kdm9pZCBvb3Bz X2VuZCh1bnNpZ25lZCBsb25nIGZsYWdzLCBzdHJ1Y3QgcHRfcmVncyAqcmVncywgaW50IHNpZ25y KQp7CiAgICAgICAgaWYgKHJlZ3MgJiYga2V4ZWNfc2hvdWxkX2NyYXNoKGN1cnJlbnQpKQogICAg ICAgICAgICAgICAgY3Jhc2hfa2V4ZWMocmVncyk7Ci0tLS0KT2YgY291cnNlIGNyYXNoX2tleGVj KCkgbmV2ZXIgcmV0dXJuIGV4Y2VwdCBmYWlsaW5nIGtleGVjIHVuZXhwZWN0ZWRseS4KClRodXMs IGtleGVjX3Nob3VsZF9jcmFzaCBzaG91bGQgcmV0dXJucyAwIGlmIGNyYXNoX2tleGVjX3Bvc3Rf bm90aWZpZXJzIGlzIHNldC4KKFNlbWFudGljYWxseSwgaXQgaXMgYSBiaXQgc3RyYW5nZSB0aGF0 IHBhbmljX29uX29vcHMgZG9lc24ndCBjYWxsIHBhbmljKCksIGJ1dAp0aGF0IGlzIGFub3RoZXIg dG9waWMuKQoKSG93ZXZlciwgeW91ciBwYXRjaCBpcyBhbHNvIG5lZWRlZCBzaW5jZSB0aGUgZmly c3QgY3Jhc2hfa2V4ZWMoKSBjYW4gZmFpbCBpbiBwYW5pYygpCndoZW4gY3Jhc2hfa2V4ZWNfcG9z dF9ub3RpZmllcnMgaXMgbm90IHNldC4gSW4gdGhhdCBjYXNlLCBrZXJuZWwgdHJpZXMgdG8gY2Fs bApub3RpZmllcnMgYW5kIGNhbGwgdGhlIDJuZCBjcmFzaF9rZXhlYygpIGFnYWluLiBBY3R1YWxs eSB0aGUgMm5kIG9uZSBpcyB1c2VsZXNzLgoKU28sIGhlcmUgaXMgbXkgcmV2aWV3ZWQtYnkuCgpS ZXZpZXdlZC1ieTogTWFzYW1pIEhpcmFtYXRzdSA8bWFzYW1pLmhpcmFtYXRzdS5wdEBoaXRhY2hp LmNvbT4KCkknbGwgYmUgcmVwbHkgdGhlIGxhdHRlciBwYXJ0IGluIG90aGVyIG1haWwuCgpUaGFu ayB5b3UsCgo+IAo+IDIpCj4gCj4gU2Vjb25kbHksIGFuZCBtb3JlIGltcG9ydGFudGx5LCB0aGUg d2hvbGUgcHJlbWlzZSBvZiBjb21taXQgCj4gZjA2ZTUxNTNmNGFlIGlzIGJyb2tlbiBJTUhPOgo+ IAo+ICAiVGhpcyBjYW4gaGVscCByYXJlIHNpdHVhdGlvbnMgd2hlcmUga2R1bXAgZmFpbHMgYmVj YXVzZSBvZiB1bnN0YWJsZQo+ICAgY3Jhc2hlZCBrZXJuZWwgb3IgaGFyZHdhcmUgZmFpbHVyZSAo bWVtb3J5IGNvcnJ1cHRpb24gb24gY3JpdGljYWwKPiAgIGRhdGEvY29kZSkiCj4gCj4gd3RmPwo+ IAo+IElmIHRoZSBrZXJuZWwgY3Jhc2hlZCBkdWUgdG8gYSBrZXJuZWwgY3Jhc2gsIHRoZW4gdGhl IGtlcm5lbCBib290aW5nIAo+IHVwIGluIHdoYXRldmVyIGhhcmR3YXJlIHN0YXRlIHNob3VsZCBi ZSBhYmxlIHRvIGRvIGEgY2xlYW4gYm9vdHVwLiBUaGUgCj4gZml4IGZvciB0aG9zZSAncmFyZSBz aXR1YXRpb25zJyBzaG91bGQgYmUgdG8gZml4IHRoZSByZWFsIGJ1ZyAoZm9yIAo+IGV4YW1wbGUg YnkgbWFraW5nIGhhcmR3YXJlIGRyaXZlciBpbml0IChvciBkZWluaXQpIHNlcXVlbmNlcyBtb3Jl IAo+IHJvYnVzdCksIG5vdCB0byBwYXBlciBpdCBvdmVyIGJ5IG9yZGVyaW5nIGFyb3VuZCBjcmFz aC10aW1lIHNlcXVlbmNlcyAKPiAuLi4KPiAKPiBJZiBpdCBjcmFzaGVkIGR1ZSB0byBzb21lIGhh cmR3YXJlIGZhaWx1cmUsIHRoZXJlJ3MgbGl0ZXJhbGx5IGFuIAo+IGluZmluaXRlIGFtb3VudCBv ZiBmYWlsdXJlIG1vZGVzIHRoYXQgbWF5IG9yIG1heSBub3QgYmUgaW1wYWN0ZWQgYnkgCj4ga2V4 ZWMgY3Jhc2gtdGltZSBoYW5kbGluZyBvcmRlcmluZy4gV2UgZG9uJ3Qgd2FudCB0byBwdXQgYSB6 aWxsaW9uIAo+IHN1Y2ggZmxhZ3MgaW50byB0aGUga2VybmVsIHByb3BlciBqdXN0IHRvIGFsbG93 IHRoZSBwZXJ0dXJiYXRpb24gb2YgCj4gdGhlIGtlcm5lbC4KPiAKPiBUaGFua3MsCj4gCj4gCUlu Z28KPiAKPiBkaWZmIC0tZ2l0IGEva2VybmVsL3BhbmljLmMgYi9rZXJuZWwvcGFuaWMuYwo+IGlu ZGV4IDgxMzZhZDc2ZTVmZC4uNzc0NjE0ZjcyY2JkIDEwMDY0NAo+IC0tLSBhL2tlcm5lbC9wYW5p Yy5jCj4gKysrIGIva2VybmVsL3BhbmljLmMKPiBAQCAtMTQyLDcgKzE0Miw4IEBAIHZvaWQgcGFu aWMoY29uc3QgY2hhciAqZm10LCAuLi4pCj4gIAkgKiBOb3RlOiBzaW5jZSBzb21lIHBhbmljX25v dGlmaWVycyBjYW4gbWFrZSBjcmFzaGVkIGtlcm5lbAo+ICAJICogbW9yZSB1bnN0YWJsZSwgaXQg Y2FuIGluY3JlYXNlIHJpc2tzIG9mIHRoZSBrZHVtcCBmYWlsdXJlIHRvby4KPiAgCSAqLwo+IC0J Y3Jhc2hfa2V4ZWMoTlVMTCk7Cj4gKwlpZiAoY3Jhc2hfa2V4ZWNfcG9zdF9ub3RpZmllcnMpCj4g KwkJY3Jhc2hfa2V4ZWMoTlVMTCk7Cj4gIAo+ICAJYnVzdF9zcGlubG9ja3MoMCk7Cj4gIAo+IC0t Cj4gVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2Ny aWJlIGxpbnV4LWtlcm5lbCIgaW4KPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21v QHZnZXIua2VybmVsLm9yZwo+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtl cm5lbC5vcmcvbWFqb3Jkb21vLWluZm8uaHRtbAo+IFBsZWFzZSByZWFkIHRoZSBGQVEgYXQgIGh0 dHA6Ly93d3cudHV4Lm9yZy9sa21sLwo+IAoKCi0tIApNYXNhbWkgSElSQU1BVFNVClNvZnR3YXJl IFBsYXRmb3JtIFJlc2VhcmNoIERlcHQuIExpbnV4IFRlY2hub2xvZ3kgUmVzZWFyY2ggQ2VudGVy CkhpdGFjaGksIEx0ZC4sIFlva29oYW1hIFJlc2VhcmNoIExhYm9yYXRvcnkKRS1tYWlsOiBtYXNh bWkuaGlyYW1hdHN1LnB0QGhpdGFjaGkuY29tCgoKCl9fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fCmtleGVjIG1haWxpbmcgbGlzdAprZXhlY0BsaXN0cy5pbmZy YWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8va2V4 ZWMK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752394AbbCXDaU (ORCPT ); Mon, 23 Mar 2015 23:30:20 -0400 Received: from mail7.hitachi.co.jp ([133.145.228.42]:35797 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752114AbbCXDaR (ORCPT ); Mon, 23 Mar 2015 23:30:17 -0400 Message-ID: <5510DA42.6040708@hitachi.com> Date: Tue, 24 Mar 2015 12:30:10 +0900 From: Masami Hiramatsu Organization: Hitachi, Ltd., Japan User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120614 Thunderbird/13.0.1 MIME-Version: 1.0 To: Ingo Molnar CC: Baoquan He , =?UTF-8?B?IkhhdGF5YW1hLCBEYWlzdWtlL+eVkeWxsQ==?= =?UTF-8?B?IOWkp+i8lCI=?= , ebiederm@xmission.com, Vivek Goyal , 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: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path References: <54F9D645.2050008@jp.fujitsu.com> <20150323034752.GD2068@dhcp-16-105.nay.redhat.com> <20150323071943.GA22765@gmail.com> In-Reply-To: <20150323071943.GA22765@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org (2015/03/23 16:19), 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. No, Daisuke's patch is not for that case. Since the kdump has a special hook in kernel oops, when both of panic_on_oops and crash_kernel are set, panic() is never called. Please see oops_end@arch/x86/kernel/dumpstack.c ---- void oops_end(unsigned long flags, struct pt_regs *regs, int signr) { if (regs && kexec_should_crash(current)) crash_kexec(regs); ---- Of course crash_kexec() never return except failing kexec unexpectedly. Thus, kexec_should_crash should returns 0 if crash_kexec_post_notifiers is set. (Semantically, it is a bit strange that panic_on_oops doesn't call panic(), but that is another topic.) However, your patch is also needed since the first crash_kexec() can fail in panic() when crash_kexec_post_notifiers is not set. In that case, kernel tries to call notifiers and call the 2nd crash_kexec() again. Actually the 2nd one is useless. So, here is my reviewed-by. Reviewed-by: Masami Hiramatsu I'll be reply the latter part in other mail. Thank you, > > 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. > > Thanks, > > Ingo > > 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); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com