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

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:

quoted

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

quoted

Signed-off-by: Paul Chaignon <redacted>

quoted

-		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.

[...]

quoted

+				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