| Message ID | 20230208205642.270567-9-iii@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | selftests/bpf: Add Memory Sanitizer support | expand |
On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > --- 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 >
On Wed, 2023-02-08 at 17:29 -0800, Andrii Nakryiko wrote: > On Wed, Feb 8, 2023 at 12:57 PM Ilya Leoshkevich <iii@linux.ibm.com> > 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@linux.ibm.com> > > --- > > 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 > >
On Thu, Feb 9, 2023 at 2:02 AM Ilya Leoshkevich <iii@linux.ibm.com> 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@linux.ibm.com> > > 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@linux.ibm.com> > > > --- > > > > 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 > > > >
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 9aff98f42a3d..12fdc4ce1780 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -92,6 +92,10 @@ int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts) fd = sys_bpf_fd(BPF_PROG_LOAD, attr, size); } while (fd < 0 && errno == EAGAIN && --attempts > 0); + if (attr->log_buf) + libbpf_mark_defined((void *)(unsigned long)attr->log_buf, + attr->log_size); + return fd; } @@ -395,6 +399,33 @@ int bpf_map_update_elem(int fd, const void *key, const void *value, return libbpf_err_errno(ret); } +static void mark_map_value_defined(int fd, void *value) +{ +#ifdef HAVE_LIBBPF_MARK_DEFINED + struct bpf_map_info info; + size_t size = 0; + __u32 info_len; + int num_cpus; + int err; + + info_len = sizeof(info); + err = bpf_obj_get_info_by_fd(fd, &info, &info_len); + if (!err) { + size = info.value_size; + if (info.type == BPF_MAP_TYPE_PERCPU_ARRAY || + info.type == BPF_MAP_TYPE_PERCPU_HASH || + info.type == BPF_MAP_TYPE_LRU_PERCPU_HASH || + info.type == BPF_MAP_TYPE_PERCPU_CGROUP_STORAGE) { + num_cpus = libbpf_num_possible_cpus(); + if (num_cpus > 0) + size *= num_cpus; + } + } + if (size) + libbpf_mark_defined(value, size); +#endif +} + int bpf_map_lookup_elem(int fd, const void *key, void *value) { const size_t attr_sz = offsetofend(union bpf_attr, flags); @@ -407,6 +438,8 @@ int bpf_map_lookup_elem(int fd, const void *key, void *value) attr.value = ptr_to_u64(value); ret = sys_bpf(BPF_MAP_LOOKUP_ELEM, &attr, attr_sz); + if (!ret) + mark_map_value_defined(fd, value); return libbpf_err_errno(ret); } @@ -423,6 +456,8 @@ int bpf_map_lookup_elem_flags(int fd, const void *key, void *value, __u64 flags) attr.flags = flags; ret = sys_bpf(BPF_MAP_LOOKUP_ELEM, &attr, attr_sz); + if (!ret) + mark_map_value_defined(fd, value); return libbpf_err_errno(ret); } @@ -438,6 +473,8 @@ int bpf_map_lookup_and_delete_elem(int fd, const void *key, void *value) attr.value = ptr_to_u64(value); ret = sys_bpf(BPF_MAP_LOOKUP_AND_DELETE_ELEM, &attr, attr_sz); + if (!ret) + mark_map_value_defined(fd, value); return libbpf_err_errno(ret); } @@ -454,6 +491,8 @@ int bpf_map_lookup_and_delete_elem_flags(int fd, const void *key, void *value, _ attr.flags = flags; ret = sys_bpf(BPF_MAP_LOOKUP_AND_DELETE_ELEM, &attr, attr_sz); + if (!ret) + mark_map_value_defined(fd, value); return libbpf_err_errno(ret); } @@ -823,10 +862,12 @@ int bpf_prog_query_opts(int target_fd, { const size_t attr_sz = offsetofend(union bpf_attr, query); union bpf_attr attr; + __u32 *prog_ids; int ret; if (!OPTS_VALID(opts, bpf_prog_query_opts)) return libbpf_err(-EINVAL); + prog_ids = OPTS_GET(opts, prog_ids, NULL); memset(&attr, 0, attr_sz); @@ -834,11 +875,15 @@ int bpf_prog_query_opts(int target_fd, attr.query.attach_type = type; attr.query.query_flags = OPTS_GET(opts, query_flags, 0); attr.query.prog_cnt = OPTS_GET(opts, prog_cnt, 0); - attr.query.prog_ids = ptr_to_u64(OPTS_GET(opts, prog_ids, NULL)); + attr.query.prog_ids = ptr_to_u64(prog_ids); attr.query.prog_attach_flags = ptr_to_u64(OPTS_GET(opts, prog_attach_flags, NULL)); ret = sys_bpf(BPF_PROG_QUERY, &attr, attr_sz); + if (!ret && prog_ids) + libbpf_mark_defined(prog_ids, + attr.query.prog_cnt * sizeof(*prog_ids)); + OPTS_SET(opts, attach_flags, attr.query.attach_flags); OPTS_SET(opts, prog_cnt, attr.query.prog_cnt); @@ -868,10 +913,14 @@ int bpf_prog_test_run_opts(int prog_fd, struct bpf_test_run_opts *opts) { const size_t attr_sz = offsetofend(union bpf_attr, test); union bpf_attr attr; + void *data_out; + void *ctx_out; int ret; if (!OPTS_VALID(opts, bpf_test_run_opts)) return libbpf_err(-EINVAL); + data_out = OPTS_GET(opts, data_out, NULL); + ctx_out = OPTS_GET(opts, ctx_out, NULL); memset(&attr, 0, attr_sz); attr.test.prog_fd = prog_fd; @@ -885,12 +934,19 @@ int bpf_prog_test_run_opts(int prog_fd, struct bpf_test_run_opts *opts) attr.test.data_size_in = OPTS_GET(opts, data_size_in, 0); attr.test.data_size_out = OPTS_GET(opts, data_size_out, 0); attr.test.ctx_in = ptr_to_u64(OPTS_GET(opts, ctx_in, NULL)); - attr.test.ctx_out = ptr_to_u64(OPTS_GET(opts, ctx_out, NULL)); + attr.test.ctx_out = ptr_to_u64(ctx_out); attr.test.data_in = ptr_to_u64(OPTS_GET(opts, data_in, NULL)); - attr.test.data_out = ptr_to_u64(OPTS_GET(opts, data_out, NULL)); + attr.test.data_out = ptr_to_u64(data_out); ret = sys_bpf(BPF_PROG_TEST_RUN, &attr, attr_sz); + if (!ret) { + if (data_out) + libbpf_mark_defined(data_out, attr.test.data_size_out); + if (ctx_out) + libbpf_mark_defined(ctx_out, attr.test.ctx_size_out); + } + OPTS_SET(opts, data_size_out, attr.test.data_size_out); OPTS_SET(opts, ctx_size_out, attr.test.ctx_size_out); OPTS_SET(opts, duration, attr.test.duration); @@ -1039,8 +1095,10 @@ int bpf_obj_get_info_by_fd(int bpf_fd, void *info, __u32 *info_len) attr.info.info = ptr_to_u64(info); err = sys_bpf(BPF_OBJ_GET_INFO_BY_FD, &attr, attr_sz); - if (!err) + if (!err) { *info_len = attr.info.info_len; + libbpf_mark_defined(info, attr.info.info_len); + } return libbpf_err_errno(err); } @@ -1103,6 +1161,8 @@ int bpf_btf_load(const void *btf_data, size_t btf_size, const struct bpf_btf_loa attr.btf_log_level = 1; fd = sys_bpf_fd(BPF_BTF_LOAD, &attr, attr_sz); } + if (log_buf) + libbpf_mark_defined(log_buf, attr.btf_log_size); return libbpf_err_errno(fd); } @@ -1122,6 +1182,8 @@ int bpf_task_fd_query(int pid, int fd, __u32 flags, char *buf, __u32 *buf_len, attr.task_fd_query.buf_len = *buf_len; err = sys_bpf(BPF_TASK_FD_QUERY, &attr, attr_sz); + if (!err && buf) + libbpf_mark_defined(buf, attr.task_fd_query.buf_len + 1); *buf_len = attr.task_fd_query.buf_len; *prog_id = attr.task_fd_query.prog_id; diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c index 64841117fbb2..24a957604a97 100644 --- a/tools/lib/bpf/btf.c +++ b/tools/lib/bpf/btf.c @@ -1388,6 +1388,7 @@ struct btf *btf_get_from_fd(int btf_fd, struct btf *base_btf) goto exit_free; } + libbpf_mark_defined(ptr, btf_info.btf_size); btf = btf_new(ptr, btf_info.btf_size, base_btf); exit_free: diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 4191a78b2815..3e32ae5f0cce 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -5443,6 +5443,7 @@ static int load_module_btfs(struct bpf_object *obj) pr_warn("failed to get BTF object #%d info: %d\n", id, err); goto err_out; } + libbpf_mark_defined(name, info.name_len + 1); /* ignore non-module BTFs */ if (!info.kernel_btf || strcmp(name, "vmlinux") == 0) { 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 +#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 */
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@linux.ibm.com> --- 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(-)