Skip to contentSkip to conversation

ext4: Fix diagnostic printf formats

PATCH Applied [email protected] 3 participants 8 messages Mar 26 – Apr 10 ext4 file system

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 ext4: Fix diagnostic printf formats

From: David Laight <[email protected]>

The formats for non-terminated names should be "%.*s" not "%*.s". The kernel currently treats "%*.s" as equivalent to "%*s" whereas userspace requires it be equivalent to "%*.0s".
Neither is correct here.

Signed-off-by David Laight <[email protected]>
---
 fs/ext4/namei.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
fs/ext4/namei.c +3 -3
index c4b5e252af0e..7aaf5fbd4498 100644
@@ -647,7 +647,7 @@ static struct stats dx_show_leaf(struct inode *dir,
 					/* Directory is not encrypted */
 					(void) ext4fs_dirhash(dir, de->name,
 						de->name_len, &h);
-					printk("%*.s:(U)%x.%u ", len,
+					printk("%.*s:(U)%x.%u ", len,
 					       name, h.hash,
 					       (unsigned) ((char *) de
 							   - base));

@@ -683,7 +683,7 @@ static struct stats dx_show_leaf(struct inode *dir,
 						(void) ext4fs_dirhash(dir,
 							de->name,
 							de->name_len, &h);
-					printk("%*.s:(E)%x.%u ", len, name,
+					printk("%.*s:(E)%x.%u ", len, name,
 					       h.hash, (unsigned) ((char *) de
 								   - base));
 					fscrypt_fname_free_buffer(

@@ -694,7 +694,7 @@ static struct stats dx_show_leaf(struct inode *dir,
 				char *name = de->name;
 				(void) ext4fs_dirhash(dir, de->name,
 						      de->name_len, &h);
-				printk("%*.s:%x.%u ", len, name, h.hash,
+				printk("%.*s:%x.%u ", len, name, h.hash,
 				       (unsigned) ((char *) de - base));
 #endif
 			}
Signature · 1 line
2.39.5
#1 lore raw
Quoted 4 lines · On Thu, Mar 26, 2026 at 08:18:04PM +0000, [email protected] wrote:

The formats for non-terminated names should be "%.*s" not "%*.s". The kernel currently treats "%*.s" as equivalent to "%*s" whereas userspace requires it be equivalent to "%*.0s".
Neither is correct here.

This entire code seems was never tested properly and it's a dead code until one defines manually DX_DEBUG. It also has tons of plain printk() calls that may behave differently if the first character is not printable but maps to the level of printk().

I'm not sure how your patch helps with all that, but apparently the printed data has to be NUL-terminated, otherwise I have no idea how it was ever working without crashes.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#2 lore raw
Quoted 7 lines · On Fri, 27 Mar 2026 12:48:56 +0200 Andy Shevchenko…
Quoted 4 lines · On Thu, Mar 26, 2026 at 08:18:04PM +0000, [email protected] wrote:

The formats for non-terminated names should be "%.*s" not "%*.s". The kernel currently treats "%*.s" as equivalent to "%*s" whereas userspace requires it be equivalent to "%*.0s".
Neither is correct here.

Quoted 9 lines

This entire code seems was never tested properly and it's a dead code until one defines manually DX_DEBUG. It also has tons of plain printk() calls that may behave differently if the first character is not printable but maps to the level of printk().

I'm not sure how your patch helps with all that, but apparently the printed data has to be NUL-terminated, otherwise I have no idea how it was ever working without crashes.

I noticed that as well.
I suspect it way have worked for the person that wrote it because the name strings all happened to be NUL terminated.
There is certainly likely to be a '\0' before you 'fall off' mapped memory and crash - so maybe they just ignored the extra characters.

Clearly the other option is to delete it all.
But regardless the format string is wrong.

David

#3 lore raw
Quoted 18 lines · David Laight
Quoted 7 lines · On Fri, 27 Mar 2026 12:48:56 +0200 Andy Shevchenko…
Quoted 4 lines · On Thu, Mar 26, 2026 at 08:18:04PM +0000, [email protected] wrote:

The formats for non-terminated names should be "%.*s" not "%*.s". The kernel currently treats "%*.s" as equivalent to "%*s" whereas userspace requires it be equivalent to "%*.0s".
Neither is correct here.

Quoted 8 lines

This entire code seems was never tested properly and it's a dead code until one defines manually DX_DEBUG. It also has tons of plain printk() calls that may behave differently if the first character is not printable but maps to the level of printk().

I'm not sure how your patch helps with all that, but apparently the printed data has to be NUL-terminated, otherwise I have no idea how it was ever working without crashes.

Quoted 7 lines

I noticed that as well.
I suspect it way have worked for the person that wrote it because the name strings all happened to be NUL terminated.
There is certainly likely to be a '\0' before you 'fall off' mapped memory and crash - so maybe they just ignored the extra characters.

Clearly the other option is to delete it all.

I would go for the history of the change and if it's old enough and not mentioned in any Documentation or not-so-old email thread, kill all that for good. But better to hear the ext4 maintainers first.

Quoted 1 line

But regardless the format string is wrong.

P.S. A bit of off-topic, have you seen this?
https://elixir.bootlin.com/linux/v7.0-rc5/source/kernel/stacktrace.c#L33
Is it correct use of %c?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#4 lore raw

On Fri, 27 Mar 2026 16:12:38 +0200
Andy Shevchenko <[email protected]> wrote:

...

Quoted 4 lines

P.S. A bit of off-topic, have you seen this?
https://elixir.bootlin.com/linux/v7.0-rc5/source/kernel/stacktrace.c#L33
Is it correct use of %c?

Works with glibc (or, rather, with whichever libc the shell I'm using is linked against):

$ printf '|%*c|\n' 5 x
|    x|
$

'man fprintf' tends to agree.
Left justify also works, either "%-*c" or passing -5.

The 'fun' starts if you print a zero with %c in the middle of some output.

I know some compilers have supported: int c = 'abcd'; But I can't remember whether the value could be printed with %4c. I do remember that the value ended up byteswapped in memory on both x86 and sparc.

David

maintainer
#5 lore raw
In reply to Andy Shevchenko
Quoted 10 lines · Andy Shevchenko
Quoted 10 lines
Quoted 3 lines

I'm not sure how your patch helps with all that, but apparently the printed data has to be NUL-terminated, otherwise I have no idea how it was ever working without crashes.

I noticed that as well.
I suspect it way have worked for the person that wrote it because the name strings all happened to be NUL terminated.
There is certainly likely to be a '\0' before you 'fall off' mapped memory and crash - so maybe they just ignored the extra characters.

Quoted 5 lines
Quoted 1 line

Clearly the other option is to delete it all.

I would go for the history of the change and if it's old enough and not mentioned in any Documentation or not-so-old email thread, kill all that for good. But better to hear the ext4 maintainers first.

This is code that can only be manually enabled by adding a

#define DX_DEBUG

to the sources; it's not anything that users can configure using Kconfig. It *has* been used relatively recently, when developers added support for three level htree directories. I'm not sure why they didn't run into the NULL termination issue, but since it is handy to have the debugging code for developers' use, my preference would be to keep the code and fix it up the problems.

- Ted

#6 lore raw
Quoted 19 lines · Theodore Tso
Quoted 10 lines · Andy Shevchenko
Quoted 10 lines
Quoted 3 lines

I'm not sure how your patch helps with all that, but apparently the printed data has to be NUL-terminated, otherwise I have no idea how it was ever working without crashes.

I noticed that as well.
I suspect it way have worked for the person that wrote it because the name strings all happened to be NUL terminated.
There is certainly likely to be a '\0' before you 'fall off' mapped memory and crash - so maybe they just ignored the extra characters.

Quoted 5 lines
Quoted 1 line

Clearly the other option is to delete it all.

I would go for the history of the change and if it's old enough and not mentioned in any Documentation or not-so-old email thread, kill all that for good. But better to hear the ext4 maintainers first.

This is code that can only be manually enabled by adding a

Quoted 8 lines

#define DX_DEBUG

to the sources; it's not anything that users can configure using Kconfig. It *has* been used relatively recently, when developers added support for three level htree directories. I'm not sure why they didn't run into the NULL termination issue, but since it is handy to have the debugging code for developers' use, my preference would be to keep the code and fix it up the problems.

Good to know!

But what is expected input here, i.o.w. should we assume it's always NUL-terminated, or use fixed-length strings? If the latter one is correct the current patch may be applied as is then.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer Applied
#7 lore raw
In reply to [email protected]
Quoted 4 lines · On Thu, 26 Mar 2026 20:18:04 +0000, [email protected] wrote:

The formats for non-terminated names should be "%.*s" not "%*.s". The kernel currently treats "%*.s" as equivalent to "%*s" whereas userspace requires it be equivalent to "%*.0s".
Neither is correct here.

Applied, thanks!

[1/1] ext4: Fix diagnostic printf formats

      commit: 6ea3b34d8625ef5544d1c619bd67e2c6080ea4c2

Best regards,

Signature · 1 line
Theodore Ts'o <[email protected]>