xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths

PATCH In review Kees Cook 4 participants 5 messages Mar 23 XFS filesystem

lore

Share & tools

lore

Kees Cook

author

2026-03-23 17:22 UTC #0 lore raw

PATCH 1/1 xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths

Replace the deprecated[1] strncpy() with strscpy_pad() in the xfile_create and xmbuf_create tracepoints.

Both tracepoints use file_path() to resolve a pathname into __entry->pathname (a char[MAXNAMELEN] trace ring buffer field). On failure, the error path overwrites the buffer with the string literal "(unknown)" via strncpy(). The original strncpy() zero-pads the remaining 246 bytes (MAXNAMELEN is 256, "(unknown)" is 10 bytes including NUL).

strscpy_pad() preserves this zero-padding, which matters because the destination is a trace ring buffer entry: ring buffer slots are not zeroed on allocation, and the raw buffer is readable by userspace via tracefs. The zero-padding ensures no stale data remains in the buffer after the error path overwrites it.

The source is a 10-byte string literal into a 256-byte destination, so there is no behavioral change.

Link https://github.com/KSPP/linux/issues/90 [1]

Signed-off-by Kees Cook <[email protected]>

---
 fs/xfs/scrub/trace.h | 3 +--
 fs/xfs/xfs_trace.h   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

fs/xfs/scrub/trace.h +1 -2

index 39ea651cbb75..46c420f51129 100644
@@ -980,8 +980,7 @@ TRACE_EVENT(xfile_create,
 		__entry->ino = file_inode(xf->file)->i_ino;
 		path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("xfino 0x%lx path '%s'",
 		  __entry->ino,

fs/xfs/xfs_trace.h +1 -2

index 813e5a9f57eb..9f9fb86097ed 100644
@@ -5101,8 +5101,7 @@ TRACE_EVENT(xmbuf_create,
 		__entry->ino = file_inode(file)->i_ino;
 		path = file_path(file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("dev %d:%d xmino 0x%lx path '%s'",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),

Signature · 1 line

2.34.1

Darrick J. Wong

Reviewed-by on v1 1/1

2026-03-23 18:24 UTC #1 lore raw

Quoted 16 lines · Kees Cook

Replace the deprecated[1] strncpy() with strscpy_pad() in the xfile_create and xmbuf_create tracepoints.
Both tracepoints use file_path() to resolve a pathname into __entry->pathname (a char[MAXNAMELEN] trace ring buffer field). On failure, the error path overwrites the buffer with the string literal "(unknown)" via strncpy(). The original strncpy() zero-pads the remaining 246 bytes (MAXNAMELEN is 256, "(unknown)" is 10 bytes including NUL).
strscpy_pad() preserves this zero-padding, which matters because the destination is a trace ring buffer entry: ring buffer slots are not zeroed on allocation, and the raw buffer is readable by userspace via tracefs. The zero-padding ensures no stale data remains in the buffer after the error path overwrites it.

Quoted 5 lines

The source is a 10-byte string literal into a 256-byte destination, so there is no behavioral change.
Link https://github.com/KSPP/linux/issues/90 [1]
Signed-off-by Kees Cook <[email protected]>

Seems fine to me,

Reviewed-by "Darrick J. Wong" <[email protected]>

--D

Quoted 37 lines

---
 fs/xfs/scrub/trace.h | 3 +--
 fs/xfs/xfs_trace.h   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

fs/xfs/scrub/trace.h +1 -2

index 39ea651cbb75..46c420f51129 100644
@@ -980,8 +980,7 @@ TRACE_EVENT(xfile_create,
 		__entry->ino = file_inode(xf->file)->i_ino;
 		path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("xfino 0x%lx path '%s'",
 		  __entry->ino,

fs/xfs/xfs_trace.h +1 -2

index 813e5a9f57eb..9f9fb86097ed 100644
@@ -5101,8 +5101,7 @@ TRACE_EVENT(xmbuf_create,
 		__entry->ino = file_inode(file)->i_ino;
 		path = file_path(file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("dev %d:%d xmino 0x%lx path '%s'",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),

Signature · 1 line

2.34.1

Steven Rostedt

2026-03-23 19:24 UTC #2 lore raw

In reply to Kees Cook

Quoted 10 lines · On Mon, 23 Mar 2026 10:22:09 -0700 Kees Cook <[email protected]> wrote:

Replace the deprecated[1] strncpy() with strscpy_pad() in the xfile_create and xmbuf_create tracepoints.
Both tracepoints use file_path() to resolve a pathname into __entry->pathname (a char[MAXNAMELEN] trace ring buffer field). On failure, the error path overwrites the buffer with the string literal "(unknown)" via strncpy(). The original strncpy() zero-pads the remaining 246 bytes (MAXNAMELEN is 256, "(unknown)" is 10 bytes including NUL).

Quoted 8 lines

strscpy_pad() preserves this zero-padding, which matters because the destination is a trace ring buffer entry: ring buffer slots are not zeroed on allocation, and the raw buffer is readable by userspace via tracefs. The zero-padding ensures no stale data remains in the buffer after the error path overwrites it.
The source is a 10-byte string literal into a 256-byte destination, so there is no behavioral change.

Hmm, seems to be a lot of waste. Note, the ring buffer now zeros out its data on allocation so there's no need to zero pad anything, at least not for exposing data. You may expose a previous trace.

Also, why are these using full path size and not just use dynamic array. That would be much more efficient in both performance and space.

-- Steve

Quoted 36 lines

Link https://github.com/KSPP/linux/issues/90 [1]

Signed-off-by Kees Cook <[email protected]>

---
 fs/xfs/scrub/trace.h | 3 +--
 fs/xfs/xfs_trace.h   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

fs/xfs/scrub/trace.h +1 -2

index 39ea651cbb75..46c420f51129 100644
@@ -980,8 +980,7 @@ TRACE_EVENT(xfile_create,
 		__entry->ino = file_inode(xf->file)->i_ino;
 		path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("xfino 0x%lx path '%s'",
 		  __entry->ino,

fs/xfs/xfs_trace.h +1 -2

index 813e5a9f57eb..9f9fb86097ed 100644
@@ -5101,8 +5101,7 @@ TRACE_EVENT(xmbuf_create,
 		__entry->ino = file_inode(file)->i_ino;
 		path = file_path(file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("dev %d:%d xmino 0x%lx path '%s'",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),

Darrick J. Wong

2026-03-23 20:56 UTC #3 lore raw

Quoted 22 lines · Steven Rostedt

Quoted 10 lines · On Mon, 23 Mar 2026 10:22:09 -0700 Kees Cook <[email protected]> wrote:
Replace the deprecated[1] strncpy() with strscpy_pad() in the xfile_create and xmbuf_create tracepoints.
Both tracepoints use file_path() to resolve a pathname into __entry->pathname (a char[MAXNAMELEN] trace ring buffer field). On failure, the error path overwrites the buffer with the string literal "(unknown)" via strncpy(). The original strncpy() zero-pads the remaining 246 bytes (MAXNAMELEN is 256, "(unknown)" is 10 bytes including NUL).
Quoted 8 lines
strscpy_pad() preserves this zero-padding, which matters because the destination is a trace ring buffer entry: ring buffer slots are not zeroed on allocation, and the raw buffer is readable by userspace via tracefs. The zero-padding ensures no stale data remains in the buffer after the error path overwrites it.
The source is a 10-byte string literal into a 256-byte destination, so there is no behavioral change.

Quoted 6 lines

Hmm, seems to be a lot of waste. Note, the ring buffer now zeros out its data on allocation so there's no need to zero pad anything, at least not for exposing data. You may expose a previous trace.
Also, why are these using full path size and not just use dynamic array. That would be much more efficient in both performance and space.

"Darrick was too stupid to figure that out when he wrote it."

Initially the shmem files used by xfile/xmbuf provided a longer description of exactly what the file was being used for (e.g. "AG 43 rmapbt") but the dynamic string memory allocations were annoying to have to catch since some of them exceeded 16 bytes (or whatever the "guaranteed not to fail" limits arenow) and then I was snowed under by a bunch of static checker people and so I gave up and ripped all that out.

So at this point the description is hardly useful and maybe I'll just submit a patch to rip out all the file_path tracepoint stuff.

--D

Quoted 41 lines

-- Steve

Quoted 36 lines

Link https://github.com/KSPP/linux/issues/90 [1]

Signed-off-by Kees Cook <[email protected]>

---
 fs/xfs/scrub/trace.h | 3 +--
 fs/xfs/xfs_trace.h   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

fs/xfs/scrub/trace.h +1 -2

index 39ea651cbb75..46c420f51129 100644
@@ -980,8 +980,7 @@ TRACE_EVENT(xfile_create,
 		__entry->ino = file_inode(xf->file)->i_ino;
 		path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("xfino 0x%lx path '%s'",
 		  __entry->ino,

fs/xfs/xfs_trace.h +1 -2

index 813e5a9f57eb..9f9fb86097ed 100644
@@ -5101,8 +5101,7 @@ TRACE_EVENT(xmbuf_create,
 		__entry->ino = file_inode(file)->i_ino;
 		path = file_path(file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("dev %d:%d xmino 0x%lx path '%s'",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),

David Laight

2026-03-23 20:19 UTC #4 lore raw

In reply to Kees Cook

Quoted 10 lines · On Mon, 23 Mar 2026 10:22:09 -0700 Kees Cook <[email protected]> wrote:

Replace the deprecated[1] strncpy() with strscpy_pad() in the xfile_create and xmbuf_create tracepoints.
Both tracepoints use file_path() to resolve a pathname into __entry->pathname (a char[MAXNAMELEN] trace ring buffer field). On failure, the error path overwrites the buffer with the string literal "(unknown)" via strncpy(). The original strncpy() zero-pads the remaining 246 bytes (MAXNAMELEN is 256, "(unknown)" is 10 bytes including NUL).

Quoted 5 lines

strscpy_pad() preserves this zero-padding, which matters because the destination is a trace ring buffer entry: ring buffer slots are not zeroed on allocation, and the raw buffer is readable by userspace via tracefs. The zero-padding ensures no stale data remains in the buffer after the error path overwrites it.

Eh?
AFAICT file_path() doesn't zero pad on success.
Not only that is calls d_path() to do the work and that has the comment:

* Returns a pointer into the buffer or an error code if the path was
* too long. Note: Callers should use the returned pointer, not the passed
* in buffer, to use the name! The implementation often starts at an offset
* into the buffer, and may leave 0 bytes at the start.

So the code actually looks entirely broken.

David

Quoted 39 lines

The source is a 10-byte string literal into a 256-byte destination, so there is no behavioral change.

Link https://github.com/KSPP/linux/issues/90 [1]

Signed-off-by Kees Cook <[email protected]>

---
 fs/xfs/scrub/trace.h | 3 +--
 fs/xfs/xfs_trace.h   | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

fs/xfs/scrub/trace.h +1 -2

index 39ea651cbb75..46c420f51129 100644
@@ -980,8 +980,7 @@ TRACE_EVENT(xfile_create,
 		__entry->ino = file_inode(xf->file)->i_ino;
 		path = file_path(xf->file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("xfino 0x%lx path '%s'",
 		  __entry->ino,

fs/xfs/xfs_trace.h +1 -2

index 813e5a9f57eb..9f9fb86097ed 100644
@@ -5101,8 +5101,7 @@ TRACE_EVENT(xmbuf_create,
 		__entry->ino = file_inode(file)->i_ino;
 		path = file_path(file, __entry->pathname, MAXNAMELEN);
 		if (IS_ERR(path))
-			strncpy(__entry->pathname, "(unknown)",
-					sizeof(__entry->pathname));
+			strscpy_pad(__entry->pathname, "(unknown)");
 	),
 	TP_printk("dev %d:%d xmino 0x%lx path '%s'",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),