From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) (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 AC6B448124D for ; Wed, 19 Aug 2026 15:52:43 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.46 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787154765; cv=none; b=GvFm+GLY2SIFh7ABynpw7jn1B4DPMUD21xx34u7HJkjgizxmNXEemG4LF5CWHMd1jcDkZABRP5b43Ay/w4HeroOeJSBZzbNXI5D+a5S5Txia79c87pkCYqntY4B+Nr0WrKqiLNreJNWNXjplghqgY0u+483Avdazwzqu1L6qovo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1787154765; c=relaxed/simple; bh=pXZ69cjGcn/+xMAppBDahOEtyghhTrsLjChdrGLvlMg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Dc5U08sLELiRjysp86EUuOwuX+aoEdX5mT22tAbair/Zkh1ddWazRaB42qBQfgkGAQxDvLOtejb3r9KKOyIRKFLBuRSVFE0v5EVHTkPs+I11WUflxbpdN70LS+1afgq70Z8Ret7qioeoxKAs/Hgmkddo0yN0ibyIJIMLOWHCbu4= 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=CuNh4dx1; arc=none smtp.client-ip=209.85.128.46 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="CuNh4dx1" Received: by mail-wm1-f46.google.com with SMTP id 5b1f17b1804b1-4994c49f588so380625e9.0 for ; Wed, 19 Aug 2026 08:52:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=google; t=1787154762; x=1787759562; 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=gnzgvOSB+Nizm4evVk6JtmNaOOhkBfwJPgxX0HZjRt4=; b=CuNh4dx1YMVaZ+FuvdxRkjZGoB0yNykbAXZ/KDA4+Y/lcYocZ7Mr49AXqUqRXyxEXV 4RN9gxnS4MOIbs9lD/tSJAwwv6aWzIz6FwI8R2kuTDZ7f8YB4y3qUtOQG7qtZN18RCyc HN7ZuO6mDIL0q898A6KSI3m9TvOASmVf5lNN6ca6IZRzz8OqJVN1RVGQ/wEBM6WBXn9K Dx8dHettNPoQ9dNBSH9viPzQGeEFVuvFhyFMV2L0LoNQ/A31w+gz1NDp7LNZ5NN7GLEt mcvX91aUykRTKbglh7vzxTfOkpBepobGvFQ6lHqqu7EaOy5+iDJyLv0Rk2whpNjEaC1H 1ixw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1787154762; x=1787759562; 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=gnzgvOSB+Nizm4evVk6JtmNaOOhkBfwJPgxX0HZjRt4=; b=RLlep/K9T8g0KNL34MgLqWdjmfNu/ikd+6eaAtjl8OGT0saUfAbrVDjDW3uMChOltW KyR9Yg0lpQjWIMUfjrqDWEdszne8aT+9Sv6NaSzBrTnkeTtR0an+DibTsNryN6/h2XrL dE+skPSJyBbnEV/xal8i1TSrCdpXa/X4JraP4JSfdV1ffeddrdwDo5oNAaPQKPIDDJID MukcTcoEo9KGuHkGMuJil6kq+sDJwk7pQsxYDuvCYINhF4/KkPEcrmU+0M4B3aU9fIxi u/J7inLlXmQwrD2thRikPno/jcZg3aG1efFryJWVqxVgnFOFxoBFHvG/SdVRmiMvb53f 2Hmw== X-Forwarded-Encrypted: i=1; AHgh+RqCRHU8xbgZvZLBjCZitieuCjwgZHJDIe304jIPR57Y+whn7f3wgd8tKKV2vCO5fChnSuVAsHPLS0bCW7I=@vger.kernel.org X-Gm-Message-State: AOJu0Ywk4HgqAvQ8jh4VfccyVCjlVV1K7IrRWLxZw+9DbWXAmXAdTdbJ C14eiWQuxqovmUFO3ru/mTbvCgeDIwC45SF1rRgQYaD2MzXLn6wAinDM8uHpekOb75arNzmscie MBG0C1SY= X-Gm-Gg: AR+sD104VLt8cWFY4wgwO7YhvZUOTjehwzp2s7qtA7hyfvmq/Sp6mqTWBCBOXIS4Fo6 t0YmZ7l5WsnABvdLeGj7cuSRcq80k3uFK5QLBHq2IZ4bUhhKT2wxTRFkWyQzGBXX1HuyL0uiXc6 rZgr8c98GejgKdiE5Lb3NWfZhOGnrvX/LKIKmG9wFT4CD8f5zl94khYDNOFl7n/lwUChMciAIUt XSHxbUz3VBDBctq7x45ZqcFKvC0JiSxtYfwMVRdgr/+GvtyOFTlsnJBbBPcCZJ1eYqtJVJGrJqQ A3bPsFgchmXVogNWKglOD+mWdCynwcyfWYz6NZvPdkCdRKgs0rUW+cwTvjGuf9cqp5MpVDQqfMX RFh8wMmjyD8poM3ij/WTU6SNLUDxZIp4G9HsbQXiCL18KqcyYffFo1i5HSA+cfl9pwYl9Yd6n6n CTmsoO/6nU1BUPXlDgzJOQTnmGAk2bXBY3kxVeGOQM+hml5DUi8FQ3h2FHxcWu1A== X-Received: by 2002:a05:600c:4309:b0:499:60e5:248a with SMTP id 5b1f17b1804b1-499b06ea247mr1333265e9.4.1787154761704; Wed, 19 Aug 2026 08:52:41 -0700 (PDT) Received: from pathway.suse.cz ([176.114.240.130]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-482b14415ecsm6217343f8f.7.2026.08.19.08.52.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 19 Aug 2026 08:52:41 -0700 (PDT) Date: Wed, 19 Aug 2026 17:52:39 +0200 From: Petr Mladek To: John Ogness Cc: Sergey Senozhatsky , Steven Rostedt , linux-kernel@vger.kernel.org Subject: Re: [PATCH printk v2] printk/nbcon: WARN on unsafe reentrance Message-ID: References: <20260731144000.592883-1-john.ogness@linutronix.de> 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: <20260731144000.592883-1-john.ogness@linutronix.de> On Fri 2026-07-31 16:45:46, John Ogness wrote: > Since the nbcon unsafe enter/exit functions are simply toggling a > state boolean, a buggy nbcon driver might enter an unsafe section > when the context is already in an unsafe section and it would go > unnoticed, even though doing so is a bug. Unsafe sections are not > reentrant! > > Add a WARN_ON_ONCE() to nbcon_enter_unsafe() if the context is > already in an unsafe section. > > Signed-off-by: John Ogness > Link: https://lore.kernel.org/lkml/87o6fwms02.fsf@jogness.linutronix.de > --- > Changes since v1: > > - Move the WARN to after the can_proceed() check. Otherwise > cur.unsafe could be referring to a different owning context. > > kernel/printk/nbcon.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c > index 4b03b019cd5ee..e88ddd742d5a0 100644 > --- a/kernel/printk/nbcon.c > +++ b/kernel/printk/nbcon.c > @@ -848,6 +848,9 @@ static bool __nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsaf > if (!nbcon_context_can_proceed(ctxt, &cur)) > return false; > > + /* Unsafe sections are not reentrant. */ > + WARN_ON_ONCE(unsafe && cur.unsafe); Sashiko AI has some good points: | Will this trigger a spurious warning during a panic if the console was | acquired via an unsafe hostile takeover? | | During a panic, if nbcon_context_try_acquire_hostile() acquires an | interrupted console, it preserves the unsafe state (cur.unsafe = true and | cur.unsafe_takeover = true). | | When nbcon_emit_next_record() then processes the next record, it calls | nbcon_context_enter_unsafe(), which invokes __nbcon_context_update_unsafe() | with unsafe = true. Since cur.unsafe remains true from the hostile takeover, | this condition evaluates to true, potentially polluting the panic log with a | spurious stack trace. IMHO, it has a point. A solution might be to print the warning only when there was no unsafe_takeover, e.g. /* * Unsafe sections are not reentrant except when an unsafe_takeover * already happened. */ if (!cur.unsafe_takeover) WARN_ON_ONCE(unsafe && cur.unsafe); | Does this also trigger a spurious warning when KDB outputs to an nbcon | console? | | Looking at kdb_msg_write(), it calls nbcon_kdb_try_acquire() which | explicitly puts the console into the unsafe state: | | kernel/printk/nbcon.c:nbcon_kdb_try_acquire() { | ... | if (!nbcon_context_enter_unsafe(ctxt)) | return false; | ... | } | | After doing so, KDB invokes the driver's write_atomic callback directly: | | kernel/debug/kdb/kdb_io.c:kdb_msg_write() { | ... | c->write_atomic(c, &wctxt); | ... | } | | According to the API, driver write_atomic() implementations must call | nbcon_enter_unsafe() at the beginning of their execution. Since KDB's | wrapper already forced the unsafe state, the driver's subsequent call | evaluates unsafe == true and cur.unsafe == true, firing this warning | on every KDB output. This concern looks valid as well. IMHO, the right fix is that nbcon_kdb_try_acquire() should not call nbcon_context_enter_unsafe(). IMHO, we called nbcon_context_enter_unsafe() in nbcon_kdb_try_acquire() because of nbcon_write_context_set_buf(). We did not want to call nbcon_context_enter_unsafe() in kdb_msg_write() because it would require to access the private wctxt.ctxt. But nbcon_write_context_set_buf() can be called even when the safe takeover is allowed. It is done this way even in nbcon_emit_next_record(). So, I think that we could do something like: diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c index c399f11740ef..51d4b573b44d 100644 --- a/kernel/debug/kdb/kdb_io.c +++ b/kernel/debug/kdb/kdb_io.c @@ -604,8 +604,8 @@ static void kdb_msg_write(const char *msg, int msg_len) continue; nbcon_write_context_set_buf(&wctxt, (char *)msg, msg_len); - c->write_atomic(c, &wctxt); + nbcon_kdb_release(&wctxt); } else { /* diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c index e88ddd742d5a..a82a5cebb469 100644 --- a/kernel/printk/nbcon.c +++ b/kernel/printk/nbcon.c @@ -1944,8 +1944,7 @@ void nbcon_device_release(struct console *con) EXPORT_SYMBOL_GPL(nbcon_device_release); /** - * nbcon_kdb_try_acquire - Try to acquire nbcon console and enter unsafe - * section + * nbcon_kdb_try_acquire - Try to acquire nbcon console * @con: The nbcon console to acquire * @wctxt: The nbcon write context to be used on success * @@ -1959,8 +1958,7 @@ EXPORT_SYMBOL_GPL(nbcon_device_release); * storing them into the ring buffer. It has to acquire the console * ownership so that it could call con->write_atomic() callback a safe way. * - * This function acquires the nbcon console using priority NBCON_PRIO_EMERGENCY - * and marks it unsafe for handover/takeover. + * This function acquires the nbcon console using priority NBCON_PRIO_EMERGENCY. */ bool nbcon_kdb_try_acquire(struct console *con, struct nbcon_write_context *wctxt) @@ -1974,14 +1972,11 @@ bool nbcon_kdb_try_acquire(struct console *con, if (!nbcon_context_try_acquire(ctxt, false)) return false; - if (!nbcon_context_enter_unsafe(ctxt)) - return false; - return true; } /** - * nbcon_kdb_release - Exit unsafe section and release the nbcon console + * nbcon_kdb_release - Release the nbcon console * * @wctxt: The nbcon write context initialized by a successful * nbcon_kdb_try_acquire() @@ -1990,9 +1985,6 @@ void nbcon_kdb_release(struct nbcon_write_context *wctxt) { struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt); - if (!nbcon_context_exit_unsafe(ctxt)) - return; - nbcon_context_release(ctxt); /* > new.atom = cur.atom; > new.unsafe = unsafe; > } while (!nbcon_state_try_cmpxchg(con, &cur, &new)); Finally, we might want to do the check symmetric. I mean that also nbcon_context_exit_unsafe() should not be called twice. I mean something like: /* * Unsafe sections are not reentrant except when an unsafe_takeover * already happened. * * This check is valid only when "cur" contains the state when this * context still owned the console, aka nbcon_context_can_proceed() * succeeded. */ if (!cur.unsafe_takeover) WARN_ON_ONCE(unsafe == cur.unsafe); But honestly, I haven't checked all callers. It is possible that some code calls exit_unsafe() twice, like the kdb_msg_write() called enter_unsafe() twice. Best Regards, Petr