Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Daniel Vetter <daniel.vetter@ffwll.ch>
To:     LKML <linux-kernel@vger.kernel.org>
Cc:     DRI Development <dri-devel@lists.freedesktop.org>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        Daniel Vetter <daniel.vetter@intel.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Xuezhi Zhang <zhangxuezhi1@coolpad.com>,
        Yangxi Xiang <xyangxi5@gmail.com>,
        Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
        nick black <dankamongmen@gmail.com>,
        Petr Mladek <pmladek@suse.com>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        John Ogness <john.ogness@linutronix.de>,
        Sam Ravnborg <sam@ravnborg.org>
Subject: [PATCH] tty/vt: Add console_lock check to vt_console_print()
Date:   Tue, 30 Aug 2022 16:49:45 +0200
Message-Id: <20220830144945.430528-1-daniel.vetter@ffwll.ch>
In-Reply-To: <20220830132803.403744-1-daniel.vetter@ffwll.ch>
References: <20220830132803.403744-1-daniel.vetter@ffwll.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

I'm scratching my head why we have this printing_lock. Digging through
historical git trees shows that:
- Added in 1.1.73, and I found absolutely no reason why.
- Converted to atomic bitops in 2.1.125pre2, I guess as part of SMP
  enabling/bugfixes.
- Converted to a proper spinlock in b0940003f25d ("vt: bitlock fix")
  because the hand-rolled atomic version lacked necessary memory
  barriers.

Digging around in lore for that time period did also not shed further
light.

The only reason I think this might still be relevant today is that (to
my understanding at least, ymmv) during an oops we might be printing
without console_lock held. See console_flush_on_panic() and the
comments in there - we flush out the console buffers irrespective of
whether we managed to acquire the right locks.

The strange thing is that this reason is fairly recent, because the
console flushing was historically done without oops_in_progress set.
This only changed in c7c3f05e341a ("panic: avoid deadlocks in
re-entrant console drivers"), which removed the call to
bust_spinlocks(0) (which decrements oops_in_progress again) before
flushing out the console (which back then was open coded as a
console_trylock/unlock pair).

Note that this entire mess should be properly fixed in the
printk/console layer, and not inflicted on each implementation.

For now just document what's going on and check that in all other
cases callers obey the locking rules.

v2: WARN_CONSOLE_UNLOCKED already checks for oops_in_progress
(something else that should be fixed I guess), hence remove the
open-coded check I've had.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jirislaby@kernel.org>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Xuezhi Zhang <zhangxuezhi1@coolpad.com>
Cc: Yangxi Xiang <xyangxi5@gmail.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: nick black <dankamongmen@gmail.com>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Sam Ravnborg <sam@ravnborg.org>
--
Note that this applies on top of my earlier vt patch:

https://lore.kernel.org/lkml/20220826202419.198535-1-daniel.vetter@ffwll.ch/

Expect more, I'm digging around in here a bit ...
-Daniel
---
 drivers/tty/vt/vt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 4d29e4a17db7..a6be32798fad 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3083,7 +3083,9 @@ static void vt_console_print(struct console *co, const char *b, unsigned count)
 	ushort start_x, cnt;
 	int kmsg_console;
 
-	/* console busy or not yet initialized */
+	WARN_CONSOLE_UNLOCKED();
+
+	/* this protects against concurrent oops only */
 	if (!spin_trylock(&printing_lock))
 		return;
 
-- 
2.37.2