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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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