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

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

[Jaidev Shastri via B4 Relay]

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 <jaidevshastri@vt.edu>

[PATCH 1/6] vt: keyboard: publish shift_state with release semantics

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

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 <jaidevshastri@vt.edu>
---
 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

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

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

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 <jaidevshastri@vt.edu>
---
 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

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

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

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 <jaidevshastri@vt.edu>
---
 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

[PATCH 4/6] vt: keyboard: publish accent_table_size with release semantics

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

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 <jaidevshastri@vt.edu>
---
 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

[PATCH 5/6] vt: keyboard: recompute the shift state into locals before publishing it

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

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 <jaidevshastri@vt.edu>
---
 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

[PATCH 6/6] vt: keyboard: publish the shift_down[] counters with release semantics

[Jaidev Shastri via B4 Relay]

From: Jaidev Shastri <jaidevshastri@vt.edu>

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 <jaidevshastri@vt.edu>
---
 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

Re: [PATCH 1/6] vt: keyboard: publish shift_state with release semantics

[Greg Kroah-Hartman]

On Mon, Sep 21, 2026 at 09:28:14PM -0400, Jaidev Shastri via B4 Relay wrote:
> From: Jaidev Shastri <jaidevshastri@vt.edu>
> 
> 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 <jaidevshastri@vt.edu>
> ---
>  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);

Again, no.