Re: [PATCH 2/6] vt: keyboard: publish npadch_value with release semantics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vt: redundant memory barriers on npadch_value under spinlock
- [Low] vt: broken lockless pattern with inverted flag/payload release
--

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

vt: keyboard: publish npadch_value with release semantics
    
k_ascii() accumulates the numeric keypad code in npadch_value after it
has set npadch_active. k_shift() tests npadch_active and then emits
npadch_value when the modifier is released. Store the accumulated value
with smp_store_release() and read it with smp_load_acquire().

> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 089f3b048beee..6f3472cd4f24d 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -898,10 +898,13 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
>  
>  	/* kludge */
>  	if (up_flag && shift_state != old_state && npadch_active) {
> +		/* Pairs with the smp_store_release() in k_ascii(). */
> +		unsigned int npadch = smp_load_acquire(&npadch_value);
> +

[Severity: Medium]
Are these memory barriers redundant?

All invocations of k_shift() and k_ascii() occur under the kbd_event_lock.
They are only dispatched via kbd_keycode(), which strictly executes within a
scoped_guard(spinlock, &kbd_event_lock) in kbd_event().

Because all accesses to npadch_value and npadch_active are serialized by
this lock, concurrent execution is not possible here. The static analysis
tool appears to have missed this locking context.

>  		if (kbd->kbdmode == VC_UNICODE)
> -			to_utf8(vc, npadch_value);
> +			to_utf8(vc, npadch);
>  		else
> -			put_queue(vc, npadch_value & 0xff);
> +			put_queue(vc, npadch & 0xff);
>  		npadch_active = false;
>  	}
>  }
> @@ -939,7 +942,8 @@ static void k_ascii(struct vc_data *vc, unsigned char value, char up_flag)
>  		npadch_active = true;
>  	}
>  
> -	npadch_value = npadch_value * base + value;
> +	/* Pairs with the smp_load_acquire() in k_shift(). */
> +	smp_store_release(&npadch_value, npadch_value * base + value);

[Severity: Low]
Does this inverted lockless pattern fail to provide the intended guarantees?

Even if concurrent execution were possible, the release and acquire
semantics are applied to the data payload (npadch_value) rather than the
condition flag (npadch_active).

Here, the producer updates npadch_active with a plain store before executing
a release store on npadch_value. This allows the condition flag to become
visible prematurely.

In a correct lockless pattern, the producer updates the payload first and
then executes a release store on the condition flag. The consumer would then
execute an acquire load on the flag before reading the payload.


While reviewing this file, I noticed a pre-existing issue nearby in k_shift():

drivers/tty/vt/keyboard.c:k_shift() {
    ...
	/* Pairs with the smp_load_acquire() in vt_get_shift_state(). */
	smp_store_release(&shift_state, state);
    ...
}

[Severity: Low]
This is a pre-existing issue, but is the use of release/acquire barriers
invalid for this standalone read?

The shift_state variable is a standalone integer and does not act as a
condition flag to synchronize dependent memory accesses. Applying heavyweight
memory barriers here adds unnecessary complexity and overhead on
architectures where barriers are not free.

The reader vt_get_shift_state() merely returns the shift_state value to
userspace without taking kbd_event_lock:

drivers/tty/vt/keyboard.c:vt_get_shift_state() {
    ...
	return smp_load_acquire(&shift_state);
    ...
}

To safely perform a lockless read of a standalone variable and mitigate
compiler tearing, READ_ONCE() and WRITE_ONCE() would be the correct and
sufficient primitives.

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