[bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)

[bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)

[Dan Carpenter]

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

Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)

[Steven Rostedt]

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

Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)

[Dan Carpenter]

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

Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)

[Masami Hiramatsu]

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>

[PATCH] tracing: probes: Fix to zero initialize a local variable

[Masami Hiramatsu (Google)]

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)

Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)

[Steven Rostedt]

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

Re: [PATCH] tracing: probes: Fix to zero initialize a local variable

[Steven Rostedt]

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)

Re: [PATCH] tracing: probes: Fix to zero initialize a local variable

[Steven Rostedt]

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

Re: [PATCH] tracing: probes: Fix to zero initialize a local variable

[Masami Hiramatsu]

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>