On Mon, Feb 20, 2023 at 4:46 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> On Thu, 2023-02-16 at 15:28 -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 14, 2023 at 3:12 PM Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > wrote:
> > >
> > > MSan runs into a few false positives in libbpf. They all come from
> > > the
> > > fact that MSan does not know anything about the bpf syscall,
> > > particularly, what it writes to.
> > >
> > > Add __libbpf_mark_mem_written() function to mark memory modified by
> > > the
> > > bpf syscall, and a few convenience wrappers. Use the abstract name
> > > (it
> > > could be e.g. libbpf_msan_unpoison()), because it can be used for
> > > Valgrind in the future as well.
> > >
> > > Signed-off-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>
> > > ---
> > > tools/lib/bpf/bpf.c | 161
> > > ++++++++++++++++++++++++++++++--
> > > tools/lib/bpf/btf.c | 1 +
> > > tools/lib/bpf/libbpf.c | 1 +
> > > tools/lib/bpf/libbpf_internal.h | 38 ++++++++
> > > 4 files changed, 194 insertions(+), 7 deletions(-)
> >
>
> [...]
>
> > > +/* Helper macros for telling memory checkers that an array pointed
> > > to by
> > > + * a struct bpf_{btf,link,map,prog}_info member is initialized.
> > > Before doing
> > > + * that, they make sure that kernel has provided the respective
> > > member.
> > > + */
> > > +
> > > +/* Handle arrays with a certain element size. */
> > > +#define __MARK_INFO_ARRAY_WRITTEN(ptr, nr, elem_size) do
> > > { \
> > > + if (info_len >= offsetofend(typeof(*info), ptr)
> > > && \
> > > + info_len >= offsetofend(typeof(*info), nr)
> > > && \
> > > + info-
> > > >ptr) \
> > > + libbpf_mark_mem_written(u64_to_ptr(info-
> > > >ptr), \
> > > + info->nr *
> > > elem_size); \
> > > +} while (0)
> > > +
> > > +/* Handle arrays with a certain element type. */
> > > +#define MARK_INFO_ARRAY_WRITTEN(ptr, nr,
> > > type) \
> > > + __MARK_INFO_ARRAY_WRITTEN(ptr, nr, sizeof(type))
> > > +
> > > +/* Handle arrays with element size defined by a struct member. */
> > > +#define MARK_INFO_REC_ARRAY_WRITTEN(ptr, nr, rec_size) do
> > > { \
> > > + if (info_len >= offsetofend(typeof(*info),
> > > rec_size)) \
> > > + __MARK_INFO_ARRAY_WRITTEN(ptr, nr, info-
> > > >rec_size); \
> > > +} while (0)
> > > +
> > > +/* Handle null-terminated strings. */
> > > +#define MARK_INFO_STR_WRITTEN(ptr, nr) do
> > > { \
> > > + if (info_len >= offsetofend(typeof(*info), ptr)
> > > && \
> > > + info_len >= offsetofend(typeof(*info), nr)
> > > && \
> > > + info-
> > > >ptr) \
> > > + libbpf_mark_mem_written(u64_to_ptr(info-
> > > >ptr), \
> > > + info->nr +
> > > 1); \
> > > +} while (0)
> > > +
> > > +/* Helper functions for telling memory checkers that arrays
> > > pointed to by
> > > + * bpf_{btf,link,map,prog}_info members are initialized.
> > > + */
> > > +
> > > +static void mark_prog_info_written(struct bpf_prog_info *info,
> > > __u32 info_len)
> > > +{
> > > + MARK_INFO_ARRAY_WRITTEN(map_ids, nr_map_ids, __u32);
> > > + MARK_INFO_ARRAY_WRITTEN(jited_ksyms, nr_jited_ksyms,
> > > __u64);
> > > + MARK_INFO_ARRAY_WRITTEN(jited_func_lens,
> > > nr_jited_func_lens, __u32);
> > > + MARK_INFO_REC_ARRAY_WRITTEN(func_info, nr_func_info,
> > > + func_info_rec_size);
> > > + MARK_INFO_REC_ARRAY_WRITTEN(line_info, nr_line_info,
> > > + line_info_rec_size);
> > > + MARK_INFO_REC_ARRAY_WRITTEN(jited_line_info,
> > > nr_jited_line_info,
> > > + jited_line_info_rec_size);
> > > + MARK_INFO_ARRAY_WRITTEN(prog_tags, nr_prog_tags,
> > > __u8[BPF_TAG_SIZE]);
> > > +}
> > > +
> > > +static void mark_btf_info_written(struct bpf_btf_info *info, __u32
> > > info_len)
> > > +{
> > > + MARK_INFO_ARRAY_WRITTEN(btf, btf_size, __u8);
> > > + MARK_INFO_STR_WRITTEN(name, name_len);
> > > +}
> > > +
> > > +static void mark_link_info_written(struct bpf_link_info *info,
> > > __u32 info_len)
> > > +{
> > > + switch (info->type) {
> > > + case BPF_LINK_TYPE_RAW_TRACEPOINT:
> > > + MARK_INFO_STR_WRITTEN(raw_tracepoint.tp_name,
> > > + raw_tracepoint.tp_name_len);
> > > + break;
> > > + case BPF_LINK_TYPE_ITER:
> > > + MARK_INFO_STR_WRITTEN(iter.target_name,
> > > iter.target_name_len);
> > > + break;
> > > + default:
> > > + break;
> > > + }
> > > +}
> > > +
> > > +#undef MARK_INFO_STR_WRITTEN
> > > +#undef MARK_INFO_REC_ARRAY_WRITTEN
> > > +#undef MARK_INFO_ARRAY_WRITTEN
> > > +#undef __MARK_INFO_ARRAY_WRITTEN
> >
> > Ugh... I wasn't a big fan of adding all these "mark_mem_written"
> > across a bunch of APIs to begin with, but this part is really putting
> > me off.
> >
> > I like the bpf_{map,btf,prog,btf}_info_by_fd() improvements you did,
> > but maybe adding these MSan annotations is a bit too much?
> > Applications that really care about this whole "do I read
> > uninitialized memory" business could do their own simpler wrappers on
> > top of libbpf APIs, right?
> >
> > Maybe we should start there first and see if there is more demand to
> > have built-in libbpf support?
>
> I can try moving all this to selftests.
So I'm afraid this will introduce so much extra code just for marking
some memory written by kernel for MSan. It might be "cleaner" to just
zero-initialize all the memory we expect kernel to fill out, tbh.
> Alternatively this could be made a part of LLVM sanitizers, but then
> we come back to the question of resolving fd types.
>
> > BTW, is this all needed for ASan as well?
>
> Not strictly needed, but this would help detecting bad writes.
So truth be told, ASan and LeakSanitizer seem like more
immediately-useful next steps, I bet we'll find a bunch of memory
leaks and missing resource clean up sequences with this. If you are
playing with sanitizers, maybe let's start with Leak and Address
sanitizer first and make sure we run them continuously in BPF CI as
well?
And then from there let's see whether/what things we'd need to
annotate for sanitizers' sake?
>
> > One more worry I have is that given we don't exercise all these
> > sanitizers regularly in BPF CI, we'll keep forgetting adding new
> > annotations and all this machinery will start bit rotting.
> >
> > So I'd say that we should first make sure that we have sanitizer
> > builds/runs in BPF CI, before signing up for maintaining these
> > "annotations".
>
> I'll wait until LLVM folks review my patches, and then see if I can
> add MSan to the CI. Configuring it locally wasn't too complicated,
> the main difficulty is that one needs instrumented zlib and elfutils.
> For the CI, they can be prebuilt and uploaded to S3, and then added
> to the build environment and the image.
Will we need special versions of zlib and elfutils to be able to
enable ASan and LeakSan? I hope not. We might want to allow static
linking against them, though. Not sure. But as I said, I'd start with
ASan/LeakSan first.
>
> [...]
