btrfs: Replace memcpy + NUL termination in _btrfs_printk

[PATCH] btrfs: Replace memcpy + NUL termination in _btrfs_printk

Posted by Thorsten Blum 2 months, 1 week ago

Use strscpy() to copy the NUL-terminated source string 'fmt' to the
destination buffer 'lvl' instead of using memcpy() followed by a manual
NUL termination.  No functional changes.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 fs/btrfs/messages.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c
index a0cf8effe008..083e228e6d6c 100644
--- a/fs/btrfs/messages.c
+++ b/fs/btrfs/messages.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/string.h>
 #include "fs.h"
 #include "messages.h"
 #include "discard.h"
@@ -229,8 +230,7 @@ void __cold _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt,
 		size_t size = printk_skip_level(fmt) - fmt;
 
 		if (kern_level >= '0' && kern_level <= '7') {
-			memcpy(lvl, fmt,  size);
-			lvl[size] = '\0';
+			strscpy(lvl, fmt, size + 1);
 			type = logtypes[kern_level - '0'];
 			ratelimit = &printk_limits[kern_level - '0'];
 		}
-- 
Thorsten Blum <thorsten.blum@linux.dev>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4

Re: [PATCH] btrfs: Replace memcpy + NUL termination in _btrfs_printk

Posted by david laight 2 months, 1 week ago

On Sun, 30 Nov 2025 01:55:17 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> Use strscpy() to copy the NUL-terminated source string 'fmt' to the
> destination buffer 'lvl' instead of using memcpy() followed by a manual
> NUL termination.  No functional changes.

Why?
The code has just got the length of part of the format string, it wants
a copy of it with a '\0' terminator.
So memcpy() is correct and strscpy() just expensive.
The code is actually strange (and strangely written), but 'size' is always 2.

One might question why btrfs has to invent its own 'printk' scheme...

But PRINTK_MAX_SINGLE_HEADER_LEN is only used here - so take out the 'MAX_'
and use it inside printk_skip_level'.
Then here use the constant PRINTK_SINGLE_HEADER_LEN instead of 'size'.
Since lvl[] is initialised it will all be zero, so after the
mempy(lvl, fmt, 2) (which will be inlined) there is no need to the
lvl[2] = 0 at all.

	David

> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  fs/btrfs/messages.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c
> index a0cf8effe008..083e228e6d6c 100644
> --- a/fs/btrfs/messages.c
> +++ b/fs/btrfs/messages.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> +#include <linux/string.h>
>  #include "fs.h"
>  #include "messages.h"
>  #include "discard.h"
> @@ -229,8 +230,7 @@ void __cold _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt,
>  		size_t size = printk_skip_level(fmt) - fmt;
>  
>  		if (kern_level >= '0' && kern_level <= '7') {
> -			memcpy(lvl, fmt,  size);
> -			lvl[size] = '\0';
> +			strscpy(lvl, fmt, size + 1);
>  			type = logtypes[kern_level - '0'];
>  			ratelimit = &printk_limits[kern_level - '0'];
>  		}

Re: [PATCH] btrfs: Replace memcpy + NUL termination in _btrfs_printk

Posted by David Sterba 2 months ago

On Sun, Nov 30, 2025 at 11:06:40AM +0000, david laight wrote:
> On Sun, 30 Nov 2025 01:55:17 +0100
> Thorsten Blum <thorsten.blum@linux.dev> wrote:
> 
> > Use strscpy() to copy the NUL-terminated source string 'fmt' to the
> > destination buffer 'lvl' instead of using memcpy() followed by a manual
> > NUL termination.  No functional changes.
> 
> Why?
> The code has just got the length of part of the format string, it wants
> a copy of it with a '\0' terminator.
> So memcpy() is correct and strscpy() just expensive.
> The code is actually strange (and strangely written), but 'size' is always 2.
> 
> One might question why btrfs has to invent its own 'printk' scheme...

The first code of the printk helper was added in 2012 as 4da35113426d
("btrfs: add varargs to btrfs_error") and since then it evolved a lot
and I'm not sure we still need it.

Own helpers for messages insert the filesystem identification in front
of the message. There's per-level ratelimit which needs the parsing
added in commit 35f4e5e6f198 ("btrfs: Add ratelimit to btrfs printing").
This can be possibly removed as we can ratelimit specific messages if
they're known to be noisy.

Re: [PATCH] btrfs: Replace memcpy + NUL termination in _btrfs_printk

Posted by David Sterba 1 month, 3 weeks ago

On Wed, Dec 03, 2025 at 02:45:58PM +0100, David Sterba wrote:
> On Sun, Nov 30, 2025 at 11:06:40AM +0000, david laight wrote:
> > On Sun, 30 Nov 2025 01:55:17 +0100
> > Thorsten Blum <thorsten.blum@linux.dev> wrote:
> > 
> > > Use strscpy() to copy the NUL-terminated source string 'fmt' to the
> > > destination buffer 'lvl' instead of using memcpy() followed by a manual
> > > NUL termination.  No functional changes.
> > 
> > Why?
> > The code has just got the length of part of the format string, it wants
> > a copy of it with a '\0' terminator.
> > So memcpy() is correct and strscpy() just expensive.
> > The code is actually strange (and strangely written), but 'size' is always 2.
> > 
> > One might question why btrfs has to invent its own 'printk' scheme...
> 
> The first code of the printk helper was added in 2012 as 4da35113426d
> ("btrfs: add varargs to btrfs_error") and since then it evolved a lot
> and I'm not sure we still need it.
> 
> Own helpers for messages insert the filesystem identification in front
> of the message. There's per-level ratelimit which needs the parsing
> added in commit 35f4e5e6f198 ("btrfs: Add ratelimit to btrfs printing").
> This can be possibly removed as we can ratelimit specific messages if
> they're known to be noisy.

For the record the level parsing is now gone
https://lore.kernel.org/linux-btrfs/f884e72071839aeb0f8b77e79a6ae2d0bc8adf78.1765299883.git.dsterba@suse.com/