netdev - RE: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

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: <6172f428e4174_86bab20824@john-XPS-13-9370.notmuch>
Date:   Fri, 22 Oct 2021 10:26:00 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     John Fastabend <john.fastabend@...il.com>,
        Stanislav Fomichev <sdf@...gle.com>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        Stanislav Fomichev <sdf@...gle.com>
Subject: RE: [PATCH bpf-next 2/3] bpftool: don't append / to the progtype

John Fastabend wrote:
> Stanislav Fomichev wrote:
> > Otherwise, attaching with bpftool doesn't work with strict section names.
> > 
> > Also, switch to libbpf strict mode to use the latest conventions
> > (note, I don't think we have any cli api guarantees?).
> > 
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  tools/bpf/bpftool/main.c |  4 ++++
> >  tools/bpf/bpftool/prog.c | 15 +--------------
> >  2 files changed, 5 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/bpf/bpftool/main.c b/tools/bpf/bpftool/main.c
> > index 02eaaf065f65..8223bac1e401 100644
> > --- a/tools/bpf/bpftool/main.c
> > +++ b/tools/bpf/bpftool/main.c
> > @@ -409,6 +409,10 @@ int main(int argc, char **argv)
> >  	block_mount = false;
> >  	bin_name = argv[0];
> >  
> > +	ret = libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
> > +	if (ret)
> > +		p_err("failed to enable libbpf strict mode: %d", ret);
> > +
> 
> Would it better to just warn? Seems like this shouldn't be fatal from
> bpftool side?
> 
> Also this is a potentially breaking change correct? Programs that _did_
> work in the unstrict might suddently fail in the strict mode? If this
> is the case whats the versioning plan? We don't want to leak these
> type of changes across multiple versions, idealy we have a hard
> break and bump the version.
> 
> I didn't catch a cover letter on the series. A small
> note about versioning and upgrading bpftool would be helpful.
> 
> 
> >  	hash_init(prog_table.table);
> >  	hash_init(map_table.table);
> >  	hash_init(link_table.table);
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index 277d51c4c5d9..17505dc1243e 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -1396,8 +1396,6 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> >  
> >  	while (argc) {
> >  		if (is_prefix(*argv, "type")) {
> > -			char *type;
> > -
> >  			NEXT_ARG();
> >  
> >  			if (common_prog_type != BPF_PROG_TYPE_UNSPEC) {
> > @@ -1407,19 +1405,8 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> >  			if (!REQ_ARGS(1))
> >  				goto err_free_reuse_maps;
> >  
> > -			/* Put a '/' at the end of type to appease libbpf */
> > -			type = malloc(strlen(*argv) + 2);
> > -			if (!type) {
> > -				p_err("mem alloc failed");
> > -				goto err_free_reuse_maps;
> > -			}
> > -			*type = 0;
> > -			strcat(type, *argv);
> > -			strcat(type, "/");
> > -
> > -			err = get_prog_type_by_name(type, &common_prog_type,
> > +			err = get_prog_type_by_name(*argv, &common_prog_type,
> >  						    &expected_attach_type);
> > -			free(type);
> >  			if (err < 0)
> >  				goto err_free_reuse_maps;
> 
> This wont potentially break existing programs correct? It looks like
> just adding a '/' should be fine.
> 
> Thanks,
> John

Oops  wrong version of the patch. I'll reply in the more recent one.