[PATCH 08/11] fix tt_command_write()

[PATCH 08/11] fix tt_command_write()

[Al Viro]

1) unbalanced debugfs_file_get().  Not needed in the first place -
file_operations are accessed only via debugfs_create_file(), so
debugfs wrappers will take care of that itself.

2) kmalloc() for a buffer used only for duration of a function is not
a problem, but for a buffer no longer than 16 bytes?

3) strstr() is for finding substrings; for finding a character there's
strchr().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/thermal/testing/command.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/thermal/testing/command.c b/drivers/thermal/testing/command.c
index ba11d70e8021..1159ecea57e7 100644
--- a/drivers/thermal/testing/command.c
+++ b/drivers/thermal/testing/command.c
@@ -139,31 +139,21 @@ static int tt_command_exec(int index, const char *arg)
 	return ret;
 }
 
-static ssize_t tt_command_process(struct dentry *dentry, const char __user *user_buf,
-				  size_t count)
+static ssize_t tt_command_process(char *s)
 {
-	char *buf __free(kfree);
 	char *arg;
 	int i;
 
-	buf = kmalloc(count + 1, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
+	strim(s);
 
-	if (copy_from_user(buf, user_buf, count))
-		return -EFAULT;
-
-	buf[count] = '\0';
-	strim(buf);
-
-	arg = strstr(buf, ":");
+	arg = strchr(s, ':');
 	if (arg) {
 		*arg = '\0';
 		arg++;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(tt_command_strings); i++) {
-		if (!strcmp(buf, tt_command_strings[i]))
+		if (!strcmp(s, tt_command_strings[i]))
 			return tt_command_exec(i, arg);
 	}
 
@@ -173,20 +163,20 @@ static ssize_t tt_command_process(struct dentry *dentry, const char __user *user
 static ssize_t tt_command_write(struct file *file, const char __user *user_buf,
 				size_t count, loff_t *ppos)
 {
-	struct dentry *dentry = file->f_path.dentry;
+	char buf[TT_COMMAND_SIZE];
 	ssize_t ret;
 
 	if (*ppos)
 		return -EINVAL;
 
-	if (count + 1 > TT_COMMAND_SIZE)
+	if (count > TT_COMMAND_SIZE - 1)
 		return -E2BIG;
 
-	ret = debugfs_file_get(dentry);
-	if (unlikely(ret))
-		return ret;
+	if (copy_from_user(buf, user_buf, count))
+		return -EFAULT;
+	buf[count] = '\0';
 
-	ret = tt_command_process(dentry, user_buf, count);
+	ret = tt_command_process(buf);
 	if (ret)
 		return ret;
 
-- 
2.39.5

Re: [PATCH 08/11] fix tt_command_write()

[Rafael J. Wysocki]

On Wed, Jul 2, 2025 at 11:25 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> 1) unbalanced debugfs_file_get().  Not needed in the first place -
> file_operations are accessed only via debugfs_create_file(), so
> debugfs wrappers will take care of that itself.
>
> 2) kmalloc() for a buffer used only for duration of a function is not
> a problem, but for a buffer no longer than 16 bytes?
>
> 3) strstr() is for finding substrings; for finding a character there's
> strchr().
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

Or do you want me to apply this?

> ---
>  drivers/thermal/testing/command.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/thermal/testing/command.c b/drivers/thermal/testing/command.c
> index ba11d70e8021..1159ecea57e7 100644
> --- a/drivers/thermal/testing/command.c
> +++ b/drivers/thermal/testing/command.c
> @@ -139,31 +139,21 @@ static int tt_command_exec(int index, const char *arg)
>         return ret;
>  }
>
> -static ssize_t tt_command_process(struct dentry *dentry, const char __user *user_buf,
> -                                 size_t count)
> +static ssize_t tt_command_process(char *s)
>  {
> -       char *buf __free(kfree);
>         char *arg;
>         int i;
>
> -       buf = kmalloc(count + 1, GFP_KERNEL);
> -       if (!buf)
> -               return -ENOMEM;
> +       strim(s);
>
> -       if (copy_from_user(buf, user_buf, count))
> -               return -EFAULT;
> -
> -       buf[count] = '\0';
> -       strim(buf);
> -
> -       arg = strstr(buf, ":");
> +       arg = strchr(s, ':');
>         if (arg) {
>                 *arg = '\0';
>                 arg++;
>         }
>
>         for (i = 0; i < ARRAY_SIZE(tt_command_strings); i++) {
> -               if (!strcmp(buf, tt_command_strings[i]))
> +               if (!strcmp(s, tt_command_strings[i]))
>                         return tt_command_exec(i, arg);
>         }
>
> @@ -173,20 +163,20 @@ static ssize_t tt_command_process(struct dentry *dentry, const char __user *user
>  static ssize_t tt_command_write(struct file *file, const char __user *user_buf,
>                                 size_t count, loff_t *ppos)
>  {
> -       struct dentry *dentry = file->f_path.dentry;
> +       char buf[TT_COMMAND_SIZE];
>         ssize_t ret;
>
>         if (*ppos)
>                 return -EINVAL;
>
> -       if (count + 1 > TT_COMMAND_SIZE)
> +       if (count > TT_COMMAND_SIZE - 1)
>                 return -E2BIG;
>
> -       ret = debugfs_file_get(dentry);
> -       if (unlikely(ret))
> -               return ret;
> +       if (copy_from_user(buf, user_buf, count))
> +               return -EFAULT;
> +       buf[count] = '\0';
>
> -       ret = tt_command_process(dentry, user_buf, count);
> +       ret = tt_command_process(buf);
>         if (ret)
>                 return ret;
>
> --
> 2.39.5
>
>