* [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
@ 2024-03-19 7:19 Dan Carpenter
2024-03-19 14:10 ` Steven Rostedt
2024-03-20 8:10 ` [PATCH] tracing: probes: Fix to zero initialize a local variable Masami Hiramatsu (Google)
0 siblings, 2 replies; 9+ messages in thread
From: Dan Carpenter @ 2024-03-19 7:19 UTC (permalink / raw)
To: mhiramat; +Cc: linux-trace-kernel
Hello Masami Hiramatsu (Google),
Commit 25f00e40ce79 ("tracing/probes: Support $argN in return probe
(kprobe and fprobe)") from Mar 4, 2024 (linux-next), leads to the
following Smatch static checker warning:
kernel/trace/trace_probe.c:856 store_trace_entry_data()
error: uninitialized symbol 'val'.
kernel/trace/trace_probe.c
846 return;
847
848 for (i = 0; i < earg->size; i++) {
849 struct fetch_insn *code = &earg->code[i];
850
851 switch (code->op) {
852 case FETCH_OP_ARG:
853 val = regs_get_kernel_argument(regs, code->param);
854 break;
855 case FETCH_OP_ST_EDATA:
--> 856 *(unsigned long *)((unsigned long)edata + code->offset) = val;
Probably the earg->code[i] always has FETCH_OP_ARG before
FETCH_OP_ST_EDATA but Smatch isn't smart enough to figure that out...
857 break;
858 case FETCH_OP_END:
859 goto end;
860 default:
861 break;
862 }
863 }
864 end:
865 return;
866 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
2024-03-19 7:19 [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe) Dan Carpenter
@ 2024-03-19 14:10 ` Steven Rostedt
2024-03-19 14:39 ` Dan Carpenter
2024-03-20 3:44 ` Masami Hiramatsu
2024-03-20 8:10 ` [PATCH] tracing: probes: Fix to zero initialize a local variable Masami Hiramatsu (Google)
1 sibling, 2 replies; 9+ messages in thread
From: Steven Rostedt @ 2024-03-19 14:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: mhiramat, linux-trace-kernel
On Tue, 19 Mar 2024 10:19:09 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:
> Hello Masami Hiramatsu (Google),
>
> Commit 25f00e40ce79 ("tracing/probes: Support $argN in return probe
> (kprobe and fprobe)") from Mar 4, 2024 (linux-next), leads to the
> following Smatch static checker warning:
>
> kernel/trace/trace_probe.c:856 store_trace_entry_data()
> error: uninitialized symbol 'val'.
>
> kernel/trace/trace_probe.c
> 846 return;
> 847
> 848 for (i = 0; i < earg->size; i++) {
> 849 struct fetch_insn *code = &earg->code[i];
> 850
> 851 switch (code->op) {
> 852 case FETCH_OP_ARG:
> 853 val = regs_get_kernel_argument(regs, code->param);
> 854 break;
> 855 case FETCH_OP_ST_EDATA:
> --> 856 *(unsigned long *)((unsigned long)edata + code->offset) = val;
>
> Probably the earg->code[i] always has FETCH_OP_ARG before
> FETCH_OP_ST_EDATA but Smatch isn't smart enough to figure that out...
Looks that way:
case FETCH_OP_END:
earg->code[i].op = FETCH_OP_ARG;
earg->code[i].param = argnum;
earg->code[i + 1].op = FETCH_OP_ST_EDATA;
earg->code[i + 1].offset = offset;
return offset;
But probably should still initialize val to zero or have a WARN_ON() if
that doesn't happen.
-- Steve
>
> 857 break;
> 858 case FETCH_OP_END:
> 859 goto end;
> 860 default:
> 861 break;
> 862 }
> 863 }
> 864 end:
> 865 return;
> 866 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
2024-03-19 14:10 ` Steven Rostedt
@ 2024-03-19 14:39 ` Dan Carpenter
2024-03-20 3:44 ` Masami Hiramatsu
1 sibling, 0 replies; 9+ messages in thread
From: Dan Carpenter @ 2024-03-19 14:39 UTC (permalink / raw)
To: Steven Rostedt; +Cc: mhiramat, linux-trace-kernel
On Tue, Mar 19, 2024 at 10:10:00AM -0400, Steven Rostedt wrote:
> On Tue, 19 Mar 2024 10:19:09 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> > Hello Masami Hiramatsu (Google),
> >
> > Commit 25f00e40ce79 ("tracing/probes: Support $argN in return probe
> > (kprobe and fprobe)") from Mar 4, 2024 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > kernel/trace/trace_probe.c:856 store_trace_entry_data()
> > error: uninitialized symbol 'val'.
> >
> > kernel/trace/trace_probe.c
> > 846 return;
> > 847
> > 848 for (i = 0; i < earg->size; i++) {
> > 849 struct fetch_insn *code = &earg->code[i];
> > 850
> > 851 switch (code->op) {
> > 852 case FETCH_OP_ARG:
> > 853 val = regs_get_kernel_argument(regs, code->param);
> > 854 break;
> > 855 case FETCH_OP_ST_EDATA:
> > --> 856 *(unsigned long *)((unsigned long)edata + code->offset) = val;
> >
> > Probably the earg->code[i] always has FETCH_OP_ARG before
> > FETCH_OP_ST_EDATA but Smatch isn't smart enough to figure that out...
>
> Looks that way:
>
> case FETCH_OP_END:
> earg->code[i].op = FETCH_OP_ARG;
> earg->code[i].param = argnum;
> earg->code[i + 1].op = FETCH_OP_ST_EDATA;
> earg->code[i + 1].offset = offset;
> return offset;
>
> But probably should still initialize val to zero or have a WARN_ON() if
> that doesn't happen.
>
Most people use the GCC extension to initialize everything to zero so
initializing to zero really has zero cost. I always recomend people to
do it.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
2024-03-19 14:10 ` Steven Rostedt
2024-03-19 14:39 ` Dan Carpenter
@ 2024-03-20 3:44 ` Masami Hiramatsu
2024-03-20 13:22 ` Steven Rostedt
1 sibling, 1 reply; 9+ messages in thread
From: Masami Hiramatsu @ 2024-03-20 3:44 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Dan Carpenter, mhiramat, linux-trace-kernel
On Tue, 19 Mar 2024 10:10:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 19 Mar 2024 10:19:09 +0300
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> > Hello Masami Hiramatsu (Google),
> >
> > Commit 25f00e40ce79 ("tracing/probes: Support $argN in return probe
> > (kprobe and fprobe)") from Mar 4, 2024 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > kernel/trace/trace_probe.c:856 store_trace_entry_data()
> > error: uninitialized symbol 'val'.
> >
> > kernel/trace/trace_probe.c
> > 846 return;
> > 847
> > 848 for (i = 0; i < earg->size; i++) {
> > 849 struct fetch_insn *code = &earg->code[i];
> > 850
> > 851 switch (code->op) {
> > 852 case FETCH_OP_ARG:
> > 853 val = regs_get_kernel_argument(regs, code->param);
> > 854 break;
> > 855 case FETCH_OP_ST_EDATA:
> > --> 856 *(unsigned long *)((unsigned long)edata + code->offset) = val;
> >
> > Probably the earg->code[i] always has FETCH_OP_ARG before
> > FETCH_OP_ST_EDATA but Smatch isn't smart enough to figure that out...
>
> Looks that way:
>
> case FETCH_OP_END:
> earg->code[i].op = FETCH_OP_ARG;
> earg->code[i].param = argnum;
> earg->code[i + 1].op = FETCH_OP_ST_EDATA;
> earg->code[i + 1].offset = offset;
> return offset;
>
> But probably should still initialize val to zero or have a WARN_ON() if
> that doesn't happen.
OK, let's val = 0 in the store_trace_entry_data(), but WARN_ON() in this loop
is a bit strange. I think we should have a verifiler.
Thank you,
>
> -- Steve
>
>
> >
> > 857 break;
> > 858 case FETCH_OP_END:
> > 859 goto end;
> > 860 default:
> > 861 break;
> > 862 }
> > 863 }
> > 864 end:
> > 865 return;
> > 866 }
> >
> > regards,
> > dan carpenter
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
2024-03-20 3:44 ` Masami Hiramatsu
@ 2024-03-20 13:22 ` Steven Rostedt
0 siblings, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2024-03-20 13:22 UTC (permalink / raw)
To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel
On Wed, 20 Mar 2024 12:44:23 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:
> > > kernel/trace/trace_probe.c
> > > 846 return;
> > > 847
> > > 848 for (i = 0; i < earg->size; i++) {
> > > 849 struct fetch_insn *code = &earg->code[i];
> > > 850
> > > 851 switch (code->op) {
> > > 852 case FETCH_OP_ARG:
> > > 853 val = regs_get_kernel_argument(regs, code->param);
> > > 854 break;
> > > 855 case FETCH_OP_ST_EDATA:
> > > --> 856 *(unsigned long *)((unsigned long)edata + code->offset) = val;
> > >
> > > Probably the earg->code[i] always has FETCH_OP_ARG before
> > > FETCH_OP_ST_EDATA but Smatch isn't smart enough to figure that out...
> >
> > Looks that way:
> >
> > case FETCH_OP_END:
> > earg->code[i].op = FETCH_OP_ARG;
> > earg->code[i].param = argnum;
> > earg->code[i + 1].op = FETCH_OP_ST_EDATA;
> > earg->code[i + 1].offset = offset;
> > return offset;
> >
> > But probably should still initialize val to zero or have a WARN_ON() if
> > that doesn't happen.
>
> OK, let's val = 0 in the store_trace_entry_data(), but WARN_ON() in this loop
> is a bit strange. I think we should have a verifiler.
Initializing to zero is fine.
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] tracing: probes: Fix to zero initialize a local variable
2024-03-19 7:19 [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe) Dan Carpenter
2024-03-19 14:10 ` Steven Rostedt
@ 2024-03-20 8:10 ` Masami Hiramatsu (Google)
2024-03-20 13:26 ` Steven Rostedt
2024-03-20 13:27 ` Steven Rostedt
1 sibling, 2 replies; 9+ messages in thread
From: Masami Hiramatsu (Google) @ 2024-03-20 8:10 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Dan Carpenter, mhiramat, linux-trace-kernel
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Fix to initialize 'val' local variable with zero.
Dan reported that Smatch static code checker reports an error that a local
'val' variable needs to be initialized. Actually, the 'val' is expected to
be initialized by FETCH_OP_ARG in the same loop, but it is not obvious. So
initialize it with zero.
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/all/b010488e-68aa-407c-add0-3e059254aaa0@moroto.mountain/
Fixes: 25f00e40ce79 ("tracing/probes: Support $argN in return probe (kprobe and fprobe)")
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
kernel/trace/trace_probe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
index 217169de0920..dfe3ee6035ec 100644
--- a/kernel/trace/trace_probe.c
+++ b/kernel/trace/trace_probe.c
@@ -839,7 +839,7 @@ int traceprobe_get_entry_data_size(struct trace_probe *tp)
void store_trace_entry_data(void *edata, struct trace_probe *tp, struct pt_regs *regs)
{
struct probe_entry_arg *earg = tp->entry_arg;
- unsigned long val;
+ unsigned long val = 0;
int i;
if (!earg)
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing: probes: Fix to zero initialize a local variable
2024-03-20 8:10 ` [PATCH] tracing: probes: Fix to zero initialize a local variable Masami Hiramatsu (Google)
@ 2024-03-20 13:26 ` Steven Rostedt
2024-03-20 13:27 ` Steven Rostedt
1 sibling, 0 replies; 9+ messages in thread
From: Steven Rostedt @ 2024-03-20 13:26 UTC (permalink / raw)
To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel
On Wed, 20 Mar 2024 17:10:38 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Fix to initialize 'val' local variable with zero.
> Dan reported that Smatch static code checker reports an error that a local
> 'val' variable needs to be initialized. Actually, the 'val' is expected to
> be initialized by FETCH_OP_ARG in the same loop, but it is not obvious. So
> initialize it with zero.
>
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/all/b010488e-68aa-407c-add0-3e059254aaa0@moroto.mountain/
> Fixes: 25f00e40ce79 ("tracing/probes: Support $argN in return probe (kprobe and fprobe)")
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
> kernel/trace/trace_probe.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> index 217169de0920..dfe3ee6035ec 100644
> --- a/kernel/trace/trace_probe.c
> +++ b/kernel/trace/trace_probe.c
> @@ -839,7 +839,7 @@ int traceprobe_get_entry_data_size(struct trace_probe *tp)
> void store_trace_entry_data(void *edata, struct trace_probe *tp, struct pt_regs *regs)
> {
> struct probe_entry_arg *earg = tp->entry_arg;
> - unsigned long val;
> + unsigned long val = 0;
> int i;
>
> if (!earg)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing: probes: Fix to zero initialize a local variable
2024-03-20 8:10 ` [PATCH] tracing: probes: Fix to zero initialize a local variable Masami Hiramatsu (Google)
2024-03-20 13:26 ` Steven Rostedt
@ 2024-03-20 13:27 ` Steven Rostedt
2024-03-20 22:56 ` Masami Hiramatsu
1 sibling, 1 reply; 9+ messages in thread
From: Steven Rostedt @ 2024-03-20 13:27 UTC (permalink / raw)
To: Masami Hiramatsu (Google); +Cc: Dan Carpenter, linux-trace-kernel
On Wed, 20 Mar 2024 17:10:38 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
>
> Fix to initialize 'val' local variable with zero.
> Dan reported that Smatch static code checker reports an error that a local
> 'val' variable needs to be initialized. Actually, the 'val' is expected to
> be initialized by FETCH_OP_ARG in the same loop, but it is not obvious. So
> initialize it with zero.
BTW, that loop should really have a comment stating that FETCH_OP_ARG is
expected to happen before FETCH_OP_ST_EDATA.
-- Steve
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] tracing: probes: Fix to zero initialize a local variable
2024-03-20 13:27 ` Steven Rostedt
@ 2024-03-20 22:56 ` Masami Hiramatsu
0 siblings, 0 replies; 9+ messages in thread
From: Masami Hiramatsu @ 2024-03-20 22:56 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Dan Carpenter, linux-trace-kernel
On Wed, 20 Mar 2024 09:27:49 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 20 Mar 2024 17:10:38 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
>
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> >
> > Fix to initialize 'val' local variable with zero.
> > Dan reported that Smatch static code checker reports an error that a local
> > 'val' variable needs to be initialized. Actually, the 'val' is expected to
> > be initialized by FETCH_OP_ARG in the same loop, but it is not obvious. So
> > initialize it with zero.
>
> BTW, that loop should really have a comment stating that FETCH_OP_ARG is
> expected to happen before FETCH_OP_ST_EDATA.
Indeed, OK, let me add it.
Thank you,
>
> -- Steve
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-20 22:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-19 7:19 [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe) Dan Carpenter
2024-03-19 14:10 ` Steven Rostedt
2024-03-19 14:39 ` Dan Carpenter
2024-03-20 3:44 ` Masami Hiramatsu
2024-03-20 13:22 ` Steven Rostedt
2024-03-20 8:10 ` [PATCH] tracing: probes: Fix to zero initialize a local variable Masami Hiramatsu (Google)
2024-03-20 13:26 ` Steven Rostedt
2024-03-20 13:27 ` Steven Rostedt
2024-03-20 22:56 ` Masami Hiramatsu
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.