| Message ID | 20201204232002.3589803-1-andrii@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [bpf] tools/bpftool: fix PID fetching with a lot of results | expand |
On 12/4/20 3:20 PM, Andrii Nakryiko wrote: > In case of having so many PID results that they don't fit into a singe page > (4096) bytes, bpftool will erroneously conclude that it got corrupted data due > to 4096 not being a multiple of struct pid_iter_entry, so the last entry will > be partially truncated. Fix this by sizing the buffer to fit exactly N entries > with no truncation in the middle of record. > > Fixes: d53dee3fe013 ("tools/bpftool: Show info for processes holding BPF map/prog/link/btf FDs") > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Ack with one nit below. Acked-by: Yonghong Song <yhs@fb.com> > --- > tools/bpf/bpftool/pids.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c > index df7d8ec76036..477e55d59c34 100644 > --- a/tools/bpf/bpftool/pids.c > +++ b/tools/bpf/bpftool/pids.c > @@ -89,9 +89,9 @@ libbpf_print_none(__maybe_unused enum libbpf_print_level level, > > int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type) > { > - char buf[4096]; > - struct pid_iter_bpf *skel; > struct pid_iter_entry *e; > + char buf[4096 / sizeof(*e) * sizeof(*e)]; > + struct pid_iter_bpf *skel; No need to move "struct pid_iter_bpf *skel", right? > int err, ret, fd = -1, i; > libbpf_print_fn_t default_print; > >
On Sat, Dec 5, 2020 at 11:11 AM Yonghong Song <yhs@fb.com> wrote: > > > > On 12/4/20 3:20 PM, Andrii Nakryiko wrote: > > In case of having so many PID results that they don't fit into a singe page > > (4096) bytes, bpftool will erroneously conclude that it got corrupted data due > > to 4096 not being a multiple of struct pid_iter_entry, so the last entry will > > be partially truncated. Fix this by sizing the buffer to fit exactly N entries > > with no truncation in the middle of record. > > > > Fixes: d53dee3fe013 ("tools/bpftool: Show info for processes holding BPF map/prog/link/btf FDs") > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > Ack with one nit below. > > Acked-by: Yonghong Song <yhs@fb.com> > > > --- > > tools/bpf/bpftool/pids.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c > > index df7d8ec76036..477e55d59c34 100644 > > --- a/tools/bpf/bpftool/pids.c > > +++ b/tools/bpf/bpftool/pids.c > > @@ -89,9 +89,9 @@ libbpf_print_none(__maybe_unused enum libbpf_print_level level, > > > > int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type) > > { > > - char buf[4096]; > > - struct pid_iter_bpf *skel; > > struct pid_iter_entry *e; > > + char buf[4096 / sizeof(*e) * sizeof(*e)]; > > + struct pid_iter_bpf *skel; > > No need to move "struct pid_iter_bpf *skel", right? It's actually a move of `struct pid_iter_entry *e;` in from of char buf[], to be able to use sizeof(*e) instead of sizeof(struct pid_iter_bpf). It's just that diff tool didn't catch this properly :) > > > int err, ret, fd = -1, i; > > libbpf_print_fn_t default_print; > > > >
On 12/7/20 6:55 PM, Andrii Nakryiko wrote: > On Sat, Dec 5, 2020 at 11:11 AM Yonghong Song <yhs@fb.com> wrote: >> >> >> >> On 12/4/20 3:20 PM, Andrii Nakryiko wrote: >>> In case of having so many PID results that they don't fit into a singe page >>> (4096) bytes, bpftool will erroneously conclude that it got corrupted data due >>> to 4096 not being a multiple of struct pid_iter_entry, so the last entry will >>> be partially truncated. Fix this by sizing the buffer to fit exactly N entries >>> with no truncation in the middle of record. >>> >>> Fixes: d53dee3fe013 ("tools/bpftool: Show info for processes holding BPF map/prog/link/btf FDs") >>> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> >> >> Ack with one nit below. >> >> Acked-by: Yonghong Song <yhs@fb.com> >> >>> --- >>> tools/bpf/bpftool/pids.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c >>> index df7d8ec76036..477e55d59c34 100644 >>> --- a/tools/bpf/bpftool/pids.c >>> +++ b/tools/bpf/bpftool/pids.c >>> @@ -89,9 +89,9 @@ libbpf_print_none(__maybe_unused enum libbpf_print_level level, >>> >>> int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type) >>> { >>> - char buf[4096]; >>> - struct pid_iter_bpf *skel; >>> struct pid_iter_entry *e; >>> + char buf[4096 / sizeof(*e) * sizeof(*e)]; >>> + struct pid_iter_bpf *skel; >> >> No need to move "struct pid_iter_bpf *skel", right? > > It's actually a move of `struct pid_iter_entry *e;` in from of char > buf[], to be able to use sizeof(*e) instead of sizeof(struct > pid_iter_bpf). It's just that diff tool didn't catch this properly :) Indeed. Looking at the final code, no unnecessary code churn. > >> >>> int err, ret, fd = -1, i; >>> libbpf_print_fn_t default_print; >>> >>>
Hello: This patch was applied to bpf/bpf.git (refs/heads/master): On Fri, 4 Dec 2020 15:20:01 -0800 you wrote: > In case of having so many PID results that they don't fit into a singe page > (4096) bytes, bpftool will erroneously conclude that it got corrupted data due > to 4096 not being a multiple of struct pid_iter_entry, so the last entry will > be partially truncated. Fix this by sizing the buffer to fit exactly N entries > with no truncation in the middle of record. > > Fixes: d53dee3fe013 ("tools/bpftool: Show info for processes holding BPF map/prog/link/btf FDs") > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > [...] Here is the summary with links: - [bpf] tools/bpftool: fix PID fetching with a lot of results https://git.kernel.org/bpf/bpf/c/932c60558109 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c index df7d8ec76036..477e55d59c34 100644 --- a/tools/bpf/bpftool/pids.c +++ b/tools/bpf/bpftool/pids.c @@ -89,9 +89,9 @@ libbpf_print_none(__maybe_unused enum libbpf_print_level level, int build_obj_refs_table(struct obj_refs_table *table, enum bpf_obj_type type) { - char buf[4096]; - struct pid_iter_bpf *skel; struct pid_iter_entry *e; + char buf[4096 / sizeof(*e) * sizeof(*e)]; + struct pid_iter_bpf *skel; int err, ret, fd = -1, i; libbpf_print_fn_t default_print;
In case of having so many PID results that they don't fit into a singe page (4096) bytes, bpftool will erroneously conclude that it got corrupted data due to 4096 not being a multiple of struct pid_iter_entry, so the last entry will be partially truncated. Fix this by sizing the buffer to fit exactly N entries with no truncation in the middle of record. Fixes: d53dee3fe013 ("tools/bpftool: Show info for processes holding BPF map/prog/link/btf FDs") Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/bpf/bpftool/pids.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)