From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com [209.85.221.43]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8DC8043B6FA for ; Mon, 13 Jul 2026 14:04:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.43 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783951452; cv=none; b=jnECRuCGCON2CF1mjtGdP3RX3YPY5C38C3W60Ya0b8qtcdmqbQXiM3BKYN7s9NiFiveqYFW2wSZBZWYXH2rc3npfe8WVXXNl0GiMDQJHBOtnUD1zY5/gFsKebmpwQ+oP90KNs+dTbm9IsjA6XOauU5/qmbkaJ9OAm2BsVwhsskw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783951452; c=relaxed/simple; bh=0aLhvEKD6P9we25KkF0nlFCW6qROWet2D9s+XL2NPaY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=OWmUuzCcV5wOqk3gM+TcrCW92Y41lfff3LBbRLtgQVU/oPM6Fdg8HPuunGxw/OOAHUgc3eV+fNGGp1NcXKrMFm43EMLBNmOK0ik98l8ezxp7fh9LH7VbZJDeMBPr9bOgwoEm+b26ZeWzdCKAzWOTZN1he9x8hZK++DyKQaOdEhI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com; spf=pass smtp.mailfrom=suse.com; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b=K1+qm+5k; arc=none smtp.client-ip=209.85.221.43 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=suse.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=suse.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=suse.com header.i=@suse.com header.b="K1+qm+5k" Received: by mail-wr1-f43.google.com with SMTP id ffacd0b85a97d-472326ca506so2606389f8f.2 for ; Mon, 13 Jul 2026 07:04:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1783951449; x=1784556249; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=9uA7uIF5Z8lD4HP32mfcctxaUcpnIm/yBzKVXH0XX2k=; b=K1+qm+5klOkDTuLUSdRIEd1WYgyfAFmjw1al7YscXflXMtmOYHjsLi8z4xJlLP+9o+ WZHGAftcK2zZI1UFiJEyaLUTQ/fGNQ2XxWr0trXSeZJwb6eYZ48BFF7g9N9A4Y40NlfS 8bK/efkP8tjZQ9SvfaqN+1lI0VyCKjZ4UK2AS1s0cVVZwa16n2cbsFgIt4OEibTnrYaS yxlG/D3+80axWlSmf8Nig6xt+NJxZgheyVSTKrB1W/GhvgPRcsPt84ABil7nI3yCvaD3 KDn2TQ1zbq+OIpdZ1Z2ZOlN1A3YgH2Ie93pkZcNQMmJq9oxWS8M2woh5pcGY18TQMd7+ hxBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1783951449; x=1784556249; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=9uA7uIF5Z8lD4HP32mfcctxaUcpnIm/yBzKVXH0XX2k=; b=eNwpugxq14Oe6KcFA/NtkC98EXHY1HGvN9uLYDWpr3QxyItmEm9W8eh9g24jAf0V/f 3Od9L83XjqrsH4RzvUxw7+tpqWh2SbdG4ci2biAORwvahAwf6d0Y1NlnSAgzJLYtWNKo OxwOysMq1ALbhJi9+0gw7prDlrKM75/igoSGTnPgiiwyy6pa85q4pBCLfzKfVHO97tae o17kr8jZMvKz/nClefwFTqGSl/Z8/S1X9dAF9/tYsKVss0sZPcB+bbwl9anc5Hd4VV1t IslRWN4WoQrcV1TvSsBGSLbvimHvfRbWQrGt4EG+FTuo7pVG6rYWnSWTPepaMg8ihvO1 FkVg== X-Forwarded-Encrypted: i=1; AHgh+RqBE6u3k7PlH72wl47nj4aAYt7QcZwIMwQXFE8Fglktb6WOqaVopL9udcwAateFmPIeTmyGd5T+8P0IuZM=@vger.kernel.org X-Gm-Message-State: AOJu0Yxd35kPmwOo3/BbKctK2A5Utj/cLrS2hUlKVeh64TgKZ2Ci6XJU En6bLpSn2afcDj4aKOHi9d826w1rwIe4nrL26D7CbICfNtRsU+p++sXADZh+vC4R+Ss= X-Gm-Gg: AfdE7cmrCsZMD4BjQzYc8oulGnMVTGbc0RcPQZuHq8HHHR7AEyDWvFzKqilYIZJzQx4 7ZnOgg0VqneWbVfWolhOMmDhVxy24giKH+k++FJFxAxkA5/SxgkzHFrqbREgFMBDgRczHl0T3yg leRKVKaaWT267breUeGW+nL7h8EZIhz7XTs2EV1EQU2B5HTEvYAf/RH5U6d8ocWAjnM/wo/flBo E92/FN8oqA9ldjaaYyYaMgqAQjQbqyyTiS9IZvce1k34ssZ88kL33N3JiaWa7YQiI+RQLfjttWu /gRIVNlAcygJKXCVC/YFaJvABrMHO8nfHqeCQ8Ohe9Lzd0WMeFLnnLLhcNcxN/uO8HiySfmt06F UDh4ayGoDVz57VLp4IUPrI9v+royo628kcT6Lv2/8yJeBvlBPP8X6NGaFtS5TjZsa14UuDfHKsE odST9/wbF/qAL6Cmw= X-Received: by 2002:a05:6000:2611:b0:46f:558:a42c with SMTP id ffacd0b85a97d-47f2dcb2974mr9917271f8f.4.1783951448741; Mon, 13 Jul 2026 07:04:08 -0700 (PDT) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47a9de1d91bsm90780687f8f.4.2026.07.13.07.04.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jul 2026 07:04:07 -0700 (PDT) Date: Mon, 13 Jul 2026 16:04:05 +0200 From: Petr Mladek To: Bradley Morgan Cc: akpm@linux-foundation.org, feng.tang@linux.alibaba.com, tglx@kernel.org, peterz@infradead.org, rostedt@goodmis.org, linux-kernel@vger.kernel.org, Sashiko Subject: Re: [PATCH v2] panic: allow force_cpu redirect from an NMI Message-ID: References: <20260708164312.19044-1-include@grrlz.net> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260708164312.19044-1-include@grrlz.net> On Wed 2026-07-08 16:43:12, Bradley Morgan wrote: > nmi_panic() calls panic_try_start() before panic(), so it claims > panic_cpu first. When the panic then reaches panic_try_force_cpu(), > panic_in_progress() sees panic_cpu set and returns false, so the > redirect to the requested CPU never happens. The crash kernel runs > on the CPU that took the NMI instead. > > smp_call_function_single_async() is safe from NMI context, Why do you think so, please? I was not sure and asked about this in v1, see https://lore.kernel.org/all/ak5GGf7ypVthkZk_@pathway.suse.cz/ I haven't seen any answer or explanation. The comment above smp_call_function_single_async() says that it should be safe in IRQ context. It does not talk about NMI. You might be right. But I would expect an explanation instead of a simple claim. > the redirect first. nmi_panic() now calls panic_try_force_cpu() > before panic_try_start(), and only claims panic_cpu when no redirect > is done. The requested CPU then claims panic_cpu itself. > > Also use panic_on_other_cpu() in place of the open coded check and > return true to stop directly. This is weird. This patch replaces panic_in_progress() with panic_on_other_cpu() and the commit message should explain why. The patch also shuffles the code formatting the panic message: + It should be described in the commit message. + It actually should be done in a separate patch. Because mixing too many changes into a single patch complicates the review and bisection of eventual regressions. + Most importantly, it can't be done because it is racy, see below. > diff --git a/kernel/panic.c b/kernel/panic.c > index ebdc46af6aa9..b039bb6f1d19 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -396,9 +393,9 @@ static bool panic_try_force_cpu(const char *fmt, va_list args) > return false; > } > > - /* Another panic already in progress */ > - if (panic_in_progress()) > - return false; > + /* Stop this CPU when the panic is already proceeding elsewhere. */ > + if (panic_on_other_cpu()) > + return true; How should we handle the case when the panic is on this CPU? Could this happen? We should not try to redirect panic() when "panic_cpu" is already assigned. We should always return when panic_in_progress(). But we should return either true or false depending whether the panic is on another or this CPU. > /* > * Only one CPU can do the redirection. Others should go > @@ -412,12 +409,7 @@ static bool panic_try_force_cpu(const char *fmt, va_list args) > * fall back to static message for early boot panics or allocation failure. > */ > if (panic_force_buf) { > - va_list ap; > - > - /* Do not consume args, the caller reuses it if we fail */ > - va_copy(ap, args); > - vsnprintf(panic_force_buf, PANIC_MSG_BUFSZ, fmt, ap); > - va_end(ap); > + strscpy(panic_force_buf, msg, PANIC_MSG_BUFSZ); This removes a code added by another patch which is still in Andrew's staging. A better solution would have been to ask Andrew to replace the older patch with this one. But we actually want to keep it to avoid the race, see below. > msg = panic_force_buf; > } else { > msg = "Redirected panic (buffer unavailable)"; > @@ -515,7 +506,9 @@ EXPORT_SYMBOL(panic_on_other_cpu); > */ > void nmi_panic(struct pt_regs *regs, const char *msg) > { > - if (panic_try_start()) > + if (panic_try_force_cpu(msg)) > + nmi_panic_self_stop(regs); > + else if (panic_try_start()) > panic("%s", msg); > else if (panic_on_other_cpu()) > nmi_panic_self_stop(regs); This code is kind of a puzzle. It is partly because panic_try*() does not explain well the meaning. And partly because the return values from both panic_try*() functions have a different meaning. Also the repeated nmi_panic_self_stop() looks a bit ugly. I think that we could do better. And we could use the fact that panic() is a no return function. I would suggest something like: /* Try to redirect panic() to a requested CPU when set. */ if (panic_try_force_cpu(msg)) goto self_stop; /* Try to acquire rights to proceed with (noreturn) panic(). */ if (panic_try_start()) panic("%s", msg); if (panic_on_other_cpu()) goto self_stop; /* * This should never happen. This CPU either acquired "panic_cpu" * or it has already been taken in which case panic_on_other_cpu() * should return true. */ return; self_stop: nmi_panic_self_stop(regs); IMHO, we actually could do: /* Try to redirect panic() to a requested CPU when set. */ if (panic_try_force_cpu(msg)) goto self_stop; /* Try to acquire rights to proceed with (noreturn) panic(). */ if (panic_try_start()) panic("%s", msg); /* * panic_try_start() might fail only when the panic() is * already in progress on another CPU in which case * this CPU should stop. */ self_stop: nmi_panic_self_stop(regs); It would be better to split this into two patches: + 1st patch would remove the original if else. + 2nd patch would add the panic_try_force_cpu(msg) + goto. > @@ -609,8 +602,13 @@ void vpanic(const char *fmt, va_list args) > local_irq_disable(); > preempt_disable_notrace(); > > + /* Format the message once; reused for the log and any redirect IPI. */ > + len = vscnprintf(buf, sizeof(buf), fmt, args); > + if (len && buf[len - 1] == '\n') > + buf[len - 1] = '\0'; This is not safe. "buf" is defined as a static variable. It is _not_ on stack but it is a global variable. It can be used only by the CPU which wins cmpxchg to "panic_cpu". IMHO, we need to keep it as is. > + > /* Redirect panic to target CPU if configured via panic_force_cpu=. */ > - if (panic_try_force_cpu(fmt, args)) { > + if (panic_try_force_cpu(buf)) { > /* > * Mark ourselves offline so panic_other_cpus_shutdown() won't wait > * for us on architectures that check num_online_cpus(). More info: This patch has been hard to review: 1. It did not apply because it depended on two other patches. Only one of them was in Andrews' unstable tree. => It is better to send patchsets than single patches which depend on each other. => Also it is better to wait until the discussion settles so that you know which patches were taken and which not. [*] 2. The patch did many things together. => It is always better to split changes into more patches. 3. The commit message did not describe all the changes. => Double check everything before sending patches to the mailing list. [*] The problem was partly also because Andrew did take the patches into the unstable tree so quickly. Best Regards, Petr