netdev - Re: [PATCH bpf-next v3] bpftool: use only nftw for file tree parsing

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: <fdd2d23f-773d-172c-fce1-0f2641763580@isovalent.com>
Date:   Mon, 20 Jul 2020 09:13:26 +0100
From:   Quentin Monnet <quentin@...valent.com>
To:     Tony Ambardar <tony.ambardar@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v3] bpftool: use only nftw for file tree parsing

On 17/07/2020 23:55, Tony Ambardar wrote:
> The bpftool sources include code to walk file trees, but use multiple
> frameworks to do so: nftw and fts. While nftw conforms to POSIX/SUSv3 and
> is widely available, fts is not conformant and less common, especially on
> non-glibc systems. The inconsistent framework usage hampers maintenance
> and portability of bpftool, in particular for embedded systems.
> 
> Standardize code usage by rewriting one fts-based function to use nftw and
> clean up some related function warnings by extending use of "const char *"
> arguments. This change helps in building bpftool against musl for OpenWrt.
> 
> Also fix an unsafe call to dirname() by duplicating the string to pass,
> since some implementations may directly alter it. The same approach is
> used in libbpf.c.
> 
> Signed-off-by: Tony Ambardar <Tony.Ambardar@...il.com>
> ---
> 
> V3:
> * clarify dirname() path copy in commit message
> * fix whitespace and rearrange comment for clarity
> * drop unnecessary initializers, rebalance Christmas tree
> * fixup error message and drop others not previously present
> * simplify malloc() + memset() -> calloc() and check for mem errors
> 
> V2:
> * use _GNU_SOURCE to pull in getpagesize(), getline(), nftw() definitions
> * use "const char *" in open_obj_pinned() and open_obj_pinned_any()
> * make dirname() safely act on a string copy
> 
> ---
>  tools/bpf/bpftool/common.c | 132 +++++++++++++++++++++----------------
>  tools/bpf/bpftool/main.h   |   4 +-
>  2 files changed, 78 insertions(+), 58 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 29f4e7611ae8..2ecfafcd01df 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c

>  int build_pinned_obj_table(struct pinned_obj_table *tab,
>  			   enum bpf_obj_type type)
>  {

[...]

>  	while ((mntent = getmntent(mntfile))) {

[...]

> -		while ((ftse = fts_read(fts))) {

[...]

> +		if (nftw(path, do_build_table_cb, nopenfd, flags) == -1)
> +			break;

Sorry I missed that on the previous reviews; but I think a simple break
out of the loop changes the previous behaviour, we should instead
"return -1" from build_pinned_obj_table() if nftw() returns -1, as we
were doing so far.

Looks good otherwise.

>  	}
>  	fclose(mntfile);
>  	return 0;