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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191213123922.GA6538@Omicron>
Date:   Fri, 13 Dec 2019 13:39:22 +0100
From:   Paul Chaignon <paul.chaignon@...nge.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     bpf@...r.kernel.org, Quentin Monnet <quentin.monnet@...ronome.com>,
        paul.chaignon@...il.com, netdev@...r.kernel.org,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Andrii Nakryiko <andriin@...com>
Subject: 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:
> > 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@...nge.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
>