| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAErzpmupfPU9AW-76Jw7qNc1McPOuC2d1vcJEyQBOxHNEt76sg@mail.gmail.com>
Date: Tue, 9 Dec 2025 11:14:43 +0800
From: Donglin Peng <dolinux.peng@...il.com>
To: Masami Hiramatsu <mhiramat@...nel.org>
Cc: rostedt@...dmis.org, linux-trace-kernel@...r.kernel.org,
linux-kernel@...r.kernel.org, pengdonglin <pengdonglin@...omi.com>,
Xiaoqin Zhang <zhangxiaoqin@...omi.com>
Subject: Re: [PATCH v2 1/2] fgraph: Use BTF to trim and filter return values
On Tue, Dec 9, 2025 at 9:42 AM Donglin Peng <dolinux.peng@...il.com> wrote:
>
> On Tue, Dec 9, 2025 at 8:48 AM Masami Hiramatsu <mhiramat@...nel.org> wrote:
> >
> > On Mon, 8 Dec 2025 21:19:16 +0800
> > Donglin Peng <dolinux.peng@...il.com> wrote:
> >
> > > From: pengdonglin <pengdonglin@...omi.com>
> > >
> > > The current funcgraph-retval implementation has two limitations:
> > >
> > > 1. It prints a return value even when the traced function returns void.
> > > 2. When the return type is narrower than a register, the printed value may
> > > be incorrect because high bits can contain undefined data.
> > >
> > > Both issues are addressed by using BTF to obtain the precise return type
> > > of each traced function:
> > >
> > > - Return values are now printed only for functions whose return type is
> > > not void.
> > > - The value is truncated to the actual width of the return type, ensuring
> > > correct representation.
> > >
> > > These changes make the funcgraph-retval output more accurate and remove
> > > noise from void functions.
> > >
> > > Cc: Steven Rostedt (Google) <rostedt@...dmis.org>
> > > Cc: Masami Hiramatsu <mhiramat@...nel.org>
> > > Cc: Xiaoqin Zhang <zhangxiaoqin@...omi.com>
> > > Signed-off-by: pengdonglin <pengdonglin@...omi.com>
> > > ---
> > > kernel/trace/trace_functions_graph.c | 64 +++++++++++++++++++++++-----
> > > 1 file changed, 54 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
> > > index 17c75cf2348e..9e63665c81e2 100644
> > > --- a/kernel/trace/trace_functions_graph.c
> > > +++ b/kernel/trace/trace_functions_graph.c
> > > @@ -15,6 +15,7 @@
> > >
> > > #include "trace.h"
> > > #include "trace_output.h"
> > > +#include "trace_btf.h"
> > >
> > > /* When set, irq functions might be ignored */
> > > static int ftrace_graph_skip_irqs;
> > > @@ -865,6 +866,46 @@ static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_e
> > >
> > > #if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
> > >
> > > +static void trim_retval(unsigned long func, unsigned long *retval, bool *print_retval)
> > > +{
> > > + const struct btf_type *t;
> > > + char name[KSYM_NAME_LEN];
> > > + struct btf *btf;
> > > + u32 v, msb;
> > > +
> > > + if (!IS_ENABLED(CONFIG_DEBUG_INFO_BTF))
> > > + return;
> > > +
> > > + if (lookup_symbol_name(func, name))
> > > + return;
> > > +
> > > + t = btf_find_func_proto(name, &btf);
> > > + if (IS_ERR_OR_NULL(t))
> > > + return;
> > > +
> > > + t = btf_type_skip_modifiers(btf, t->type, NULL);
> > > + switch (t ? BTF_INFO_KIND(t->info) : BTF_KIND_UNKN) {
> > > + case BTF_KIND_UNKN:
> > > + *print_retval = false;
> > > + break;
> > > + case BTF_KIND_ENUM:
> > > + case BTF_KIND_ENUM64:
> > > + msb = BITS_PER_BYTE * t->size - 1;
> > > + *retval &= GENMASK(msb, 0);
> > > + break;
> > > + case BTF_KIND_INT:
> > > + v = *(u32 *)(t + 1);
> > > + if (BTF_INT_ENCODING(v) == BTF_INT_BOOL)
> > > + msb = 0;
> > > + else
> > > + msb = BTF_INT_BITS(v) - 1;
> > > + *retval &= GENMASK(msb, 0);
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +}
> >
> > Hmm, I think we should have another flag which directly tells the print
> > format of retval from BTF.
>
> Thanks, I thought about it and the format flag could have the following values:
>
> 1. HEX - hexadecimal (%lx for pointers/unsigned)
> 2. DEC - decimal (%ld for signed/error_code)
> 3. BOOL - boolean strings ("true"/"false")
>
> WDYT?
>
> Thanks,
> Donglin
> >
> > > +
> > > static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entry *entry,
> > > struct ftrace_graph_ret *graph_ret, void *func,
> > > u32 opt_flags, u32 trace_flags, int args_size)
> > > @@ -884,17 +925,20 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
> > > print_retaddr = !!(opt_flags & TRACE_GRAPH_PRINT_RETADDR);
> > > #endif
> > >
> > > - if (print_retval && retval && !hex_format) {
> > > - /* Check if the return value matches the negative format */
> > > - if (IS_ENABLED(CONFIG_64BIT) && (retval & BIT(31)) &&
> > > - (((u64)retval) >> 32) == 0) {
> > > - err_code = sign_extend64(retval, 31);
> > > - } else {
> > > - err_code = retval;
> >
> > The original code just guesses the type (signed/unsigned) from
> > the retval, but we don't need to do that with BTF.
Sorry, I think the above guessing process is necessary to display an error
code. In most cases, the return type is int, but retval is of size
sizeof(unsigned long).
We should perform sign extension to check whether it represents an error code.
Thanks,
Donglin
>
> Thanks. I will fix it in the next version.
>
> Thanks,
> Donglin
> >
> > Thank you,
> >
> > > + if (print_retval) {
> > > + trim_retval((unsigned long)func, &retval, &print_retval);
> > > + if (print_retval && retval && !hex_format) {
> > > + /* Check if the return value matches the negative format */
> > > + if (IS_ENABLED(CONFIG_64BIT) && (retval & BIT(31)) &&
> > > + (((u64)retval) >> 32) == 0) {
> > > + err_code = sign_extend64(retval, 31);
> > > + } else {
> > > + err_code = retval;
> > > + }
> > > +
> > > + if (!IS_ERR_VALUE(err_code))
> > > + err_code = 0;
> > > }
> > > -
> > > - if (!IS_ERR_VALUE(err_code))
> > > - err_code = 0;
> > > }
> > >
> > > if (entry) {
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@...nel.org>
Powered by blists - more mailing lists