Re: [PATCH 3/6] vt: keyboard: publish dead_key_next with release semantics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] tty: vt: bogus memory barriers for dead_key_next under spinlock
- [Medium] tty: vt: flawed acquire semantics in k_unicode lockless logic
--

commit 8306184a36f96d0dec76dd629c6b568b695fecc2
Author: Jaidev Shastri <jaidevshastri@vt.edu>

vt: keyboard: publish dead_key_next with release semantics

fn_compose() arms the dead-key state in dead_key_next and k_unicode()
consumes it together with diacr.

> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 6f3472cd4f24d..d7db5e226d7dc 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -618,7 +618,8 @@ static void fn_boot_it(struct vc_data *vc)
>  
>  static void fn_compose(struct vc_data *vc)
>  {
> -	dead_key_next = true;
> +	/* Pairs with the smp_load_acquire() in k_unicode(). */
> +	smp_store_release(&dead_key_next, true);
>  }

[Severity: Medium]
Are these memory barriers redundant here?

The dead_key_next flag is only accessed within fn_compose() and k_unicode().
Both of these functions are exclusively invoked as part of the keyboard event
handling path originating from kbd_keycode().

kbd_keycode() is called by kbd_event(), which wraps the entire execution block
with scoped_guard(spinlock, &kbd_event_lock). Since all accesses to
dead_key_next are strictly serialized by this spinlock, no concurrent access
across cores can occur, making the lockless memory barriers useless.

[ ... ]

> @@ -672,7 +673,8 @@ static void k_unicode(struct vc_data *vc, unsigned int value, char up_flag)
>  	if (diacr)
>  		value = handle_diacr(vc, value);
>  
> -	if (dead_key_next) {
> +	/* Pairs with the smp_store_release() in fn_compose(). */
> +	if (smp_load_acquire(&dead_key_next)) {
>  		dead_key_next = false;
>  		diacr = value;
>  		return;

[Severity: Medium]
If this were truly a lockless environment, does reading diacr before the
acquire barrier defeat the intended synchronization?

The payload diacr is read prior to the smp_load_acquire(&dead_key_next)
barrier, meaning it is fetched outside the intended synchronization domain,
which could allow stale reads.

Additionally, dead_key_next is reset to false using a plain store inside the
block. In a concurrent setting, consuming a shared flag requires an atomic
operation (like xchg()) to avoid a read-modify-write data race where multiple
threads could read true and simultaneously clobber diacr.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-mb-keyboard-v1-0-d170228b80c0@vt.edu?part=3