Re: [PATCH v2 4/9] printk: Cleanup _braille_(un)register_console() wrappers

[John Ogness]

On 2026-04-23, Sashiko wrote:
>> diff --git a/drivers/accessibility/braille/braille_console.c b/drivers/accessibility/braille/braille_console.c
>> index 06b43b678d6e..7b324329882f 100644
>> --- a/drivers/accessibility/braille/braille_console.c
>> +++ b/drivers/accessibility/braille/braille_console.c
>> @@ -360,12 +360,12 @@ int braille_register_console(struct console *console, int index,
>>  		if (ret != 0)
>>  			return ret;
>>  	}
>> -	console->flags |= CON_ENABLED;
>> +	console->flags |= CON_ENABLED | CON_BRL;
>>  	console->index = index;
>>  	braille_co = console;
>>  	register_keyboard_notifier(&keyboard_notifier_block);
>>  	register_vt_notifier(&vt_notifier_block);
>> -	return 1;
>> +	return 0;
>>  }
>
> While reviewing this function, I noticed a pre-existing
> regression. The printk subsystem relies on console_lock() to serialize
> console setup against boot consoles that might be actively printing to
> the same hardware port.  For normal consoles, this synchronization is
> handled explicitly:
>
> kernel/printk/printk.c:console_call_setup() {
>     ...
>     console_lock();
>     err = newcon->setup(newcon, options);
>     console_unlock();
>     ...
> }
>
> However, it looks like braille_register_console() bypasses this
> protection and directly invokes console->setup() just above this block
> without acquiring console_lock().
>
> Could this unprotected setup call cause a data race or hardware state
> corruption if the underlying driver shares the hardware with an active
> boot console?

I do not know enough about braille consoles if they can also be early
consoles, but it makes sense to synchronize the ->setup()
callback. Something like this:

--- a/drivers/accessibility/braille/braille_console.c
+++ b/drivers/accessibility/braille/braille_console.c
@@ -356,7 +356,9 @@ int braille_register_console(struct console *console, int index,
 	if (braille_co)
 		return -ENODEV;
 	if (console->setup) {
+		console_lock();
 		ret = console->setup(console, console_options);
+		console_unlock();
 		if (ret != 0)
 			return ret;
 	}

>>  int braille_unregister_console(struct console *console)
>> @@ -375,5 +375,6 @@ int braille_unregister_console(struct console *console)
>>  	unregister_keyboard_notifier(&keyboard_notifier_block);
>>  	unregister_vt_notifier(&vt_notifier_block);
>>  	braille_co = NULL;
>> -	return 1;
>> +	console->flags &= ~(CON_ENABLED | CON_BRL);
>> +	return 0;
>>  }
>
> This also appears to be a pre-existing regression. The
> unregister_keyboard_notifier() and unregister_vt_notifier() functions
> remove callbacks from atomic notifier chains. Since atomic notifiers
> execute their callbacks under RCU read lock, unregistering them only
> unlinks the node without blocking for concurrent callbacks to finish.
>
> By immediately clearing the global braille_co pointer and returning,
> could a concurrently executing keyboard_notifier_call that already
> passed the braille_co check on another CPU cause a use-after-free or
> null pointer dereference?
>
> Does this function need a synchronize_rcu() before returning to ensure
> all callbacks have completed?

atomic_notifier_chain_unregister() already calls synchronize_rcu()
before returning, so no action is needed here.

For this patch as it is:

Reviewed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>

(The console lock/unlock should be a separate patch.)

John