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?
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?
> 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
>
