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: <6172ef4180b84_840632087a@john-XPS-13-9370.notmuch>
Date:   Fri, 22 Oct 2021 10:05:05 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     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

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