On Wed, 2023-02-08 at 17:29 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 8, 2023 at 12:57 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_defined() function to mark memory modified by the
> > bpf
> > syscall. 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>
> > ---
>
> This is a lot to satisfy MSan, especially mark_map_value_defined
> which
> has to do bpf_obj_get_info_by_fd()... Is there any other way? What
> happens if we don't do it?
It would generate false positives when accessing the resulting map
values, which is not nice. The main complexity in this case comes not
from mark_map_value_defined() per se, but from the fact that we don't
know the value size, especially for percpu maps.
I toyed with the idea to extend the BPF_MAP_LOOKUP_ELEM interface to
return the number of bytes written, but decided against it - the only
user of this would be MSan, and at least for the beginning it's better
to have extra complexity in userspace, rather than in kernel.
> As for libbpf_mark_defined, wouldn't it be cleaner to teach it to
> handle NULL pointer and/or zero size transparently? Also, we can have
> libbpf_mark_defined_if(void *ptr, size_t sz, bool cond), so in code
> we
> don't have to have those explicit if conditions. Instead of:
>
> > + if (!ret && prog_ids)
> > + libbpf_mark_defined(prog_ids,
> > + attr.query.prog_cnt *
> > sizeof(*prog_ids));
>
> we can write just
>
> libbpf_mark_defined_if(prog_ids, attr.query.prog_cnt *
> sizeof(*prog_ids), ret == 0);
>
> ?
>
> Should we also call a helper something like
> 'libbpf_mark_mem_written',
> because that's what we are saying here, right?
Sure, all this sounds reasonable. Will do.
>
> > tools/lib/bpf/bpf.c | 70
> > +++++++++++++++++++++++++++++++--
> > tools/lib/bpf/btf.c | 1 +
> > tools/lib/bpf/libbpf.c | 1 +
> > tools/lib/bpf/libbpf_internal.h | 14 +++++++
> > 4 files changed, 82 insertions(+), 4 deletions(-)
> >
>
> [...]
>
> > diff --git a/tools/lib/bpf/libbpf_internal.h
> > b/tools/lib/bpf/libbpf_internal.h
> > index fbaf68335394..4e4622f66fdf 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -577,4 +577,18 @@ static inline bool is_pow_of_2(size_t x)
> > #define PROG_LOAD_ATTEMPTS 5
> > int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int
> > attempts);
> >
> > +#if defined(__has_feature)
> > +#if __has_feature(memory_sanitizer)
> > +#define LIBBPF_MSAN
> > +#endif
> > +#endif
> > +
> > +#ifdef LIBBPF_MSAN
>
> would below work:
>
> #if defined(__has_feature) && __has_feature(memory_sanitizer)
>
> ?
>
> > +#define HAVE_LIBBPF_MARK_DEFINED
> > +#include <sanitizer/msan_interface.h>
> > +#define libbpf_mark_defined __msan_unpoison
> > +#else
> > +static inline void libbpf_mark_defined(void *s, size_t n) {}
> > +#endif
> > +
> > #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> > --
> > 2.39.1
> >
