Received-SPF: pass (google.com: domain of linux-kernel+bounces-110198-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Fri, 22 Mar 2024 00:07:59 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Ye Bin <yebin10@huawei.com>, <mhiramat@kernel.org>,
 <mathieu.desnoyers@efficios.com>, <linux-trace-kernel@vger.kernel.org>,
 <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 1/5] tracing/probes: support '%pd' type for print
 struct dentry's name
Message-Id: <20240322000759.ec04bca3bb2afdfaef37a545@kernel.org>
In-Reply-To: <20240321101547.1f4e68ab@gandalf.local.home>
References: <20240320132924.2802187-1-yebin10@huawei.com>
	<20240320132924.2802187-2-yebin10@huawei.com>
	<20240321101547.1f4e68ab@gandalf.local.home>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Thu, 21 Mar 2024 10:15:47 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 20 Mar 2024 21:29:20 +0800
> Ye Bin <yebin10@huawei.com> wrote:
> 
> > Support print type '%pd' for print dentry's  name.
> > 
> 
> The above is not a very detailed change log. A change log should state not
> only what the change is doing, but also why.
> 
> Having examples of before and after would be useful in the change log.

Hm, OK. Ye, something like this.
-----
Support print type '%pd' for print dentry's name. For example "name=$arg1:%pd"
casts the `$arg1` as (struct dentry *), dereferences the "d_name.name" field
and stores it to "name" argument as a kernel string. Here is an example;

echo 'p:testprobe dput name=$arg1:%pd' > kprobe_events 
echo 1 > events/kprobes/testprobe/enable 
cat trace
..
              sh-132     [004] .....   333.333051: testprobe: (dput+0x4/0x20) name="enable"


Note that this expects the given argument (e.g. $arg1) is an address of struct
dentry. User must ensure it.
-----

And add similar changelog on [2/5]. 

Thank you,

> 
> > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > ---
> >  kernel/trace/trace.c        |  2 +-
> >  kernel/trace/trace_fprobe.c |  6 +++++
> >  kernel/trace/trace_kprobe.c |  6 +++++
> >  kernel/trace/trace_probe.c  | 50 +++++++++++++++++++++++++++++++++++++
> >  kernel/trace/trace_probe.h  |  2 ++
> >  5 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> > index b12f8384a36a..ac26b8446337 100644
> > --- a/kernel/trace/trace.c
> > +++ b/kernel/trace/trace.c
> 
> 
> > +/* @buf: *buf must be equal to NULL. Caller must to free *buf */
> > +int traceprobe_expand_dentry_args(int argc, const char *argv[], char **buf)
> > +{
> > +	int i, used, ret;
> > +	const int bufsize = MAX_DENTRY_ARGS_LEN;
> > +	char *tmpbuf = NULL;
> > +
> > +	if (*buf)
> > +		return -EINVAL;
> > +
> > +	used = 0;
> > +	for (i = 0; i < argc; i++) {
> > +		if (glob_match("*:%pd", argv[i])) {
> > +			char *tmp;
> > +			char *equal;
> > +
> > +			if (!tmpbuf) {
> > +				tmpbuf = kmalloc(bufsize, GFP_KERNEL);
> > +				if (!tmpbuf)
> > +					return -ENOMEM;
> > +			}
> > +
> > +			tmp = kstrdup(argv[i], GFP_KERNEL);
> > +			if (!tmp)
> > +				goto nomem;
> > +
> > +			equal = strchr(tmp, '=');
> > +			if (equal)
> > +				*equal = '\0';
> > +			tmp[strlen(argv[i]) - 4] = '\0';
> > +			ret = snprintf(tmpbuf + used, bufsize - used,
> > +				       "%s%s+0x0(+0x%zx(%s)):string",
> > +				       equal ? tmp : "", equal ? "=" : "",
> > +				       offsetof(struct dentry, d_name.name),
> > +				       equal ? equal + 1 : tmp);
> 
> What would be really useful is if we had a way to expose BTF here. Something like:
> 
>  "%pB:<struct>:<field>"
> 
> The "%pB" would mean to look up the struct/field offsets and types via BTF,
> and create the appropriate command to find and print it.

Would you mean casing the pointer to "<struct>"?


> 
> That would be much more flexible and useful as it would allow reading any
> field from any structure without having to use gdb.
> 
> -- Steve
> 
> 
> > +			kfree(tmp);
> > +			if (ret >= bufsize - used)
> > +				goto nomem;
> > +			argv[i] = tmpbuf + used;
> > +			used += ret + 1;
> > +		}
> > +	}
> > +
> > +	*buf = tmpbuf;
> > +	return 0;
> > +nomem:
> > +	kfree(tmpbuf);
> > +	return -ENOMEM;
> > +}
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>