netdev - Re: [PATCH bpf-next] 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] [day] [month] [year] [list]

Date:   Wed, 15 Jul 2020 20:02:55 -0700
From:   Tony Ambardar <tony.ambardar@...il.com>
To:     Quentin Monnet <quentin@...valent.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next] bpftool: use only nftw for file tree parsing

On Wed, 15 Jul 2020 at 10:35, Quentin Monnet <quentin@...valent.com> wrote:
>
> 2020-07-14 22:12 UTC-0700 ~ Tony Ambardar <tony.ambardar@...il.com>
> > 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 usage by rewriting one fts-based function to use nftw. This
> > change allows building bpftool against musl for OpenWrt.
> >
> > Signed-off-by: Tony Ambardar <Tony.Ambardar@...il.com>
>
> Thanks!
>

Thanks for your feedback and testing, Quentin, I really appreciate it.

> I tested your set, and bpftool does not compile on my setup. The
> definitions from <ftw.h> are not picked up by gcc, common.c should have
> a "#define _GNU_SOURCE" above its list of includes for this to work
> (like perf.c has).
>

OK, I see what happened. I omitted a required "#define _XOPEN_SOURCE
..." (like in cgroup.c).  Strictly speaking, "_GNU_SOURCE" is only
needed for a nftw() GNU extension not used in common.c or cgroup.c
(but used perf.c). It turns out there are still problems with missing
definitions for getpagesize() and getline(), which are most easily
pulled in with "_GNU_SOURCE". Will update as you suggest.

> I also get a warning on this line:
>
>
> > +static int do_build_table_cb(const char *fpath, const struct stat *sb,
> > +                         int typeflag, struct FTW *ftwbuf)
> >  {
>
> Because passing fptath to open_obj_pinned() below discards the "const"
> qualifier:
>
> > +     fd = open_obj_pinned(fpath, true);
>
> Fixed by having simply "char *fpath" as the first argument for
> do_build_table_cb().

Hmm, that only shifts the warning, since the cb function signature for
nftw still specifies "const char":

> common.c: In function ‘build_pinned_obj_table’:
> common.c:438:18: warning: passing argument 2 of ‘nftw’ from incompatible pointer type [-Wincompatible-pointer-types]
>    if (nftw(path, do_build_table_cb, nopenfd, flags) == -1)
>                   ^~~~~~~~~~~~~~~~~
> In file included from common.c:9:0:
> /usr/include/ftw.h:158:12: note: expected ‘__nftw_func_t {aka int (*)(const char *, const struct stat *, int,  struct FTW *)}’ but argument is of type ‘int (*)(char *, const struct stat *, int,  struct FTW *)’
>  extern int nftw (const char *__dir, __nftw_func_t __func, int __descriptors,
>             ^~~~

Wouldn't it be better/safer in general to constify the passed char to
'open_obj_pinned' and 'open_obj_pinned_any'?  However, doing so
revealed a problem in open_obj_pinned(), where dirname() is called
directly on the passed string. This could be dangerous since some
dirname() implementations may modify the string. Let's copy the string
instead (same approach in tools/lib/bpf/libbpf.c).

> With those two modifications, bpftool compiles fine and listing objects
> with the "-f" option works as expected.
>
> Regards,
> Quentin

Let me make these changes and see what you think.

Best regards,
Tony