[PATCH 0/6] vt: keyboard: make the kbd_event_lock ordering explicit

Six variables in keyboard.c are written under kbd_event_lock and read either
without it or from a different handler, with plain accesses throughout:
shift_state, shift_down[], npadch_value, dead_key_next, accent_table_size,
and the in-place rebuild in do_compute_shiftstate().

vt_get_shift_state() is the one reader that deliberately takes no lock, so
the relation between shift_down[] and the shift_state summary word is
currently unspecified. The remaining pairs are lock-ordered today and those
patches only state the order in the code.

One patch fixes a real mistake rather than documenting an existing order:
vt_do_kdskbdiacr() sets accent_table_size before the loop that converts the
entries, so the size briefly covers entries that have not been written yet.
That store now follows the loop, in both ioctl paths.

Found with MBCheck, a static herd7-based memory consistency checker.
Compile-tested on arm64 with W=1, no new warnings.

---
Jaidev Shastri (6):
      vt: keyboard: publish shift_state with release semantics
      vt: keyboard: publish npadch_value with release semantics
      vt: keyboard: publish dead_key_next with release semantics
      vt: keyboard: publish accent_table_size with release semantics
      vt: keyboard: recompute the shift state into locals before publishing it
      vt: keyboard: publish the shift_down[] counters with release semantics

 drivers/tty/vt/keyboard.c | 72 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 50 insertions(+), 22 deletions(-)
---
base-commit: 93f51579e7df248780214094418f205253383cc5
change-id: 20260921-mb-keyboard-7f493253537e

Best regards,
--  
Jaidev Shastri [off-list ref]

From: Jaidev Shastri <redacted>

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().

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/tty/vt/keyboard.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 089f3b048..6f3472cd4 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);
+
 		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);
 }
 
 static void k_lock(struct vc_data *vc, unsigned char value, char up_flag)

-- 
2.43.0

From: Jaidev Shastri <redacted>

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

Store the flag with smp_store_release() and read it with
smp_load_acquire().

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/tty/vt/keyboard.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 6f3472cd4..d7db5e226 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);
 }
 
 static void fn_spawn_con(struct vc_data *vc)

@@ -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;

-- 
2.43.0

From: Jaidev Shastri <redacted>

k_shift() updates shift_down[] and then shift_state under
kbd_event_lock. vt_get_shift_state() reads shift_state without the lock
for TIOCL_GETSHIFTSTATE, so the plain accesses leave the relation
between the counters and the summary word unspecified.

Store shift_state with smp_store_release() and read it with
smp_load_acquire().

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/tty/vt/keyboard.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c41d850b2..089f3b048 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c

@@ -865,6 +865,7 @@ static void k_pad(struct vc_data *vc, unsigned char value, char up_flag)
 static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 {
 	int old_state = shift_state;
+	int state;
 
 	if (rep)
 		return;

@@ -889,9 +890,11 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 		shift_down[value]++;
 
 	if (shift_down[value])
-		shift_state |= BIT(value);
+		state = shift_state | BIT(value);
 	else
-		shift_state &= ~BIT(value);
+		state = shift_state & ~BIT(value);
+	/* Pairs with the smp_load_acquire() in vt_get_shift_state(). */
+	smp_store_release(&shift_state, state);
 
 	/* kludge */
 	if (up_flag && shift_state != old_state && npadch_active) {

@@ -2212,8 +2215,11 @@ void vt_reset_unicode(unsigned int console)
  */
 int vt_get_shift_state(void)
 {
-        /* Don't lock as this is a transient report */
-        return shift_state;
+	/*
+	 * Don't lock as this is a transient report. Pairs with the
+	 * smp_store_release() in k_shift().
+	 */
+	return smp_load_acquire(&shift_state);
 }
 
 /**

-- 
2.43.0

From: Jaidev Shastri <redacted>

vt_do_kdskbdiacr() and vt_do_kdskbdiacruc() rewrite accent_table[] and
accent_table_size. handle_diacr() reads the size and then walks the
table.

vt_do_kdskbdiacr() sets the size before it converts the entries, so the
size covers entries that have not been written yet. Set it after the
loop in both paths and publish it with smp_store_release(); read it once
with smp_load_acquire() before the walk.

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/tty/vt/keyboard.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index d7db5e226..c2fd92e2b 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c

@@ -447,7 +447,7 @@ void vt_set_leds_compute_shiftstate(void)
 static unsigned int handle_diacr(struct vc_data *vc, unsigned int ch)
 {
 	unsigned int d = diacr;
-	unsigned int i;
+	unsigned int i, n;
 
 	diacr = 0;
 

@@ -455,7 +455,9 @@ static unsigned int handle_diacr(struct vc_data *vc, unsigned int ch)
 		if ((ch & ~0xff) == BRL_UC_ROW)
 			return d | ch;
 	} else {
-		for (i = 0; i < accent_table_size; i++)
+		/* Pairs with the smp_store_release() in vt_do_diacrit(). */
+		n = smp_load_acquire(&accent_table_size);
+		for (i = 0; i < n; i++)
 			if (accent_table[i].diacr == d && accent_table[i].base == ch)
 				return accent_table[i].result;
 	}

@@ -1810,7 +1812,6 @@ static int vt_do_kdskbdiacr(void __user *udp, int perm)
 	}
 
 	guard(spinlock_irqsave)(&kbd_event_lock);
-	accent_table_size = ct;
 	for (i = 0; i < ct; i++) {
 		accent_table[i].diacr =
 				conv_8bit_to_uni(dia[i].diacr);

@@ -1819,6 +1820,8 @@ static int vt_do_kdskbdiacr(void __user *udp, int perm)
 		accent_table[i].result =
 				conv_8bit_to_uni(dia[i].result);
 	}
+	/* Pairs with the smp_load_acquire() in handle_diacr(). */
+	smp_store_release(&accent_table_size, ct);
 
 	return 0;
 }

@@ -1848,7 +1851,8 @@ static int vt_do_kdskbdiacruc(void __user *udp, int perm)
 	if (ct)
 		memcpy(accent_table, buf,
 				ct * sizeof(struct kbdiacruc));
-	accent_table_size = ct;
+	/* Pairs with the smp_load_acquire() in handle_diacr(). */
+	smp_store_release(&accent_table_size, ct);
 	return 0;
 }
 

-- 
2.43.0

From: Jaidev Shastri <redacted>

do_compute_shiftstate() clears shift_state and shift_down[] and rebuilds
them in place while it iterates key_down[]. vt_get_shift_state() reads
shift_state without kbd_event_lock and can observe the cleared or
partially rebuilt value.

Compute the new state into locals, copy shift_down[] first and publish
shift_state last with smp_store_release().

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/tty/vt/keyboard.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c2fd92e2b..87970415a 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c

@@ -403,10 +403,9 @@ static void set_leds(void)
 
 static void do_compute_shiftstate(void)
 {
+	unsigned char down[NR_SHIFT] = { 0 };
 	unsigned int k, sym, val;
-
-	shift_state = 0;
-	memset(shift_down, 0, sizeof(shift_down));
+	int state = 0;
 
 	for_each_set_bit(k, key_down, min(NR_KEYS, KEY_CNT)) {
 		sym = U(key_maps[0][k]);

@@ -417,9 +416,17 @@ static void do_compute_shiftstate(void)
 		if (val == KVAL(K_CAPSSHIFT))
 			val = KVAL(K_SHIFT);
 
-		shift_down[val]++;
-		shift_state |= BIT(val);
+		down[val]++;
+		state |= BIT(val);
 	}
+
+	memcpy(shift_down, down, sizeof(shift_down));
+	/*
+	 * Publish the recomputed state in one step. vt_get_shift_state()
+	 * reads shift_state without kbd_event_lock; pairs with its
+	 * smp_load_acquire().
+	 */
+	smp_store_release(&shift_state, state);
 }
 
 /* We still have to export this method to vt.c */

-- 
2.43.0

From: Jaidev Shastri <redacted>

k_shift() maintains the per-modifier depress counters in shift_down[]
and k_pad() tests shift_down[KG_SHIFT] to choose between application and
numeric keypad codes.

Update the counter with smp_store_release() and read it with
smp_load_acquire().

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>
---
 drivers/tty/vt/keyboard.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 87970415a..e6112e0db 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c

@@ -823,7 +823,8 @@ static void k_pad(struct vc_data *vc, unsigned char value, char up_flag)
 		return;		/* no action, if this is a key release */
 
 	/* kludge... shift forces cursor/number keys */
-	if (vc_kbd_mode(kbd, VC_APPLIC) && !shift_down[KG_SHIFT]) {
+	/* Pairs with the smp_store_release() in k_shift(). */
+	if (vc_kbd_mode(kbd, VC_APPLIC) && !smp_load_acquire(&shift_down[KG_SHIFT])) {
 		applkey(vc, app_map[value], 1);
 		return;
 	}

@@ -877,6 +878,7 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 {
 	int old_state = shift_state;
 	int state;
+	unsigned char cnt;
 
 	if (rep)
 		return;

@@ -890,15 +892,18 @@ static void k_shift(struct vc_data *vc, unsigned char value, char up_flag)
 			clr_vc_kbd_led(kbd, VC_CAPSLOCK);
 	}
 
+	cnt = shift_down[value];
 	if (up_flag) {
 		/*
 		 * handle the case that two shift or control
 		 * keys are depressed simultaneously
 		 */
-		if (shift_down[value])
-			shift_down[value]--;
+		if (cnt)
+			cnt--;
 	} else
-		shift_down[value]++;
+		cnt++;
+	/* Pairs with the smp_load_acquire() in k_pad(). */
+	smp_store_release(&shift_down[value], cnt);
 
 	if (shift_down[value])
 		state = shift_state | BIT(value);

-- 
2.43.0

From: Jaidev Shastri <redacted>

do_compute_shiftstate() clears shift_state and shift_down[] and rebuilds
them in place while it iterates key_down[]. vt_get_shift_state() reads
shift_state without kbd_event_lock and can observe the cleared or
partially rebuilt value.

Compute the new state into locals, copy shift_down[] first and publish
shift_state last with smp_store_release().

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>

Sashiko has reviewed this patch and found no issues. It looks great!

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

From: Jaidev Shastri <redacted>

vt_do_kdskbdiacr() and vt_do_kdskbdiacruc() rewrite accent_table[] and
accent_table_size. handle_diacr() reads the size and then walks the
table.

vt_do_kdskbdiacr() sets the size before it converts the entries, so the
size covers entries that have not been written yet. Set it after the
loop in both paths and publish it with smp_store_release(); read it once
with smp_load_acquire() before the walk.

Found with MBCheck, a static herd7-based memory consistency checker.

Signed-off-by: Jaidev Shastri <redacted>

Sashiko has reviewed this patch and found no issues. It looks great!

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vt: unnecessary memory barriers for lockless scalar access
--

commit 97a82ad73b6718204026d341ed2e3831f7cbd52f
Author: Jaidev Shastri [off-list ref]

vt: keyboard: publish shift_state with release semantics

This commit replaces plain reads and writes of shift_state with
smp_load_acquire() and smp_store_release() to specify the relation
between the counters and the summary word.

[ ... ]

[Severity: Low]
Are acquire/release semantics strictly necessary here?

The only lockless reader is vt_get_shift_state(), which simply returns
shift_state directly to userspace when it calls the TIOCL_GETSHIFTSTATE ioctl.
It does not read any dependent memory like the shift_down[] counters.

Without dependent memory reads to sequence after the flag read, the memory
ordering guarantees provided by acquire/release semantics seem unused,
adding memory barrier overhead to every shift key press and release.

Would a simple WRITE_ONCE() here and READ_ONCE() in vt_get_shift_state()
be sufficient to prevent compiler tearing and reordering for this single
scalar value?

 
 	/* kludge */
 	if (up_flag && shift_state != old_state && npadch_active) {

[ ... ]

[Severity: Low]
Could this use a plain READ_ONCE() instead of smp_load_acquire() since
it does not read any dependent memory?

 }
 
 /**

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

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 [off-list ref]

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.

[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.

[ ... ]

[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

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 [off-list ref]

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().

[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.

[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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] tty: vt: unnecessary memory barriers on shift_down
--

commit 00b20e2546808f30870c343c8e766b47b9bd3b39
Author: Jaidev Shastri [off-list ref]

vt: keyboard: publish the shift_down[] counters with release semantics
    
The commit updates accesses to the shift_down[] array by adding
smp_store_release() and smp_load_acquire() semantics. This change was based on
a static analyzer report.

[Severity: Low]
Are these acquire semantics necessary?

A key press or release triggers kbd_event(), acquiring the kbd_event_lock
spinlock. This calls kbd_keycode() which invokes k_shift() and k_pad() via the
k_handler array.

All functions accessing shift_down[] (k_shift(), k_pad(), and
do_compute_shiftstate()) appear to be strictly executed under this spinlock,
meaning there are no lockless readers for shift_down[] in the kernel. Did
the static analyzer produce a false positive by failing to trace the
spinlock across the function pointer invocations in k_handler[]?

 		applkey(vc, app_map[value], 1);
 		return;
 	}

[ ... ]

[Severity: Low]
Are these release semantics necessary?

As mentioned above, since shift_down[] is already protected by the
kbd_event_lock spinlock, are there any lockless readers that require these
memory barriers? Also, as there are plain reads adjacent to the release
store (e.g. cnt = shift_down[value] just above), the implementation would
be flawed if lockless concurrency were actually present.

 
 	if (shift_down[value])
 		state = shift_state | BIT(value);

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

On Mon, Sep 21, 2026 at 09:28:14PM -0400, Jaidev Shastri via B4 Relay wrote:

Again, no.