netdev - Re: [PATCH bpf-next] libbpf: fix compilation warnings for 64-bit printf args

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: <20200826160852.e4hnkyvg2kzrtzjj@ast-mbp.dhcp.thefacebook.com>
Date:   Wed, 26 Aug 2020 09:08:52 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andriin@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...com,
        daniel@...earbox.net, andrii.nakryiko@...il.com,
        kernel-team@...com, Naresh Kamboju <naresh.kamboju@...aro.org>
Subject: Re: [PATCH bpf-next] libbpf: fix compilation warnings for 64-bit
 printf args

On Tue, Aug 25, 2020 at 08:09:21PM -0700, Andrii Nakryiko wrote:
> Add __pu64 and __ps64 (sort of like "printf u64 and s64") for libbpf-internal
> use only in printf-like situations to avoid compilation warnings due to
> %lld/%llu mismatch with a __u64/__s64 due to some architecture defining the
> latter as either `long` or `long long`. Use that on all %lld/%llu cases in
> libbpf.c.
> 
> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
> Fixes: eacaaed784e2 ("libbpf: Implement enum value-based CO-RE relocations")
> Fixes: 50e09460d9f8 ("libbpf: Skip well-known ELF sections when iterating ELF")
> Signed-off-by: Andrii Nakryiko <andriin@...com>
> ---
>  tools/lib/bpf/libbpf.c          | 15 ++++++++-------
>  tools/lib/bpf/libbpf_internal.h | 11 +++++++++++
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 2e2523d8bb6d..211eb0d9020c 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1529,12 +1529,12 @@ static int set_kcfg_value_num(struct extern_desc *ext, void *ext_val,
>  {
>  	if (ext->kcfg.type != KCFG_INT && ext->kcfg.type != KCFG_CHAR) {
>  		pr_warn("extern (kcfg) %s=%llu should be integer\n",
> -			ext->name, (unsigned long long)value);
> +			ext->name, (__pu64)value);
>  		return -EINVAL;
>  	}
>  	if (!is_kcfg_value_in_range(ext, value)) {
>  		pr_warn("extern (kcfg) %s=%llu value doesn't fit in %d bytes\n",
> -			ext->name, (unsigned long long)value, ext->kcfg.sz);
> +			ext->name, (__pu64)value, ext->kcfg.sz);
>  		return -ERANGE;
>  	}
>  	switch (ext->kcfg.sz) {
> @@ -2823,7 +2823,8 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
>  			obj->efile.bss = data;
>  			obj->efile.bss_shndx = idx;
>  		} else {
> -			pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name, sh.sh_size);
> +			pr_info("elf: skipping section(%d) %s (size %zu)\n", idx, name,
> +				(size_t)sh.sh_size);
>  		}
>  	}
>  
> @@ -5244,7 +5245,7 @@ static int bpf_core_patch_insn(struct bpf_program *prog,
>  		if (res->validate && imm != orig_val) {
>  			pr_warn("prog '%s': relo #%d: unexpected insn #%d (LDIMM64) value: got %llu, exp %u -> %u\n",
>  				bpf_program__title(prog, false), relo_idx,
> -				insn_idx, imm, orig_val, new_val);
> +				insn_idx, (__pu64)imm, orig_val, new_val);
>  			return -EINVAL;
>  		}
>  
> @@ -5252,7 +5253,7 @@ static int bpf_core_patch_insn(struct bpf_program *prog,
>  		insn[1].imm = 0; /* currently only 32-bit values are supported */
>  		pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n",
>  			 bpf_program__title(prog, false), relo_idx, insn_idx,
> -			 imm, new_val);
> +			 (__pu64)imm, new_val);
>  		break;
>  	}
>  	default:
> @@ -7782,8 +7783,8 @@ static int bpf_object__collect_st_ops_relos(struct bpf_object *obj,
>  		st_ops = map->st_ops;
>  		pr_debug("struct_ops reloc %s: for %lld value %lld shdr_idx %u rel.r_offset %zu map->sec_offset %zu name %d (\'%s\')\n",
>  			 map->name,
> -			 (long long)(rel.r_info >> 32),
> -			 (long long)sym.st_value,
> +			 (__ps64)(rel.r_info >> 32),
> +			 (__ps64)sym.st_value,
>  			 shdr_idx, (size_t)rel.r_offset,
>  			 map->sec_offset, sym.st_name, name);
>  
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 4d1c366fca2c..7ad3c4b9917c 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -69,6 +69,17 @@ extern void libbpf_print(enum libbpf_print_level level,
>  			 const char *format, ...)
>  	__attribute__((format(printf, 2, 3)));
>  
> +/* These types are for casting 64-bit arguments of printf-like functions to
> + * avoid compiler warnings on various architectures that define size_t, __u64,
> + * uint64_t, etc as either unsigned long or unsigned long long (similarly for
> + * signed variants). Use these typedefs only for these purposes. Alternative
> + * is PRIu64 (and similar) macros, requiring stitching printf format strings
> + * which are extremely ugly and should be avoided in libbpf code base. With
> + * arguments casted to __pu64/__ps64, always use %llu/%lld in format string.
> + */
> +typedef unsigned long long __pu64;
> +typedef long long __ps64;

I think these extra typedefs will cause confusion. Original approach
of open coding type casts to long long and unsigned long long is imo cleaner.