[PATCH v2] tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl

[PATCH v2] tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl

[Thorsten Blum]

Hoist 'len' and use it in both cases.

Add a comment explaining why reassigning 'kbs' is intentional.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
Changes in v2:
- Keep 'kbs' reassignment and add a comment why it's required (Jiri)
- Link to v1: https://lore.kernel.org/lkml/20260226123419.737669-1-thorsten.blum@linux.dev/
---
 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 13bc048f45e8..88fd4ef2634a 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2000,17 +2000,18 @@ static char *vt_kdskbsent(char *kbs, unsigned char cur)
 int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 {
 	unsigned char kb_func;
+	ssize_t len;
 
 	if (get_user(kb_func, &user_kdgkb->kb_func))
 		return -EFAULT;
 
 	kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);
 
+	/* size should have been a struct member */
+	len = sizeof(user_kdgkb->kb_string);
+
 	switch (cmd) {
 	case KDGKBSENT: {
-		/* size should have been a struct member */
-		ssize_t len = sizeof(user_kdgkb->kb_string);
-
 		char __free(kfree) *kbs = kmalloc(len, GFP_KERNEL);
 		if (!kbs)
 			return -ENOMEM;
@@ -2031,11 +2032,16 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 			return -EPERM;
 
 		char __free(kfree) *kbs = strndup_user(user_kdgkb->kb_string,
-						       sizeof(user_kdgkb->kb_string));
+						       len);
 		if (IS_ERR(kbs))
 			return PTR_ERR(kbs);
 
 		guard(spinlock_irqsave)(&func_buf_lock);
+
+		/*
+		 * Ownership transfer: vt_kdskbsent() returns a pointer
+		 * that must be freed (new buffer, old buffer, or NULL).
+		 */
 		kbs = vt_kdskbsent(kbs, kb_func);
 
 		return 0;
-- 
Thorsten Blum <thorsten.blum@linux.dev>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4

Re: [PATCH v2] tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl

[Greg Kroah-Hartman]

On Mon, Mar 02, 2026 at 04:32:52PM +0100, Thorsten Blum wrote:
> Hoist 'len' and use it in both cases.

Why?  And what is "both cases"?

> Add a comment explaining why reassigning 'kbs' is intentional.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
> Changes in v2:
> - Keep 'kbs' reassignment and add a comment why it's required (Jiri)
> - Link to v1: https://lore.kernel.org/lkml/20260226123419.737669-1-thorsten.blum@linux.dev/
> ---
>  drivers/tty/vt/keyboard.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

I feel you just made the code harder to understand, as you added
complexity :(

> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 13bc048f45e8..88fd4ef2634a 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2000,17 +2000,18 @@ static char *vt_kdskbsent(char *kbs, unsigned char cur)
>  int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  {
>  	unsigned char kb_func;
> +	ssize_t len;
>  
>  	if (get_user(kb_func, &user_kdgkb->kb_func))
>  		return -EFAULT;
>  
>  	kb_func = array_index_nospec(kb_func, MAX_NR_FUNC);
>  
> +	/* size should have been a struct member */
> +	len = sizeof(user_kdgkb->kb_string);
> +
>  	switch (cmd) {
>  	case KDGKBSENT: {
> -		/* size should have been a struct member */
> -		ssize_t len = sizeof(user_kdgkb->kb_string);
> -
>  		char __free(kfree) *kbs = kmalloc(len, GFP_KERNEL);
>  		if (!kbs)
>  			return -ENOMEM;
> @@ -2031,11 +2032,16 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  			return -EPERM;
>  
>  		char __free(kfree) *kbs = strndup_user(user_kdgkb->kb_string,
> -						       sizeof(user_kdgkb->kb_string));
> +						       len);
>  		if (IS_ERR(kbs))
>  			return PTR_ERR(kbs);
>  
>  		guard(spinlock_irqsave)(&func_buf_lock);
> +
> +		/*
> +		 * Ownership transfer: vt_kdskbsent() returns a pointer
> +		 * that must be freed (new buffer, old buffer, or NULL).
> +		 */
>  		kbs = vt_kdskbsent(kbs, kb_func);

That's fine, but what does it have to do with len?

confused,

greg k-h

Re: [PATCH v2] tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl

[Thorsten Blum]

On 12. Mar 2026, at 15:18, Greg Kroah-Hartman wrote:
> On Mon, Mar 02, 2026 at 04:32:52PM +0100, Thorsten Blum wrote:
>> Hoist 'len' and use it in both cases.
> 
> Why?  And what is "both cases"?

To reuse 'len' in both switch cases (KDGKBSENT and KDSKBSENT) instead of
defining 'len = sizeof(user_kdgkb->kb_string)' in KDGKBSENT and inlining
sizeof(user_kdgkb->kb_string) in KDSKBSENT.

>> Add a comment explaining why reassigning 'kbs' is intentional.
>> 
>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>> ---
>> Changes in v2:
>> - Keep 'kbs' reassignment and add a comment why it's required (Jiri)
>> - Link to v1: https://lore.kernel.org/lkml/20260226123419.737669-1-thorsten.blum@linux.dev/
>> ---
>> drivers/tty/vt/keyboard.c | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
> 
> I feel you just made the code harder to understand, as you added
> complexity :(

Not sure how reusing a local variable adds complexity? Would renaming
'len' to 'kb_string_len' help?

>> 		guard(spinlock_irqsave)(&func_buf_lock);
>> +
>> +		/*
>> +		 * Ownership transfer: vt_kdskbsent() returns a pointer
>> +		 * that must be freed (new buffer, old buffer, or NULL).
>> +		 */
>> 		kbs = vt_kdskbsent(kbs, kb_func);
> 
> That's fine, but what does it have to do with len?

It's unrelated to 'len' and just a drive-by change while I was at it.

Thanks,
Thorsten

Re: [PATCH v2] tty: vt/keyboard: Hoist and reuse variable in vt_do_kdgkb_ioctl

[Greg Kroah-Hartman]

On Thu, Mar 12, 2026 at 04:30:00PM +0100, Thorsten Blum wrote:
> On 12. Mar 2026, at 15:18, Greg Kroah-Hartman wrote:
> > On Mon, Mar 02, 2026 at 04:32:52PM +0100, Thorsten Blum wrote:
> >> Hoist 'len' and use it in both cases.
> > 
> > Why?  And what is "both cases"?
> 
> To reuse 'len' in both switch cases (KDGKBSENT and KDSKBSENT) instead of
> defining 'len = sizeof(user_kdgkb->kb_string)' in KDGKBSENT and inlining
> sizeof(user_kdgkb->kb_string) in KDSKBSENT.

As the sizeof() turns into a static number, the code is a bit simpler
as-is, right?

And there's no real need to change this for the sake of changing it that
I can see.

> >> +		/*
> >> +		 * Ownership transfer: vt_kdskbsent() returns a pointer
> >> +		 * that must be freed (new buffer, old buffer, or NULL).
> >> +		 */
> >> 		kbs = vt_kdskbsent(kbs, kb_func);
> > 
> > That's fine, but what does it have to do with len?
> 
> It's unrelated to 'len' and just a drive-by change while I was at it.

Which, by default, makes this patch impossible to accept :(

thanks,

greg k-h