Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
On Tue, 19 Mar 2024 10:10:00 -0400 Steven Rostedt wrote: > On Tue, 19 Mar 2024 10:19:09 +0300 > Dan Carpenter 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)
Re: [bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
On Tue, Mar 19, 2024 at 10:10:00AM -0400, Steven Rostedt wrote: > On Tue, 19 Mar 2024 10:19:09 +0300 > Dan Carpenter 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)
On Tue, 19 Mar 2024 10:19:09 +0300 Dan Carpenter 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
[bug report] tracing/probes: Support $argN in return probe (kprobe and fprobe)
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