| Message ID | 20200114224400.3027140-1-kafai@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,1/5] bpftool: Fix a leak of btf object | expand |
On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote: > > When testing a map has btf or not, maps_have_btf() tests it by actually > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it > forgot to btf__free() it. > > In maps_have_btf() stage, there is no need to test it by really > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero > info.btf_id is good enough. > > Also, the err_close case is unnecessary, and also causes double > close() because the calling func do_dump() will close() all fds again. > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name") > Cc: Paul Chaignon <paul.chaignon@orange.com> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- this is clearly a simplification, but isn't do_dump still buggy? see below > tools/bpf/bpftool/map.c | 16 ++-------------- > 1 file changed, 2 insertions(+), 14 deletions(-) > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > index c01f76fa6876..e00e9e19d6b7 100644 > --- a/tools/bpf/bpftool/map.c > +++ b/tools/bpf/bpftool/map.c > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds) > { > struct bpf_map_info info = {}; > __u32 len = sizeof(info); > - struct btf *btf = NULL; > int err, i; > > for (i = 0; i < nb_fds; i++) { > err = bpf_obj_get_info_by_fd(fds[i], &info, &len); > if (err) { > p_err("can't get map info: %s", strerror(errno)); > - goto err_close; > - } > - > - err = btf__get_from_id(info.btf_id, &btf); > - if (err) { > - p_err("failed to get btf"); > - goto err_close; > + return -1; > } > > - if (!btf) > + if (!info.btf_id) > return 0; if info.btf_id is non-zero, shouldn't we immediately return 1 and be done with it? I'm also worried about do_dump logic. What's the behavior when some maps do have BTF and some don't? Should we use btf_writer for all, some or none maps for that case? I'd expect we'd use BTF info for those maps that have BTF and fall back to raw output for those that don't, but I'm not sure that how code behaves right now. Maybe Paul can clarify... > } > > return 1; > - > -err_close: > - for (; i < nb_fds; i++) > - close(fds[i]); > - return -1; > } > > static int > -- > 2.17.1 >
On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote: > On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > When testing a map has btf or not, maps_have_btf() tests it by actually > > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it > > forgot to btf__free() it. > > > > In maps_have_btf() stage, there is no need to test it by really > > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero > > info.btf_id is good enough. > > > > Also, the err_close case is unnecessary, and also causes double > > close() because the calling func do_dump() will close() all fds again. > > > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name") > > Cc: Paul Chaignon <paul.chaignon@orange.com> > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > this is clearly a simplification, but isn't do_dump still buggy? see below > > > tools/bpf/bpftool/map.c | 16 ++-------------- > > 1 file changed, 2 insertions(+), 14 deletions(-) > > > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > > index c01f76fa6876..e00e9e19d6b7 100644 > > --- a/tools/bpf/bpftool/map.c > > +++ b/tools/bpf/bpftool/map.c > > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds) > > { > > struct bpf_map_info info = {}; > > __u32 len = sizeof(info); > > - struct btf *btf = NULL; > > int err, i; > > > > for (i = 0; i < nb_fds; i++) { > > err = bpf_obj_get_info_by_fd(fds[i], &info, &len); > > if (err) { > > p_err("can't get map info: %s", strerror(errno)); > > - goto err_close; > > - } > > - > > - err = btf__get_from_id(info.btf_id, &btf); > > - if (err) { > > - p_err("failed to get btf"); > > - goto err_close; > > + return -1; > > } > > > > - if (!btf) > > + if (!info.btf_id) > > return 0; > > if info.btf_id is non-zero, shouldn't we immediately return 1 and be > done with it? No. maps_have_btf() returns 1 only if all the maps have btf. > > I'm also worried about do_dump logic. What's the behavior when some > maps do have BTF and some don't? Should we use btf_writer for all, > some or none maps for that case? For plain_text, btf output is either for all or for none. It is the intention of the "Fixes" patch if I read it correctly, and it is kept as is in this bug fix. It will become clear by doing a plain text dump on maps with and without btf. They are very different. Can the output format for with and without BTF somehow merged for plain text? May be if it is still common to have no-BTF map going forward but how this may look like will need another discussion. > I'd expect we'd use BTF info for > those maps that have BTF and fall back to raw output for those that > don't, but I'm not sure that how code behaves right now. The json_output is doing what you described, print BTF info whenever available. > > Maybe Paul can clarify... > > > > } > > > > return 1; > > - > > -err_close: > > - for (; i < nb_fds; i++) > > - close(fds[i]); > > - return -1; > > } > > > > static int > > -- > > 2.17.1 > >
On Tue, Jan 14, 2020 at 9:46 PM Martin Lau <kafai@fb.com> wrote: > > On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote: > > On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > When testing a map has btf or not, maps_have_btf() tests it by actually > > > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it > > > forgot to btf__free() it. > > > > > > In maps_have_btf() stage, there is no need to test it by really > > > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero > > > info.btf_id is good enough. > > > > > > Also, the err_close case is unnecessary, and also causes double > > > close() because the calling func do_dump() will close() all fds again. > > > > > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name") > > > Cc: Paul Chaignon <paul.chaignon@orange.com> > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > > this is clearly a simplification, but isn't do_dump still buggy? see below > > > > > tools/bpf/bpftool/map.c | 16 ++-------------- > > > 1 file changed, 2 insertions(+), 14 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > > > index c01f76fa6876..e00e9e19d6b7 100644 > > > --- a/tools/bpf/bpftool/map.c > > > +++ b/tools/bpf/bpftool/map.c > > > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds) > > > { > > > struct bpf_map_info info = {}; > > > __u32 len = sizeof(info); > > > - struct btf *btf = NULL; > > > int err, i; > > > > > > for (i = 0; i < nb_fds; i++) { > > > err = bpf_obj_get_info_by_fd(fds[i], &info, &len); > > > if (err) { > > > p_err("can't get map info: %s", strerror(errno)); > > > - goto err_close; > > > - } > > > - > > > - err = btf__get_from_id(info.btf_id, &btf); > > > - if (err) { > > > - p_err("failed to get btf"); > > > - goto err_close; > > > + return -1; > > > } > > > > > > - if (!btf) > > > + if (!info.btf_id) > > > return 0; > > > > if info.btf_id is non-zero, shouldn't we immediately return 1 and be > > done with it? > No. maps_have_btf() returns 1 only if all the maps have btf. > > > > > I'm also worried about do_dump logic. What's the behavior when some > > maps do have BTF and some don't? Should we use btf_writer for all, > > some or none maps for that case? > For plain_text, btf output is either for all or for none. > It is the intention of the "Fixes" patch if I read it correctly, > and it is kept as is in this bug fix. > It will become clear by doing a plain text dump on maps with and > without btf. They are very different. > > Can the output format for with and without BTF somehow merged for > plain text? May be if it is still common to have no-BTF map > going forward but how this may look like will need another > discussion. I see, ok, seems like that behavior was intentional, I didn't mean to start a new discussion about format :) Acked-by: Andrii Nakryiko <andriin@fb.com> > > > I'd expect we'd use BTF info for > > those maps that have BTF and fall back to raw output for those that > > don't, but I'm not sure that how code behaves right now. > The json_output is doing what you described, print BTF info > whenever available. > > > > > Maybe Paul can clarify... > > > > > > > } > > > > > > return 1; > > > - > > > -err_close: > > > - for (; i < nb_fds; i++) > > > - close(fds[i]); > > > - return -1; > > > } > > > > > > static int > > > -- > > > 2.17.1 > > >
On Wed, Jan 15, 2020 at 6:46 AM Martin Lau <kafai@fb.com> wrote: > On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote: > > On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > When testing a map has btf or not, maps_have_btf() tests it by actually > > > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it > > > forgot to btf__free() it. > > > > > > In maps_have_btf() stage, there is no need to test it by really > > > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero > > > info.btf_id is good enough. > > > > > > Also, the err_close case is unnecessary, and also causes double > > > close() because the calling func do_dump() will close() all fds again. > > > > > > Fixes: 99f9863a0c45 ("bpftool: Match maps by name") > > > Cc: Paul Chaignon <paul.chaignon@orange.com> > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > > this is clearly a simplification, but isn't do_dump still buggy? see > below > > > > > tools/bpf/bpftool/map.c | 16 ++-------------- > > > 1 file changed, 2 insertions(+), 14 deletions(-) > > > > > > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c > > > index c01f76fa6876..e00e9e19d6b7 100644 > > > --- a/tools/bpf/bpftool/map.c > > > +++ b/tools/bpf/bpftool/map.c > > > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds) > > > { > > > struct bpf_map_info info = {}; > > > __u32 len = sizeof(info); > > > - struct btf *btf = NULL; > > > int err, i; > > > > > > for (i = 0; i < nb_fds; i++) { > > > err = bpf_obj_get_info_by_fd(fds[i], &info, &len); > > > if (err) { > > > p_err("can't get map info: %s", > strerror(errno)); > > > - goto err_close; > > > - } > > > - > > > - err = btf__get_from_id(info.btf_id, &btf); > > > - if (err) { > > > - p_err("failed to get btf"); > > > - goto err_close; > > > + return -1; > > > } > > > > > > - if (!btf) > > > + if (!info.btf_id) > > > return 0; > > > > if info.btf_id is non-zero, shouldn't we immediately return 1 and be > > done with it? > No. maps_have_btf() returns 1 only if all the maps have btf. > > > > > I'm also worried about do_dump logic. What's the behavior when some > > maps do have BTF and some don't? Should we use btf_writer for all, > > some or none maps for that case? > For plain_text, btf output is either for all or for none. > It is the intention of the "Fixes" patch if I read it correctly, > and it is kept as is in this bug fix. > It will become clear by doing a plain text dump on maps with and > without btf. They are very different. Yes, that was the intent of my patch. As you said, I wasn't sure mixes of BTF/non-BTF maps were common, especially in a batch sharing the same name. Thanks for the fixes Martin! > > Can the output format for with and without BTF somehow merged for > plain text? May be if it is still common to have no-BTF map > going forward but how this may look like will need another > discussion. > > > I'd expect we'd use BTF info for > > those maps that have BTF and fall back to raw output for those that > > don't, but I'm not sure that how code behaves right now. > The json_output is doing what you described, print BTF info > whenever available. > > > > > Maybe Paul can clarify... > > > > > > > } > > > > > > return 1; > > > - > > > -err_close: > > > - for (; i < nb_fds; i++) > > > - close(fds[i]); > > > - return -1; > > > } > > > > > > static int > > > -- > > > 2.17.1 > > > >
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c index c01f76fa6876..e00e9e19d6b7 100644 --- a/tools/bpf/bpftool/map.c +++ b/tools/bpf/bpftool/map.c @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds) { struct bpf_map_info info = {}; __u32 len = sizeof(info); - struct btf *btf = NULL; int err, i; for (i = 0; i < nb_fds; i++) { err = bpf_obj_get_info_by_fd(fds[i], &info, &len); if (err) { p_err("can't get map info: %s", strerror(errno)); - goto err_close; - } - - err = btf__get_from_id(info.btf_id, &btf); - if (err) { - p_err("failed to get btf"); - goto err_close; + return -1; } - if (!btf) + if (!info.btf_id) return 0; } return 1; - -err_close: - for (; i < nb_fds; i++) - close(fds[i]); - return -1; } static int
When testing a map has btf or not, maps_have_btf() tests it by actually getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it forgot to btf__free() it. In maps_have_btf() stage, there is no need to test it by really calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero info.btf_id is good enough. Also, the err_close case is unnecessary, and also causes double close() because the calling func do_dump() will close() all fds again. Fixes: 99f9863a0c45 ("bpftool: Match maps by name") Cc: Paul Chaignon <paul.chaignon@orange.com> Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- tools/bpf/bpftool/map.c | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-)