Re: [PATCH bpf-next 1/3] bpftool: match several programs with same tag

[Paul Chaignon <paul.chaignon@orange.com>]

On Tue, Dec 10, 2019 at 12:36:25PM -0800, Jakub Kicinski wrote:
> On Tue, 10 Dec 2019 17:06:25 +0100, Paul Chaignon wrote:
> > When several BPF programs have the same tag, bpftool matches only the
> > first (in ID order).  This patch changes that behavior such that dump and
> > show commands return all matched programs.  Commands that require a single
> > program (e.g., pin and attach) will error out if given a tag that matches
> > several.  bpftool prog dump will also error out if file or visual are
> > given and several programs have the given tag.
> > 
> > In the case of the dump command, a program header is added before each
> > dump only if the tag matches several programs; this patch doesn't change
> > the output if a single program matches.
> 
> How does this work? Could you add examples to the commit message?
> 
> This header idea doesn't seem correct, aren't id and other per-instance
> fields only printed once?

Sorry, that was unclear.  What I call the header here is the first line
from the prog show output (in the case of plain output).  So the output
when multiple programs match looks as follows.  When a single program
matches, the first line (with the ID, type, name, tag and license) is
omitted.

$ ./bpftool prog dump xlated tag 6deef7357e7b4530
3: cgroup_skb  tag 6deef7357e7b4530  gpl
   0: (bf) r6 = r1
   [...]
   7: (95) exit

4: cgroup_skb  tag 6deef7357e7b4530  gpl
   0: (bf) r6 = r1
   [...]
   7: (95) exit

> 
> > Signed-off-by: Paul Chaignon <paul.chaignon@orange.com>
> 
> > -		close(fd);
> > +		if (nb_fds > 0) {
> > +			tmp = realloc(fds, (nb_fds + 1) * sizeof(int));
> > +			if (!tmp) {
> > +				p_err("failed to realloc");
> > +				goto err_close_fd;
> > +			}
> > +			fds = tmp;
> 
> How does this work? the new array is never returned to the caller, and
> the caller will most likely access freed memory, no?

Oh, this is bad.  Yes, fds should actually be "int **" and this line
should be "*fds = tmp;".  I'll fix it in v2.

[...]

> > +				close(fds[nb_fds]);
> > +		}
> > +		fd = -1;
> > +		goto err_free;
> > +	}
> > +
> > +	fd = fds[0];
> > +err_free:
> 
> nit: we tried to call the labels exit_xyz if the code is used on both
>      error and success path, but maybe that pattern got lost over time.

Seems lost in prog.c but still valid across bpftool.  I'll make the
change.

[...]

Paul
>