Re: [PATCH 2/2] braille: nbcon: Use nbcon atomic console callbacks

[Petr Mladek <pmladek@suse.com>]

On Tue 2026-09-22 07:37:27, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] braille: operator precedence bug disables braille atomic write
> - [High] braille: infinite spinlock in braille_write() during same-CPU unsafe takeover
> --
> 
> commit a7f38c0383481bd6ad9c244405d8d201d604569b
> Author: Petr Mladek <pmladek@suse.com>
> 
> braille: nbcon: Use nbcon atomic console callbacks
> 
> This commit adapts the braille console driver to support the new NBCON API.
> It introduces logic to acquire the nbcon console ownership before printing
> and uses the atomic write callback to output the buffer. The serial8250
> driver is also adjusted to skip prepending newlines for braille consoles.
> 
> > diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
> > --- a/drivers/accessibility/braille/braille_console.c
> > +++ b/drivers/accessibility/braille/braille_console.c
> > @@ -62,14 +62,32 @@ static void braille_write(u16 *buf)
> 
> [ ... ]
> 
> > +	if (braille_co->flags & CON_NBCON) {
> > +		/*
> > +		 * Braille console might be called from unknown context via
> > +		 * vt_console_print() from console_unlock() from printk().
> > +		 * Use the atomic callback and synchronize it just using
> > +		 * the console context. Disable interrupts to prevent a nested
> > +		 * call into the driver code which might cause a deadlock when
> > +		 * trying to acquire the console ownership, see
> > +		 * __nbcon_atomic_flush_pending_con().
> > +		 */
> > +		local_irq_save(flags);
> > +		while (!nbcon_braille_try_acquire(braille_co, &wctxt))
> > +			cpu_relax();
> 
> [Severity: High]
> Could this loop become an infinite spinlock if triggered from a nested
> context on the same CPU?
> 
> If an interrupt preempts the underlying nbcon console driver (such as 8250)
> while it is in an unsafe region, and that interrupt triggers a printk(),
> the legacy VT console might be flushed via console_unlock(), eventually
> calling braille_write().
> 
> Since local interrupts are disabled here, the preempted nbcon context on
> this same CPU can never resume to release the console. Because
> nbcon_context_try_acquire() rejects handovers to the same CPU when the
> console is in an unsafe state, won't this loop spin indefinitely?

For me, it is not easy to be sure but it looks to me that this code
is called deep in the generic vt code when handling vt_notifier_list
and keyboard_notifier_list. I guess that they already have to
synchronized against each other. At least the vt_notifier_list
operations seem to be synchronized using the console lock.

Anyway, the disabled interrupts should prevent nesting except
by NMI. But I believe that the VT code should not be called
in NMI because it uses locks. The only exception might be
panic().

An improvement might be to use NBCON_PRIO_PANIC in panic.
Something like (on top of 1st patch):

--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -2044,7 +2044,14 @@ bool nbcon_braille_try_acquire(struct console *con,
 
 	memset(ctxt, 0, sizeof(*ctxt));
 	ctxt->console = con;
-	ctxt->prio    = NBCON_PRIO_EMERGENCY;
+	ctxt->prio = nbcon_get_default_prio();
+
+	/*
+	 * The Braille console might be used in an interrupt context but
+	 * NBCON_PRIO_EMERGENCY is associated with task context.
+	 */
+	if (ctxt->prio < NBCON_PRIO_EMERGENCY)
+		ctxt->prio = NBCON_PRIO_EMERGENCY;
 
 	return nbcon_context_try_acquire(ctxt, false);
 }

> [ ... ]
> 
> > -	braille_co->write(braille_co, data, c - data);
> > +	if (braille_co->flags & CON_NBCON) {
> > +		if (braille_co->write_atomic &&
> > +		    !braille_co->flags & CON_NBCON_ATOMIC_UNSAFE) {
> 
> [Severity: High]
> Does this operator precedence evaluate as intended?
> 
> Because the logical not (!) operator has higher precedence than the bitwise
> and (&) operator, this expression parses as:
> 
> (!braille_co->flags) & CON_NBCON_ATOMIC_UNSAFE
> 
> Since braille_co->flags is known to be non-zero (as CON_NBCON was just
> checked above), evaluating the logical not against the flags yields 0. This
> makes the condition permanently false, which bypasses the braille atomic
> write callback unconditionally.
> 
> Should this be grouped with parentheses to evaluate the bitwise and first?

Grr, sure, it should be:

		    !(braille_co->flags & CON_NBCON_ATOMIC_UNSAFE)) {

I'll fix this in v2.

I am going to wait with v2 a bit just in case anyone would like to
comment on v1...

Best Regards,
Petr