| Message ID | 20230727114309.3739-2-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Fix fill_link_info and add selftest | expand |
On Thu, Jul 27, 2023 at 11:43:08AM +0000, Yafang Shao wrote: > The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for > perf_event" from Jul 9, 2023, leads to the following Smatch static > checker warning: > > kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe() > error: uninitialized symbol 'type'. > > That can happens when uname is NULL. So fix it by verifying the uname > when we really need to fill it. > > Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event") > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/ > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > --- > kernel/bpf/syscall.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 7f4e8c3..ad9360d 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event, > size_t len; > int err; > > - if (!ulen ^ !uname) > - return -EINVAL; would it make more sense to keep above check in place and move the !uname change below? I'd think we want to return error in case of wrong arguments as soon as possible > - if (!uname) > - return 0; > - > err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf, > probe_offset, probe_addr); > if (err) > return err; > > + if (!ulen ^ !uname) > + return -EINVAL; > + if (!uname) > + return 0; and here we just return 0 if we do not store the name to provided buffer thanks, jirka > + > if (buf) { > len = strlen(buf); > err = bpf_copy_to_user(uname, buf, ulen, len); > -- > 1.8.3.1 >
On Thu, Jul 27, 2023 at 9:07 PM Jiri Olsa <olsajiri@gmail.com> wrote: > > On Thu, Jul 27, 2023 at 11:43:08AM +0000, Yafang Shao wrote: > > The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for > > perf_event" from Jul 9, 2023, leads to the following Smatch static > > checker warning: > > > > kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe() > > error: uninitialized symbol 'type'. > > > > That can happens when uname is NULL. So fix it by verifying the uname > > when we really need to fill it. > > > > Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event") > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/ > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > kernel/bpf/syscall.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > > index 7f4e8c3..ad9360d 100644 > > --- a/kernel/bpf/syscall.c > > +++ b/kernel/bpf/syscall.c > > @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event, > > size_t len; > > int err; > > > > - if (!ulen ^ !uname) > > - return -EINVAL; > > would it make more sense to keep above check in place and move the > !uname change below? I'd think we want to return error in case of > wrong arguments as soon as possible Good point. Will change it. > > > - if (!uname) > > - return 0; > > - > > err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf, > > probe_offset, probe_addr); > > if (err) > > return err; > > > > + if (!ulen ^ !uname) > > + return -EINVAL; > > + if (!uname) > > + return 0; > > and here we just return 0 if we do not store the name to provided buffer Will do it.
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 7f4e8c3..ad9360d 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -3376,16 +3376,16 @@ static int bpf_perf_link_fill_common(const struct perf_event *event, size_t len; int err; - if (!ulen ^ !uname) - return -EINVAL; - if (!uname) - return 0; - err = bpf_get_perf_event_info(event, &prog_id, fd_type, &buf, probe_offset, probe_addr); if (err) return err; + if (!ulen ^ !uname) + return -EINVAL; + if (!uname) + return 0; + if (buf) { len = strlen(buf); err = bpf_copy_to_user(uname, buf, ulen, len);
The patch 1b715e1b0ec5: "bpf: Support ->fill_link_info for perf_event" from Jul 9, 2023, leads to the following Smatch static checker warning: kernel/bpf/syscall.c:3416 bpf_perf_link_fill_kprobe() error: uninitialized symbol 'type'. That can happens when uname is NULL. So fix it by verifying the uname when we really need to fill it. Fixes: 1b715e1b0ec5 ("bpf: Support ->fill_link_info for perf_event") Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/bpf/85697a7e-f897-4f74-8b43-82721bebc462@kili.mountain/ Signed-off-by: Yafang Shao <laoar.shao@gmail.com> --- kernel/bpf/syscall.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)