On Thu, Feb 9, 2023 at 2:02 AM Ilya Leoshkevich <iii@xxxxxxxxxxxxx> wrote:
>
> 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.
yep, I know, it's a source of very confusing problems in some cases.
Which is why bpf_map__lookup_elem() and such expect user to provide
total key/value size (and checks it against struct bpf_map's knowledge
of key/value size, taking into account if map is per-CPU)
Ok, I don't see any other way around this as well. Just please extract
this check of whether the map is per-cpu or not into a separate
helper, so we can maintain this list in only one place. Currently we
have this check in validate_map_op() and I don't want to duplicate
this.
>
> > 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
> > >
>
