[bpf] tools: bpftool: close prog FD before exit on showing a single program
diff mbox series

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
Related show

Commit Message

Quentin Monnet Aug. 15, 2019, 2:22 p.m. UTC
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(-)

Comments

Andrii Nakryiko Aug. 15, 2019, 5:09 p.m. UTC | #1
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
>
Jakub Kicinski Aug. 15, 2019, 5:30 p.m. UTC | #2
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.
Andrii Nakryiko Aug. 15, 2019, 6:05 p.m. UTC | #3
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?
Jakub Kicinski Aug. 15, 2019, 6:09 p.m. UTC | #4
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.
Andrii Nakryiko Aug. 15, 2019, 8:16 p.m. UTC | #5
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>
Alexei Starovoitov Aug. 16, 2019, 5:11 a.m. UTC | #6
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

Patch
diff mbox series

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)