Re: [PATCH] xfs: Replace strncpy() with strscpy_pad() in tracepoint error paths

["Darrick J. Wong" <djwong@kernel.org>]

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),
> 
>