All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Thomas Richter" <tmricht@linux.ibm.com>
Cc: linux-s390@vger.kernel.org,
	"Christian Borntraeger" <borntraeger@linux.ibm.com>,
	"Alexander Gordeev" <agordeev@linux.ibm.com>,
	"Vasily Gorbik" <gor@linux.ibm.com>,
	"Heiko Carstens" <hca@linux.ibm.com>
Subject: Re: [PATCH ping2 linux-next] perf test: probe_vfs_getname add do_getname() probing fallback
Date: Fri, 11 Sep 2026 10:12:21 +0000	[thread overview]
Message-ID: <20260911101221.A4BE01F000FF@smtp.kernel.org> (raw)
In-Reply-To: <20260911100455.2131629-1-tmricht@linux.ibm.com>

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The fallback probe for `do_getname` incorrectly uses `result->iname:string` instead of `result->name:string`, leading to string truncation or out-of-bounds reads for long filenames.
- [Medium] The new `do_getname` probe logic omits the fallback to reading the user-space pointer (`filename:ustring`) if the `result` struct is optimized out by the compiler, reducing test robustness.
--

commit ad303146f8b0a504f7736915a5ea288cabae2ca8
Author: Thomas Richter <tmricht@linux.ibm.com>

perf test: probe_vfs_getname add do_getname() probing fallback

This commit extends the vfs_getname perf test to fall back to probing
do_getname() if getname_flags() cannot be probed. This accounts for
recent kernel changes that altered the getname_flags() implementation.

> diff --git a/tools/perf/tests/shell/lib/probe_vfs_getname.sh b/tools/perf/tests/shell/lib/probe_vfs_getname.sh
> index 88cd0e26d5f67..9156a79212b9e 100644
> --- a/tools/perf/tests/shell/lib/probe_vfs_getname.sh
> +++ b/tools/perf/tests/shell/lib/probe_vfs_getname.sh

[ ... ]

> @@ -28,12 +30,24 @@ add_probe_vfs_getname() {
>  			line=$(perf probe -L getname_flags 2>&1 | grep -E "$result_aname_re" | sed -r "s/$result_aname_re/\1/")
>  		fi
>  
> +		if [ -z "$line" ] ; then
> +			do_getname=1
> +			result_iname_re="[[:space:]]+([[:digit:]]+)[[:space:]]+initname\(result\);"
> +			line=$(perf probe -L do_getname 2>&1 | grep -E "$result_iname_re" | sed -r "s/$result_iname_re/\1/")
> +		fi
> +
>  		if [ -z "$line" ] ; then
>  			echo "Could not find probeable line"
>  			return 2
>  		fi
>  
> -		perf probe -q       "vfs_getname=getname_flags:${line} pathname=result->name:string" || \
> +		if [ "$do_getname" -eq 1 ]
> +		then
> +			param="vfs_getname=do_getname:${line} pathname=result->iname:string"

[Severity: Medium]
Will tracing result->iname as a string cause out-of-bounds reads or
truncated output for long filenames?

Looking at getname_long() in fs/namei.c, when a filename >=
EMBEDDED_NAME_MAX is processed, a separate buffer is allocated for
name->name:

	memcpy(p, &name->iname, EMBEDDED_NAME_MAX);
	len = strncpy_from_user(p + EMBEDDED_NAME_MAX,
				filename + EMBEDDED_NAME_MAX,
				PATH_MAX - EMBEDDED_NAME_MAX);
	if (unlikely(len < 0))
		return len;
	if (unlikely(len == PATH_MAX - EMBEDDED_NAME_MAX))
		return -ENAMETOOLONG;
	name->name = no_free_ptr(p);

This leaves name->iname completely filled without a null terminator. Should
this probe use pathname=result->name:string instead to safely handle both
short and long filenames?

> +			perf probe -q "$param" || perf probe $add_probe_verbose "$param" || return 1

[Severity: Medium]
Does this fallback logic miss the user-space pointer retry?

The original code for getname_flags falls back to tracing the user-space
pointer (filename:ustring) if probing the result struct fails (which can
happen if the compiler optimizes the struct out).

In this new do_getname block, if the first probe fails, it retries the
exact same parameter with $add_probe_verbose rather than falling back to
pathname=filename:ustring. Can this cause spurious test failures on
environments where debug info for the result pointer is optimized out?

> +			return 0
> +		fi
> +		perf probe -q "vfs_getname=getname_flags:${line} pathname=result->name:string" || \
>  		perf probe $add_probe_verbose "vfs_getname=getname_flags:${line} pathname=filename:ustring" || return 1
>  	fi
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911100455.2131629-1-tmricht@linux.ibm.com?part=1

  reply	other threads:[~2026-09-11 10:12 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-11 10:04 [PATCH ping2 linux-next] perf test: probe_vfs_getname add do_getname() probing fallback Thomas Richter
2026-09-11 10:12 ` sashiko-bot [this message]
  -- strict thread matches above, loose matches on Subject: below --
2026-05-19 11:06 Thomas Richter
2026-05-19 11:25 ` sashiko-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260911101221.A4BE01F000FF@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=agordeev@linux.ibm.com \
    --cc=borntraeger@linux.ibm.com \
    --cc=gor@linux.ibm.com \
    --cc=hca@linux.ibm.com \
    --cc=linux-s390@vger.kernel.org \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=tmricht@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.