| Message ID | 20190815142223.2203-1-quentin.monnet@netronome.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] tools: bpftool: close prog FD before exit on showing a single program | expand |
On Thu, Aug 15, 2019 at 7:24 AM Quentin Monnet <quentin.monnet@netronome.com> wrote: > > When showing metadata about a single program by invoking > "bpftool prog show PROG", the file descriptor referring to the program > is not closed before returning from the function. Let's close it. > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > tools/bpf/bpftool/prog.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > index 66f04a4846a5..43fdbbfe41bb 100644 > --- a/tools/bpf/bpftool/prog.c > +++ b/tools/bpf/bpftool/prog.c > @@ -363,7 +363,9 @@ static int do_show(int argc, char **argv) > if (fd < 0) > return -1; > > - return show_prog(fd); > + err = show_prog(fd); > + close(fd); > + return err; There is a similar problem few lines above for special case of argc == 2, which you didn't fix. Would it be better to make show_prog(fd) close provided fd instead or is it used in some other context where FD should live longer (I haven't checked, sorry)? > } > > if (argc) > -- > 2.17.1 >
On Thu, 15 Aug 2019 10:09:38 -0700, Andrii Nakryiko wrote: > On Thu, Aug 15, 2019 at 7:24 AM Quentin Monnet wrote: > > When showing metadata about a single program by invoking > > "bpftool prog show PROG", the file descriptor referring to the program > > is not closed before returning from the function. Let's close it. > > > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") > > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > --- > > tools/bpf/bpftool/prog.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > index 66f04a4846a5..43fdbbfe41bb 100644 > > --- a/tools/bpf/bpftool/prog.c > > +++ b/tools/bpf/bpftool/prog.c > > @@ -363,7 +363,9 @@ static int do_show(int argc, char **argv) > > if (fd < 0) > > return -1; > > > > - return show_prog(fd); > > + err = show_prog(fd); > > + close(fd); > > + return err; > > There is a similar problem few lines above for special case of argc == > 2, which you didn't fix. This is the special argc == 2 case. > Would it be better to make show_prog(fd) close provided fd instead or > is it used in some other context where FD should live longer (I > haven't checked, sorry)? I think it used to close that's how the bug crept in. Other than the bug it's fine the way it is.
On Thu, Aug 15, 2019 at 10:30 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Thu, 15 Aug 2019 10:09:38 -0700, Andrii Nakryiko wrote: > > On Thu, Aug 15, 2019 at 7:24 AM Quentin Monnet wrote: > > > When showing metadata about a single program by invoking > > > "bpftool prog show PROG", the file descriptor referring to the program > > > is not closed before returning from the function. Let's close it. > > > > > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") > > > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > > > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > > --- > > > tools/bpf/bpftool/prog.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c > > > index 66f04a4846a5..43fdbbfe41bb 100644 > > > --- a/tools/bpf/bpftool/prog.c > > > +++ b/tools/bpf/bpftool/prog.c > > > @@ -363,7 +363,9 @@ static int do_show(int argc, char **argv) > > > if (fd < 0) > > > return -1; > > > > > > - return show_prog(fd); > > > + err = show_prog(fd); > > > + close(fd); > > > + return err; > > > > There is a similar problem few lines above for special case of argc == > > 2, which you didn't fix. > > This is the special argc == 2 case. Yep, you are right, the other one already does this. > > > Would it be better to make show_prog(fd) close provided fd instead or > > is it used in some other context where FD should live longer (I > > haven't checked, sorry)? > > I think it used to close that's how the bug crept in. Other than the bug > it's fine the way it is. So are you saying that show_prog() should or should not close FD?
On Thu, 15 Aug 2019 11:05:16 -0700, Andrii Nakryiko wrote: > > > Would it be better to make show_prog(fd) close provided fd instead or > > > is it used in some other context where FD should live longer (I > > > haven't checked, sorry)? > > > > I think it used to close that's how the bug crept in. Other than the bug > > it's fine the way it is. > > So are you saying that show_prog() should or should not close FD? Yup, it we'd have to rename it to indicate it closes the fd, and it's only called in two places. Not worth the churn.
On Thu, Aug 15, 2019 at 11:09 AM Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Thu, 15 Aug 2019 11:05:16 -0700, Andrii Nakryiko wrote: > > > > Would it be better to make show_prog(fd) close provided fd instead or > > > > is it used in some other context where FD should live longer (I > > > > haven't checked, sorry)? > > > > > > I think it used to close that's how the bug crept in. Other than the bug > > > it's fine the way it is. > > > > So are you saying that show_prog() should or should not close FD? > > Yup, it we'd have to rename it to indicate it closes the fd, and it's > only called in two places. Not worth the churn. OK, I'm fine with that. Acked-by: Andrii Nakryiko <andriin@fb.com>
On Thu, Aug 15, 2019 at 7:22 AM Quentin Monnet <quentin.monnet@netronome.com> wrote: > > When showing metadata about a single program by invoking > "bpftool prog show PROG", the file descriptor referring to the program > is not closed before returning from the function. Let's close it. > > Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") > Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> > Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> Applied. Thanks
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c index 66f04a4846a5..43fdbbfe41bb 100644 --- a/tools/bpf/bpftool/prog.c +++ b/tools/bpf/bpftool/prog.c @@ -363,7 +363,9 @@ static int do_show(int argc, char **argv) if (fd < 0) return -1; - return show_prog(fd); + err = show_prog(fd); + close(fd); + return err; } if (argc)
When showing metadata about a single program by invoking "bpftool prog show PROG", the file descriptor referring to the program is not closed before returning from the function. Let's close it. Fixes: 71bb428fe2c1 ("tools: bpf: add bpftool") Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- tools/bpf/bpftool/prog.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)