[PATCH v4 0/2] tty: sysrq: fix the reset sequence kept per input device

[PATCH v4 0/2] tty: sysrq: fix the reset sequence kept per input device

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

v3 1/2 was wrong and is dropped.  It stopped a truncated parse from being
cached, but the parse still reached sysrq_detect_reset_sequence() in the
same call and could complete a reset.  param_array() publishes every
intermediate sequence with a fresh version, so no reader-side check can
tell them from the final one; 1/2 now parses the whole parameter in one
callback and replaces it under a seqlock.  2/2 is the same fix adapted to
the new 1/2, and depends on it.

Both fix bugs from the same 2013 commit and apply to every stable tree,
but 1/2 is over the 100-line limit in stable-kernel-rules.rst, so I left
the stable tags out.  The two have to go together.

Tested in QEMU with a uinput keyboard, one device per scenario so the
per-device state survives the parameter writes:

                                           before       after
  hold the full sequence                   reboots      reboots
  hold one key of two                      no           no
  hold the old sequence after a rewrite    no           no
  0x3b,0x3c                                reboots      reboots
  hold the new sequence after a rewrite    no           reboots
  release a stale key, then hold the new   no           reboots
  press key 1 while the write is mid-way   reboots      no

The last row needs a 500 ms delay to widen the window, in
sysrq_reset_seq_param_set() before the series and inside the seqlock
section after it.  Without 2/2's timer_delete() the armed reset survives
the replacement and fires.  Rejected input is the same apart from the
two cases listed in 1/2, and reset_seq reads back the same.

Changes in v4:
 - Dropped v3 1/2 and replaced it with the seqlock patch (Sashiko flagged
   the window on v3 1/2).
 - Reject KEY_RESERVED in the parameter instead of storing it.
 - Dropped the stable tags.
 - Tested in QEMU rather than by inspection only.

Changes in v3:
 - Added a first patch so that a truncated parse no longer sticks
   (Sashiko).
 - Added Fixes: to both patches.

Changes in v2:
 - Retitled from "Input:" to "tty: sysrq:" (Greg Kroah-Hartman).
 - Delete keyreset_timer, otherwise an already armed reset survives the
   reparse (Sashiko).

v3: https://lore.kernel.org/all/20261021022007.1717207-1-zhanxusheng1024@gmail.com/
v2: https://lore.kernel.org/all/20261016070636.2559899-1-zhanxusheng@xiaomi.com/
v1: https://lore.kernel.org/all/20261016032732.2328852-1-zhanxusheng@xiaomi.com/

Zhan Xusheng (2):
  tty: sysrq: don't expose a partially written reset_seq
  tty: sysrq: reset the per-device key state when reparsing reset_seq

 drivers/tty/sysrq.c | 142 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 107 insertions(+), 35 deletions(-)


base-commit: c339f4010dea8b0d754f84ace70cd1b089878ef8
-- 
2.43.0

[PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

reset_seq is a module_param_array, so param_array() builds it up one
element at a time: it resets the element count, then for each element
calls sysrq_reset_seq_param_set(), which stores the key and bumps
sysrq_reset_seq_version, and afterwards increments the count.  Every
intermediate step is a self-consistent sequence carrying a fresh version,
and sysrq_handle_keypress() reparses as soon as the version moves, so
writing 56,29,111 publishes a one-key sequence and then a two-key one
before the intended three-key one.

A key event that lands on the one-key state gives the device a one-key
sequence, and because the parse runs before set_bit(code, key_down) the
count is still 0 and reset_canceled is false.  The same event then reaches
sysrq_detect_reset_sequence(), where ++reset_seq_cnt matches reset_seq_len
and sysrq_handle_reset_request() runs; sysrq_reset_downtime_ms defaults to
0, so sysrq_do_reset() calls orderly_reboot() straight away.  Writing the
parameter needs root and the key has to be the first one of the new
sequence, but an administrator who presses that key while reconfiguring
the sequence reboots the machine.

Today this needs a device that has not parsed a sequence yet, because
reset_seq_len only ever grows.  The next patch resets it on every reparse,
which without this one would expose every device.

Parse the whole parameter in one callback and replace the sequence under
a seqlock.  The reader copies it out under raw_seqcount_try_begin(), as
it runs from the input event path with interrupts disabled, and keeps
its current sequence if a writer got in the way.  It is a seqlock rather
than a bare seqcount because writers were not serialised either:
sysrq_of_get_keyreset_config() stores the sequence from the kernel.sysrq
sysctl without the module parameter lock.

Behaviour changes:
 - A key of 0 is rejected instead of silently ending the sequence there.
 - A write that fails partway keeps the previous sequence.
 - An element longer than seven characters, such as a zero-padded hex
   value, is rejected.

Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 drivers/tty/sysrq.c | 133 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 99 insertions(+), 34 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index f4571d05b496..f9d63b45f859 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -39,6 +39,7 @@
 #include <linux/writeback.h>
 #include <linux/swap.h>
 #include <linux/spinlock.h>
+#include <linux/seqlock.h>
 #include <linux/vt_kern.h>
 #include <linux/workqueue.h>
 #include <linux/hrtimer.h>
@@ -675,39 +676,68 @@ struct sysrq_state {
 	unsigned long reset_keybit[BITS_TO_LONGS(KEY_CNT)];
 	int reset_seq_len;
 	int reset_seq_cnt;
-	int reset_seq_version;
+	unsigned int reset_seq_version;
 	struct timer_list keyreset_timer;
 };
 
 #define SYSRQ_KEY_RESET_MAX	20 /* Should be plenty */
 static unsigned short sysrq_reset_seq[SYSRQ_KEY_RESET_MAX];
 static unsigned int sysrq_reset_seq_len;
-static unsigned int sysrq_reset_seq_version = 1;
+static DEFINE_SEQLOCK(sysrq_reset_seq_lock);
+
+/*
+ * Replace the sequence in one go.  The seqlock serialises writers against
+ * each other and lets sysrq_parse_reset_sequence() tell a complete
+ * sequence from one that is being replaced.
+ */
+static void sysrq_publish_reset_seq(const unsigned short *keys,
+				    unsigned int len)
+{
+	write_seqlock(&sysrq_reset_seq_lock);
+	memcpy(sysrq_reset_seq, keys, len * sizeof(*keys));
+	sysrq_reset_seq_len = len;
+	write_sequnlock(&sysrq_reset_seq_lock);
+}
 
 static void sysrq_parse_reset_sequence(struct sysrq_state *state)
 {
-	int i;
-	unsigned short key;
+	unsigned short keys[SYSRQ_KEY_RESET_MAX];
+	unsigned int seq, len, i;
 
-	state->reset_seq_cnt = 0;
+	/*
+	 * This runs from the input event path with interrupts disabled, so
+	 * it must not wait for a writer.  If one is active, or finishes
+	 * while the copy is being made, keep the sequence the device
+	 * already has and try again on the next key event.
+	 */
+	if (!raw_seqcount_try_begin(&sysrq_reset_seq_lock.seqcount, seq))
+		return;
 
-	for (i = 0; i < sysrq_reset_seq_len; i++) {
-		key = sysrq_reset_seq[i];
+	if (state->reset_seq_version == seq)
+		return;
 
-		if (key == KEY_RESERVED || key > KEY_MAX)
-			break;
+	len = sysrq_reset_seq_len;
+	if (len > SYSRQ_KEY_RESET_MAX)
+		return;
+	memcpy(keys, sysrq_reset_seq, len * sizeof(keys[0]));
 
-		__set_bit(key, state->reset_keybit);
+	if (read_seqcount_retry(&sysrq_reset_seq_lock.seqcount, seq))
+		return;
+
+	state->reset_seq_cnt = 0;
+
+	for (i = 0; i < len; i++) {
+		__set_bit(keys[i], state->reset_keybit);
 		state->reset_seq_len++;
 
-		if (test_bit(key, state->key_down))
+		if (test_bit(keys[i], state->key_down))
 			state->reset_seq_cnt++;
 	}
 
 	/* Disable reset until old keys are not released */
 	state->reset_canceled = state->reset_seq_cnt != 0;
 
-	state->reset_seq_version = sysrq_reset_seq_version;
+	state->reset_seq_version = seq;
 }
 
 static void sysrq_do_reset(struct timer_list *t)
@@ -768,6 +798,8 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
 #ifdef CONFIG_OF
 static void sysrq_of_get_keyreset_config(void)
 {
+	unsigned short keys[SYSRQ_KEY_RESET_MAX];
+	unsigned int len = 0;
 	u32 key;
 	struct device_node *np;
 
@@ -777,17 +809,16 @@ static void sysrq_of_get_keyreset_config(void)
 		return;
 	}
 
-	/* Reset in case a __weak definition was present */
-	sysrq_reset_seq_len = 0;
-
 	of_property_for_each_u32(np, "keyset", key) {
 		if (key == KEY_RESERVED || key > KEY_MAX ||
-		    sysrq_reset_seq_len == SYSRQ_KEY_RESET_MAX)
+		    len == SYSRQ_KEY_RESET_MAX)
 			break;
 
-		sysrq_reset_seq[sysrq_reset_seq_len++] = (unsigned short)key;
+		keys[len++] = (unsigned short)key;
 	}
 
+	sysrq_publish_reset_seq(keys, len);
+
 	/* Get reset timeout if any. */
 	of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
 
@@ -903,8 +934,7 @@ static bool sysrq_handle_keypress(struct sysrq_state *sysrq,
 		/*
 		 * See if reset sequence has changed since the last time.
 		 */
-		if (sysrq->reset_seq_version != sysrq_reset_seq_version)
-			sysrq_parse_reset_sequence(sysrq);
+		sysrq_parse_reset_sequence(sysrq);
 
 		/*
 		 * If we are not suppressing key presses keep track of
@@ -1055,36 +1085,71 @@ static inline void sysrq_unregister_handler(void)
 static int sysrq_reset_seq_param_set(const char *buffer,
 				     const struct kernel_param *kp)
 {
-	unsigned long val;
-	int error;
+	unsigned short keys[SYSRQ_KEY_RESET_MAX];
+	unsigned int len = 0;
+	const char *pos = buffer;
+
+	for (;;) {
+		const char *end = strchrnul(pos, ',');
+		size_t n = end - pos;
+		char token[8];
+		u16 key;
 
-	error = kstrtoul(buffer, 0, &val);
-	if (error < 0)
-		return error;
+		if (len == SYSRQ_KEY_RESET_MAX || !n || n >= sizeof(token))
+			return -EINVAL;
 
-	if (val > KEY_MAX)
-		return -EINVAL;
+		memcpy(token, pos, n);
+		token[n] = '\0';
 
-	*((unsigned short *)kp->arg) = val;
-	sysrq_reset_seq_version++;
+		if (kstrtou16(token, 0, &key) || key == KEY_RESERVED ||
+		    key > KEY_MAX)
+			return -EINVAL;
+
+		keys[len++] = key;
+
+		if (!*end)
+			break;
+		pos = end + 1;
+	}
+
+	sysrq_publish_reset_seq(keys, len);
 
 	return 0;
 }
 
+static int sysrq_reset_seq_param_get(char *buffer,
+				     const struct kernel_param *kp)
+{
+	unsigned short keys[SYSRQ_KEY_RESET_MAX];
+	unsigned int seq, len, i;
+	int off = 0;
+
+	do {
+		seq = read_seqbegin(&sysrq_reset_seq_lock);
+		len = sysrq_reset_seq_len;
+		memcpy(keys, sysrq_reset_seq, len * sizeof(keys[0]));
+	} while (read_seqretry(&sysrq_reset_seq_lock, seq));
+
+	if (!len)
+		return 0;
+
+	for (i = 0; i < len; i++)
+		off += scnprintf(buffer + off, PAGE_SIZE - off, "%s%hu",
+				 i ? "," : "", keys[i]);
+
+	return off + scnprintf(buffer + off, PAGE_SIZE - off, "\n");
+}
+
 static const struct kernel_param_ops param_ops_sysrq_reset_seq = {
-	.get	= param_get_ushort,
+	.get	= sysrq_reset_seq_param_get,
 	.set	= sysrq_reset_seq_param_set,
 };
 
-#define param_check_sysrq_reset_seq(name, p)	\
-	__param_check(name, p, unsigned short)
-
 /*
  * not really modular, but the easiest way to keep compat with existing
  * bootargs behaviour is to continue using module_param here.
  */
-module_param_array_named(reset_seq, sysrq_reset_seq, sysrq_reset_seq,
-			 &sysrq_reset_seq_len, 0644);
+module_param_cb(reset_seq, &param_ops_sysrq_reset_seq, NULL, 0644);
 
 module_param_named(sysrq_downtime_ms, sysrq_reset_downtime_ms, int, 0644);
 
-- 
2.43.0

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[sashiko-bot]

> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 
> reset_seq is a module_param_array, so param_array() builds it up one
> element at a time: it resets the element count, then for each element
> calls sysrq_reset_seq_param_set(), which stores the key and bumps
> sysrq_reset_seq_version, and afterwards increments the count.  Every
> intermediate step is a self-consistent sequence carrying a fresh version,
> and sysrq_handle_keypress() reparses as soon as the version moves, so
> writing 56,29,111 publishes a one-key sequence and then a two-key one
> before the intended three-key one.
> 
> A key event that lands on the one-key state gives the device a one-key
> sequence, and because the parse runs before set_bit(code, key_down) the
> count is still 0 and reset_canceled is false.  The same event then reaches
> sysrq_detect_reset_sequence(), where ++reset_seq_cnt matches reset_seq_len
> [ ... ]
>  - An element longer than seven characters, such as a zero-padded hex
>    value, is rejected.
> 
> Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030045854.153556-2-zhanxusheng1024@gmail.com?part=1

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Greg KH]

On Fri, Oct 30, 2026 at 12:58:53PM +0800, Zhan Xusheng wrote:
> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 
> reset_seq is a module_param_array, so param_array() builds it up one
> element at a time: it resets the element count, then for each element
> calls sysrq_reset_seq_param_set(), which stores the key and bumps
> sysrq_reset_seq_version, and afterwards increments the count.  Every
> intermediate step is a self-consistent sequence carrying a fresh version,
> and sysrq_handle_keypress() reparses as soon as the version moves, so
> writing 56,29,111 publishes a one-key sequence and then a two-key one
> before the intended three-key one.
> 
> A key event that lands on the one-key state gives the device a one-key
> sequence, and because the parse runs before set_bit(code, key_down) the
> count is still 0 and reset_canceled is false.  The same event then reaches
> sysrq_detect_reset_sequence(), where ++reset_seq_cnt matches reset_seq_len
> and sysrq_handle_reset_request() runs; sysrq_reset_downtime_ms defaults to
> 0, so sysrq_do_reset() calls orderly_reboot() straight away.  Writing the
> parameter needs root and the key has to be the first one of the new
> sequence, but an administrator who presses that key while reconfiguring
> the sequence reboots the machine.
> 
> Today this needs a device that has not parsed a sequence yet, because
> reset_seq_len only ever grows.  The next patch resets it on every reparse,
> which without this one would expose every device.
> 
> Parse the whole parameter in one callback and replace the sequence under
> a seqlock.  The reader copies it out under raw_seqcount_try_begin(), as
> it runs from the input event path with interrupts disabled, and keeps
> its current sequence if a writer got in the way.  It is a seqlock rather
> than a bare seqcount because writers were not serialised either:
> sysrq_of_get_keyreset_config() stores the sequence from the kernel.sysrq
> sysctl without the module parameter lock.

I'm sorry, but I can't really parse this llm-generated text.  What is
the bug here.  What is being fixed?  And what is changing?

> Behaviour changes:
>  - A key of 0 is rejected instead of silently ending the sequence there.
>  - A write that fails partway keeps the previous sequence.
>  - An element longer than seven characters, such as a zero-padded hex
>    value, is rejected.

So what workflow hits this and will need to be changed if this patch is
applied?

> Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")

no cc: stable?

And how has no one ever hit this issue in the decades that it has been
present?  Is this really something that needs to be fixed?  If so, why?

thanks,

greg k-h

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Zhan Xusheng]

Hi Greg,

On Fri, Oct 30, 2026 at 07:05:10AM +0100, Greg KH wrote:
> I'm sorry, but I can't really parse this llm-generated text.  What is
> the bug here.  What is being fixed?  And what is changing?

Sorry, that was unreadable.

reset_seq can be changed at runtime, but the new value doesn't work:
each keyboard keeps counting the old keys too, so pressing the new
sequence does nothing.  2/2 fixes that.

1/2 is for a race that 2/2 makes easier to hit.  The new list is filled
in one key at a time and the keyboards pick up each partial list, so
pressing the first new key during the write can reboot the machine.

> And how has no one ever hit this issue in the decades that it has been
> present?  Is this really something that needs to be fixed?  If so, why?

The sequence is usually set on the command line or in DT, and that
works.  Both bugs need a runtime change, and the race also needs a key
press during the write, which takes microseconds.  I only hit it with
an added delay.

1/2 is far too big for that.  Telling the keyboards about the new value
only once the whole list is written avoids the race with a small patch,
and doesn't change what the parameter accepts.  I can send that with
2/2 as v5, or drop the series if runtime changes aren't worth fixing.

Thanks,
Zhan Xusheng

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Greg KH]

On Fri, Oct 30, 2026 at 02:30:53PM +0800, Zhan Xusheng wrote:
> Hi Greg,
> 
> On Fri, Oct 30, 2026 at 07:05:10AM +0100, Greg KH wrote:
> > I'm sorry, but I can't really parse this llm-generated text.  What is
> > the bug here.  What is being fixed?  And what is changing?
> 
> Sorry, that was unreadable.
> 
> reset_seq can be changed at runtime, but the new value doesn't work:
> each keyboard keeps counting the old keys too, so pressing the new
> sequence does nothing.  2/2 fixes that.

So the new value never worked?  This feature does not work?

> 1/2 is for a race that 2/2 makes easier to hit.  The new list is filled
> in one key at a time and the keyboards pick up each partial list, so
> pressing the first new key during the write can reboot the machine.

So the issue is that changing the sequence can race with what exactly?

> > And how has no one ever hit this issue in the decades that it has been
> > present?  Is this really something that needs to be fixed?  If so, why?
> 
> The sequence is usually set on the command line or in DT, and that
> works.  Both bugs need a runtime change, and the race also needs a key
> press during the write, which takes microseconds.  I only hit it with
> an added delay.

What race exactly?  That when this is being changed "something happens"?

> 1/2 is far too big for that.  Telling the keyboards about the new value
> only once the whole list is written avoids the race with a small patch,
> and doesn't change what the parameter accepts.  I can send that with
> 2/2 as v5, or drop the series if runtime changes aren't worth fixing.

I still don't understand what the root problem is here, sorry.

thanks,

greg k-h

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Zhan Xusheng]

Hi Greg,

On Fri, Oct 30, 2026 at 08:02:30AM +0100, Greg KH wrote:
> I still don't understand what the root problem is here, sorry.

Each keyboard keeps its own copy of reset_seq.  It rebuilds the copy on
the first key press after the parameter changes, and it rebuilds it
wrong in two ways.

1) It adds the new keys to the old copy instead of replacing it (2/2):

  echo 56,29,111 > /sys/module/sysrq/parameters/reset_seq # Alt+Ctrl+Del
  (press any key)
  echo 59,60 > /sys/module/sysrq/parameters/reset_seq     # F1+F2

Now F1+F2 does nothing.  Only holding all five keys resets.  So setting
reset_seq once works, but changing it later does not.

2) It can rebuild the copy while the write is half done (1/2).  The
write stores the keys one at a time, and the keyboard may read the list
in between:

  echo 59,60 > reset_seq
    store 59
                      <- F1 pressed: keyboard reads "59" as the sequence
    store 60

On a keyboard that has not used a reset sequence before, or on any
keyboard once 2/2 is applied, that F1 press completes the one-key
sequence and reboots the machine.  The window is microseconds; I only
hit it with an added delay.

If you want this fixed, I'll send a v5 with a much smaller 1/2, plain
changelogs, and the Assisted-by: tag I should have added from the
start.  Otherwise please drop it.

Thanks,
Zhan Xusheng

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Greg KH]

On Fri, Oct 30, 2026 at 03:58:20PM +0800, Zhan Xusheng wrote:
> Hi Greg,
> 
> On Fri, Oct 30, 2026 at 08:02:30AM +0100, Greg KH wrote:
> > I still don't understand what the root problem is here, sorry.
> 
> Each keyboard keeps its own copy of reset_seq.  It rebuilds the copy on
> the first key press after the parameter changes, and it rebuilds it
> wrong in two ways.
> 
> 1) It adds the new keys to the old copy instead of replacing it (2/2):
> 
>   echo 56,29,111 > /sys/module/sysrq/parameters/reset_seq # Alt+Ctrl+Del
>   (press any key)
>   echo 59,60 > /sys/module/sysrq/parameters/reset_seq     # F1+F2
> 
> Now F1+F2 does nothing.  Only holding all five keys resets.  So setting
> reset_seq once works, but changing it later does not.

So this feature has never worked?  Or it is just "one change only"?  So
your change is an addition to the feature, not a bugfix?

> 2) It can rebuild the copy while the write is half done (1/2).  The
> write stores the keys one at a time, and the keyboard may read the list
> in between:
> 
>   echo 59,60 > reset_seq
>     store 59
>                       <- F1 pressed: keyboard reads "59" as the sequence
>     store 60
> 
> On a keyboard that has not used a reset sequence before, or on any
> keyboard once 2/2 is applied, that F1 press completes the one-key
> sequence and reboots the machine.  The window is microseconds; I only
> hit it with an added delay.

So this isn't a "real" issue?  If you are modifying the key sequence,
how can you press the key at the same time?  Doing "major" rework for
that might just not be worth it.

thanks,

greg k-h

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Zhan Xusheng]

Hi Greg,

On Fri, Oct 30, 2026 at 09:33:41AM +0100, Greg KH wrote:
> So this feature has never worked?  Or it is just "one change only"?  So
> your change is an addition to the feature, not a bugfix?

It is a fix.  154b7a489a5b ("Input: sysrq - allow specifying alternate
reset sequence") added reset_seq as a writable parameter, together with
a version counter so that each keyboard rereads the sequence after a
write.  The reread just never clears the old keys, so on a keyboard
that already used the old value, a new one has never worked.  Hardly
anyone changes it at runtime, which is likely why nobody noticed.

> So this isn't a "real" issue?  If you are modifying the key sequence,
> how can you press the key at the same time?  Doing "major" rework for
> that might just not be worth it.

Agreed, please drop 1/2.  With only 2/2, that race can hit any
keyboard, not only ones that never used a sequence.  Do you want 2/2
resent on its own?

Thanks,
Zhan Xusheng

Re: [PATCH v4 1/2] tty: sysrq: don't expose a partially written reset_seq

[Greg KH]

On Fri, Oct 30, 2026 at 07:33:56PM +0800, Zhan Xusheng wrote:
> Hi Greg,
> 
> On Fri, Oct 30, 2026 at 09:33:41AM +0100, Greg KH wrote:
> > So this feature has never worked?  Or it is just "one change only"?  So
> > your change is an addition to the feature, not a bugfix?
> 
> It is a fix.  154b7a489a5b ("Input: sysrq - allow specifying alternate
> reset sequence") added reset_seq as a writable parameter, together with
> a version counter so that each keyboard rereads the sequence after a
> write.  The reread just never clears the old keys, so on a keyboard
> that already used the old value, a new one has never worked.  Hardly
> anyone changes it at runtime, which is likely why nobody noticed.
> 
> > So this isn't a "real" issue?  If you are modifying the key sequence,
> > how can you press the key at the same time?  Doing "major" rework for
> > that might just not be worth it.
> 
> Agreed, please drop 1/2.  With only 2/2, that race can hit any
> keyboard, not only ones that never used a sequence.  Do you want 2/2
> resent on its own?

Please resend whatever you feel should actually be applied, with better
changelog text explaining all of this.

thanks,

greg k-h

[PATCH v4 2/2] tty: sysrq: reset the per-device key state when reparsing reset_seq

[Zhan Xusheng]

From: Zhan Xusheng <zhanxusheng@xiaomi.com>

sysrq_parse_reset_sequence() clears reset_seq_cnt, but reset_keybit is
only ever __set_bit() and reset_seq_len only ever incremented.  struct
sysrq_state is allocated once per input device in sysrq_connect() and
lives until the device goes away, so both accumulate every time the
sequence is reparsed, which happens whenever the mode 0644 reset_seq
parameter is written.

After writing 56,29,111 and then 59,60 a device keeps all five keys in
reset_keybit with reset_seq_len 5, so the effective combination is the
union of the old and the new sequence: holding just 59 and 60 counts two
of five and does nothing.  Releasing one of the three stale keys takes the
key-release path, because the key still tests positive in reset_keybit, so
reset_seq_cnt is decremented for a key that was never counted and goes
negative.  From there ++reset_seq_cnt has to climb back through zero
before it can match reset_seq_len, so the emergency reset cannot be
triggered until reset_seq is written again or the device reconnects.

Rebuild both from the sequence that was just read.  Also delete
keyreset_timer: with sysrq_downtime_ms set, holding a full sequence arms
it, and it is only deleted on a press outside the sequence or a release
inside it.  Dropping the old keys from reset_keybit removes both of those
cancellations, so an already armed reset would otherwise survive into
orderly_reboot().

This depends on the previous patch.  Clearing reset_seq_len turns what
used to be a stale superset of the old and the new keys into a real
truncation, so without the sequence being published in one step the
single-keypress reboot described there would reach every device rather
than only those that have never parsed a sequence.

Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>
---
 drivers/tty/sysrq.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index f9d63b45f859..00cea2f315c3 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -724,11 +724,18 @@ static void sysrq_parse_reset_sequence(struct sysrq_state *state)
 	if (read_seqcount_retry(&sysrq_reset_seq_lock.seqcount, seq))
 		return;
 
+	/*
+	 * An armed reset belongs to the sequence being replaced: once the old
+	 * keys are gone from reset_keybit their release no longer cancels it.
+	 */
+	timer_delete(&state->keyreset_timer);
+
+	bitmap_zero(state->reset_keybit, KEY_CNT);
+	state->reset_seq_len = len;
 	state->reset_seq_cnt = 0;
 
 	for (i = 0; i < len; i++) {
 		__set_bit(keys[i], state->reset_keybit);
-		state->reset_seq_len++;
 
 		if (test_bit(keys[i], state->key_down))
 			state->reset_seq_cnt++;
-- 
2.43.0

Re: [PATCH v4 2/2] tty: sysrq: reset the per-device key state when reparsing reset_seq

[sashiko-bot]

> From: Zhan Xusheng <zhanxusheng@xiaomi.com>
> 
> sysrq_parse_reset_sequence() clears reset_seq_cnt, but reset_keybit is
> only ever __set_bit() and reset_seq_len only ever incremented.  struct
> sysrq_state is allocated once per input device in sysrq_connect() and
> lives until the device goes away, so both accumulate every time the
> sequence is reparsed, which happens whenever the mode 0644 reset_seq
> parameter is written.
> 
> After writing 56,29,111 and then 59,60 a device keeps all five keys in
> reset_keybit with reset_seq_len 5, so the effective combination is the
> union of the old and the new sequence: holding just 59 and 60 counts two
> of five and does nothing.  Releasing one of the three stale keys takes the
> key-release path, because the key still tests positive in reset_keybit, so
> reset_seq_cnt is decremented for a key that was never counted and goes
> [ ... ]
> single-keypress reboot described there would reach every device rather
> than only those that have never parsed a sequence.
> 
> Fixes: 154b7a489a5b ("Input: sysrq - allow specifying alternate reset sequence")
> Signed-off-by: Zhan Xusheng <zhanxusheng@xiaomi.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030045854.153556-2-zhanxusheng1024@gmail.com?part=2