When we use 'echo' to write a string to printk_devkmsg, it writes '\0' at
the end. It means lenp is larger then the length of string 1. However,
sometimes we write data with string length by other way, e.g
    write(fd, string, strlen(string));
In this case, the writing will fail.

Comparing err with string length is able to handle all scenarios.

Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
---
 kernel/printk/printk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 357a4d18f6387..992872f7b7747 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -229,7 +229,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 		 * Do not accept an unknown string OR a known string with
 		 * trailing crap...
 		 */
-		if (err < 0 || (err + 1 != *lenp)) {
+		if (err != strlen(devkmsg_log_str)) {
 
 			/* ... and restore old setting. */
 			devkmsg_log = old;
-- 
2.27.0

First, I am sorry for the late review. So many things have accumulated
during my vacation.

On Thu 2023-08-17 07:47:45, Yun Zhou wrote:
> When we use 'echo' to write a string to printk_devkmsg, it writes '\0' at
> the end. It means lenp is larger then the length of string 1. However,
> sometimes we write data with string length by other way, e.g
>     write(fd, string, strlen(string));
> In this case, the writing will fail.
> 
> Comparing err with string length is able to handle all scenarios.
> 
> Signed-off-by: Yun Zhou <yun.zhou@windriver.com>
> ---
>  kernel/printk/printk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 357a4d18f6387..992872f7b7747 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -229,7 +229,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  		 * Do not accept an unknown string OR a known string with
>  		 * trailing crap...
>  		 */
> -		if (err < 0 || (err + 1 != *lenp)) {
> +		if (err != strlen(devkmsg_log_str)) {
>  
>  			/* ... and restore old setting. */
>  			devkmsg_log = old;

I see. _proc_do_string() does the following:

_proc_do_string(char *data, int maxlen, int write,
		char *buffer, size_t *lenp, loff_t *ppos)
{
[...]
	if (write) {
[...]
		*ppos += *lenp;
		p = buffer;
		while ((p - buffer) < *lenp && len < maxlen - 1) {
			c = *(p++);
			if (c == 0 || c == '\n')
				break;
			data[len++] = c;
		}
		data[len] = 0;

It means that it proceeds "*lenp" characters but the trailing
'\0' need not match. It might be inserted earlier when
(c == 0 || c == '\n') was true earlier. Or one bite behind when
the trailing '\0' was missing within the *lenp characters.

As a result, the *lenp check is not reliable.

Reviewed-by: Petr Mladek <pmladek@suse.com>