* Re: [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths
2026-03-23 17:22 [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths Kees Cook
@ 2026-03-23 18:24 ` Darrick J. Wong
2026-03-23 19:25 ` Steven Rostedt
2026-03-23 20:19 ` David Laight
2 siblings, 0 replies; 5+ messages in thread
From: Darrick J. Wong @ 2026-03-23 18:24 UTC (permalink / raw)
To: Kees Cook
Cc: Carlos Maiolino, Andrey Albershteyn, Steven Rostedt, linux-kernel,
linux-xfs, linux-hardening
On Mon, Mar 23, 2026 at 10:22:09AM -0700, Kees Cook 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).
>
> 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 <kees@kernel.org>
Seems fine to me,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> ---
> fs/xfs/scrub/trace.h | 3 +--
> fs/xfs/xfs_trace.h | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> index 39ea651cbb75..46c420f51129 100644
> --- a/fs/xfs/scrub/trace.h
> +++ b/fs/xfs/scrub/trace.h
> @@ -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,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 813e5a9f57eb..9f9fb86097ed 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -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),
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths
2026-03-23 17:22 [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths Kees Cook
2026-03-23 18:24 ` Darrick J. Wong
@ 2026-03-23 19:25 ` Steven Rostedt
2026-03-23 20:56 ` Darrick J. Wong
2026-03-23 20:19 ` David Laight
2 siblings, 1 reply; 5+ messages in thread
From: Steven Rostedt @ 2026-03-23 19:25 UTC (permalink / raw)
To: Kees Cook
Cc: Carlos Maiolino, Darrick J. Wong, Andrey Albershteyn,
linux-kernel, linux-xfs, linux-hardening
On Mon, 23 Mar 2026 10:22:09 -0700
Kees Cook <kees@kernel.org> 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).
>
> 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
>
> Link: https://github.com/KSPP/linux/issues/90 [1]
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> fs/xfs/scrub/trace.h | 3 +--
> fs/xfs/xfs_trace.h | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> index 39ea651cbb75..46c420f51129 100644
> --- a/fs/xfs/scrub/trace.h
> +++ b/fs/xfs/scrub/trace.h
> @@ -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,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 813e5a9f57eb..9f9fb86097ed 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -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),
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths
2026-03-23 19:25 ` Steven Rostedt
@ 2026-03-23 20:56 ` Darrick J. Wong
0 siblings, 0 replies; 5+ messages in thread
From: Darrick J. Wong @ 2026-03-23 20:56 UTC (permalink / raw)
To: Steven Rostedt
Cc: Kees Cook, Carlos Maiolino, Andrey Albershteyn, linux-kernel,
linux-xfs, linux-hardening
On Mon, Mar 23, 2026 at 03:25:14PM -0400, Steven Rostedt wrote:
> On Mon, 23 Mar 2026 10:22:09 -0700
> Kees Cook <kees@kernel.org> 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).
> >
> > 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.
"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
> -- Steve
>
>
> >
> > Link: https://github.com/KSPP/linux/issues/90 [1]
> > Signed-off-by: Kees Cook <kees@kernel.org>
> > ---
> > fs/xfs/scrub/trace.h | 3 +--
> > fs/xfs/xfs_trace.h | 3 +--
> > 2 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> > index 39ea651cbb75..46c420f51129 100644
> > --- a/fs/xfs/scrub/trace.h
> > +++ b/fs/xfs/scrub/trace.h
> > @@ -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,
> > diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> > index 813e5a9f57eb..9f9fb86097ed 100644
> > --- a/fs/xfs/xfs_trace.h
> > +++ b/fs/xfs/xfs_trace.h
> > @@ -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),
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths
2026-03-23 17:22 [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths Kees Cook
2026-03-23 18:24 ` Darrick J. Wong
2026-03-23 19:25 ` Steven Rostedt
@ 2026-03-23 20:19 ` David Laight
2 siblings, 0 replies; 5+ messages in thread
From: David Laight @ 2026-03-23 20:19 UTC (permalink / raw)
To: Kees Cook
Cc: Carlos Maiolino, Darrick J. Wong, Andrey Albershteyn,
Steven Rostedt, linux-kernel, linux-xfs, linux-hardening
On Mon, 23 Mar 2026 10:22:09 -0700
Kees Cook <kees@kernel.org> 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).
>
> 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
>
> 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 <kees@kernel.org>
> ---
> fs/xfs/scrub/trace.h | 3 +--
> fs/xfs/xfs_trace.h | 3 +--
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/scrub/trace.h b/fs/xfs/scrub/trace.h
> index 39ea651cbb75..46c420f51129 100644
> --- a/fs/xfs/scrub/trace.h
> +++ b/fs/xfs/scrub/trace.h
> @@ -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,
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 813e5a9f57eb..9f9fb86097ed 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -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),
^ permalink raw reply [flat|nested] 5+ messages in thread