* Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
2021-06-10 5:13 [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK zhihongx.peng
@ 2021-06-10 8:32 ` Bruce Richardson
2021-06-11 4:42 ` Lin, Xueqin
2021-06-10 9:12 ` Ananyev, Konstantin
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Bruce Richardson @ 2021-06-10 8:32 UTC (permalink / raw)
To: zhihongx.peng; +Cc: anatoly.burakov, stephen, dev, xueqin.lin
On Thu, Jun 10, 2021 at 01:13:52PM +0800, zhihongx.peng@intel.com wrote:
> From: Zhihong Peng <zhihongx.peng@intel.com>
>
> AddressSanitizer (ASan) is a google memory error detect
> standard tool. It could help to detect use-after-free and
> {heap,stack,global}-buffer overflow bugs in C/C++ programs,
> print detailed error information when error happens, large
> improve debug efficiency.
>
> By referring to its implementation algorithm
> (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> ported heap-buffer-overflow and use-after-freefunctions to dpdk.
>
> Here is an example of heap-buffer-overflow bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> p[7] = 'a';
> ......
>
> Here is an example of use-after-free bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> rte_free(p);
> *p = 'a';
> ......
>
> If you want to use this feature,
> you need to use the following compilation options:
> -Dc_args='-DRTE_MALLOC_ASAN'
> -Db_lundef=false -Db_sanitize=address
>
Rather than forcing the user to pass in the extra c_args, you can
automatically add it from the eal/meson.build files. Something like:
if get_option('b_sanitize').startswith('address'):
cflags += '-DRTE_MALLOC_ASAN'
endif
/Bruce
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
2021-06-10 8:32 ` Bruce Richardson
@ 2021-06-11 4:42 ` Lin, Xueqin
0 siblings, 0 replies; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-11 4:42 UTC (permalink / raw)
To: Richardson, Bruce, Peng, ZhihongX; +Cc: Burakov, Anatoly, stephen, dev
> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Thursday, June 10, 2021 4:33 PM
> To: Peng, ZhihongX <zhihongx.peng@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>;
> stephen@networkplumber.org; dev@dpdk.org; Lin, Xueqin
> <xueqin.lin@intel.com>
> Subject: Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
>
> On Thu, Jun 10, 2021 at 01:13:52PM +0800, zhihongx.peng@intel.com wrote:
> > From: Zhihong Peng <zhihongx.peng@intel.com>
> >
> > AddressSanitizer (ASan) is a google memory error detect standard tool.
> > It could help to detect use-after-free and {heap,stack,global}-buffer
> > overflow bugs in C/C++ programs, print detailed error information when
> > error happens, large improve debug efficiency.
> >
> > By referring to its implementation algorithm
> > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> > ported heap-buffer-overflow and use-after-freefunctions to dpdk.
> >
> > Here is an example of heap-buffer-overflow bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > p[7] = 'a';
> > ......
> >
> > Here is an example of use-after-free bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > rte_free(p);
> > *p = 'a';
> > ......
> >
> > If you want to use this feature,
> > you need to use the following compilation options:
> > -Dc_args='-DRTE_MALLOC_ASAN'
> > -Db_lundef=false -Db_sanitize=address
> >
> Rather than forcing the user to pass in the extra c_args, you can
> automatically add it from the eal/meson.build files. Something like:
>
> if get_option('b_sanitize').startswith('address'):
> cflags += '-DRTE_MALLOC_ASAN'
> endif
>
> /Bruce
Thanks Bruce for your review, really good suggestion for this part optimization, we will update it.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
2021-06-10 5:13 [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK zhihongx.peng
2021-06-10 8:32 ` Bruce Richardson
@ 2021-06-10 9:12 ` Ananyev, Konstantin
2021-06-11 4:49 ` Lin, Xueqin
2021-06-10 20:03 ` Stephen Hemminger
2021-06-15 8:12 ` [dpdk-dev] [RFC v2] " zhihongx.peng
3 siblings, 1 reply; 23+ messages in thread
From: Ananyev, Konstantin @ 2021-06-10 9:12 UTC (permalink / raw)
To: Peng, ZhihongX, Burakov, Anatoly, stephen
Cc: dev, Lin, Xueqin, Peng, ZhihongX
>
> From: Zhihong Peng <zhihongx.peng@intel.com>
>
> AddressSanitizer (ASan) is a google memory error detect
> standard tool. It could help to detect use-after-free and
> {heap,stack,global}-buffer overflow bugs in C/C++ programs,
> print detailed error information when error happens, large
> improve debug efficiency.
>
> By referring to its implementation algorithm
> (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> ported heap-buffer-overflow and use-after-freefunctions to dpdk.
>
> Here is an example of heap-buffer-overflow bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> p[7] = 'a';
> ......
>
> Here is an example of use-after-free bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> rte_free(p);
> *p = 'a';
> ......
>
> If you want to use this feature,
> you need to use the following compilation options:
> -Dc_args='-DRTE_MALLOC_ASAN'
> -Db_lundef=false -Db_sanitize=address
>
> Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
> Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
> ---
> lib/eal/common/malloc_elem.c | 33 +++++++-
> lib/eal/common/malloc_elem.h | 141 ++++++++++++++++++++++++++++++++++-
> lib/eal/common/malloc_heap.c | 19 +++++
> lib/eal/common/rte_malloc.c | 6 ++
> 4 files changed, 197 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
> index c2c9461f1..4a146b1b9 100644
> --- a/lib/eal/common/malloc_elem.c
> +++ b/lib/eal/common/malloc_elem.c
> @@ -446,6 +446,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
> struct malloc_elem *new_free_elem =
> RTE_PTR_ADD(new_elem, size + MALLOC_ELEM_OVERHEAD);
>
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_split_alloczone(new_free_elem);
> +#endif
Here and everywhere:
Instead of polluting code with all these ifdefs,
I think it would be better to move all these asan_*() functions into a separate *.h.
And have all these ifdefs inside it.
Something like that:
asan.h:
#ifdef RTE_MALLOC_ASAN
static inline void asan_clear_split_alloczone(...)
{
/* actual code */
}
....
#else
/* dummy one */
static inline void asan_clear_split_alloczone(...)
{
}
...
#endif
> split_elem(elem, new_free_elem);
> malloc_elem_free_list_insert(new_free_elem);
>
> @@ -458,6 +461,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
> elem->state = ELEM_BUSY;
> elem->pad = old_elem_size;
>
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_alloczone(elem);
> +#endif
> /* put a dummy header in padding, to point to real element header */
> if (elem->pad > 0) { /* pad will be at least 64-bytes, as everything
> * is cache-line aligned */
> @@ -475,7 +481,13 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
> * Re-insert original element, in case its new size makes it
> * belong on a different list.
> */
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_split_alloczone(new_elem);
> +#endif
> split_elem(elem, new_elem);
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_alloczone(new_elem);
> +#endif
> new_elem->state = ELEM_BUSY;
> malloc_elem_free_list_insert(elem);
>
> @@ -601,6 +613,9 @@ malloc_elem_hide_region(struct malloc_elem *elem, void *start, size_t len)
> if (next && next_elem_is_adjacent(elem)) {
> len_after = RTE_PTR_DIFF(next, hide_end);
> if (len_after >= MALLOC_ELEM_OVERHEAD + MIN_DATA_SIZE) {
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_split_alloczone(hide_end);
> +#endif
> /* split after */
> split_elem(elem, hide_end);
>
> @@ -615,6 +630,9 @@ malloc_elem_hide_region(struct malloc_elem *elem, void *start, size_t len)
> if (prev && prev_elem_is_adjacent(elem)) {
> len_before = RTE_PTR_DIFF(hide_start, elem);
> if (len_before >= MALLOC_ELEM_OVERHEAD + MIN_DATA_SIZE) {
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_split_alloczone(hide_start);
> +#endif
> /* split before */
> split_elem(elem, hide_start);
>
> @@ -628,6 +646,9 @@ malloc_elem_hide_region(struct malloc_elem *elem, void *start, size_t len)
> }
> }
>
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_alloczone(elem);
> +#endif
> remove_elem(elem);
> }
>
> @@ -641,8 +662,12 @@ malloc_elem_resize(struct malloc_elem *elem, size_t size)
> const size_t new_size = size + elem->pad + MALLOC_ELEM_OVERHEAD;
>
> /* if we request a smaller size, then always return ok */
> - if (elem->size >= new_size)
> + if (elem->size >= new_size) {
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_alloczone(elem);
> +#endif
> return 0;
> + }
>
> /* check if there is a next element, it's free and adjacent */
> if (!elem->next || elem->next->state != ELEM_FREE ||
> @@ -661,9 +686,15 @@ malloc_elem_resize(struct malloc_elem *elem, size_t size)
> /* now we have a big block together. Lets cut it down a bit, by splitting */
> struct malloc_elem *split_pt = RTE_PTR_ADD(elem, new_size);
> split_pt = RTE_PTR_ALIGN_CEIL(split_pt, RTE_CACHE_LINE_SIZE);
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_split_alloczone(split_pt);
> +#endif
> split_elem(elem, split_pt);
> malloc_elem_free_list_insert(split_pt);
> }
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_alloczone(elem);
> +#endif
> return 0;
> }
>
> diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
> index a1e5f7f02..d0d8bbb48 100644
> --- a/lib/eal/common/malloc_elem.h
> +++ b/lib/eal/common/malloc_elem.h
> @@ -36,10 +36,20 @@ struct malloc_elem {
> uint64_t header_cookie; /* Cookie marking start of data */
> /* trailer cookie at start + size */
> #endif
> +#ifdef RTE_MALLOC_ASAN
> + size_t user_size;
> + uint64_t asan_cookie[2]; /*must be next to header_cookie*/
> +#endif
> } __rte_cache_aligned;
>
> +static const unsigned MALLOC_ELEM_HEADER_LEN = sizeof(struct malloc_elem);
> +
> #ifndef RTE_MALLOC_DEBUG
> +#ifdef RTE_MALLOC_ASAN
> +static const unsigned MALLOC_ELEM_TRAILER_LEN = RTE_CACHE_LINE_SIZE;
> +#else
> static const unsigned MALLOC_ELEM_TRAILER_LEN = 0;
> +#endif
>
> /* dummy function - just check if pointer is non-null */
> static inline int
> @@ -90,9 +100,138 @@ malloc_elem_cookies_ok(const struct malloc_elem *elem)
>
> #endif
>
> -static const unsigned MALLOC_ELEM_HEADER_LEN = sizeof(struct malloc_elem);
> #define MALLOC_ELEM_OVERHEAD (MALLOC_ELEM_HEADER_LEN + MALLOC_ELEM_TRAILER_LEN)
>
> +#ifdef RTE_MALLOC_ASAN
> +
> +#define ASAN_SHADOW_GRAIN_SIZE 8
> +#define ASAN_MEM_FREE_FLAG 0xfd
> +#define ASAN_MEM_REDZONE_FLAG 0xfa
> +#define ASAN_MEM_TO_SHADOW(mem) (((mem) >> 3) + 0x00007fff8000)
> +
> +#if defined(__clang__)
> +__attribute__((no_sanitize("address", "hwaddress")))
> +#else
> +__attribute__((no_sanitize_address))
> +#endif
> +static inline void
> +asan_set_shadow(void *addr, char val)
> +{
> + *(char *)addr = val;
> +}
> +
> +static inline void
> +asan_set_zone(void *ptr, size_t len, uint32_t val)
> +{
> + size_t offset;
> + char *shadow;
> + size_t zone_len = len / ASAN_SHADOW_GRAIN_SIZE;
> + if (len % ASAN_SHADOW_GRAIN_SIZE != 0)
> + zone_len += 1;
> +
> + for (size_t i = 0; i < zone_len; i++) {
> + offset = i * ASAN_SHADOW_GRAIN_SIZE;
> + shadow = (char *)ASAN_MEM_TO_SHADOW(((int64_t)ptr + offset));
> + asan_set_shadow(shadow, val);
> + }
> +}
> +
> +/*
> + * When the memory is released, the release mark is
> + * set in the corresponding range of the shadow area.
> + */
> +static inline void
> +asan_set_freezone(void *ptr, size_t size)
> +{
> + asan_set_zone(ptr, size, ASAN_MEM_FREE_FLAG);
> +}
> +
> +/*
> + * When the memory is allocated, memory state must set accessible.
> + */
> +static inline void
> +asan_clear_alloczone(struct malloc_elem *elem)
> +{
> + asan_set_zone((void *)elem, elem->size, 0x0);
> +}
> +
> +static inline void
> +asan_clear_split_alloczone(struct malloc_elem *elem)
> +{
> + void *ptr = RTE_PTR_SUB(elem, MALLOC_ELEM_TRAILER_LEN);
> + asan_set_zone(ptr, MALLOC_ELEM_OVERHEAD, 0x0);
> +}
> +
> +/*
> + * When the memory is allocated, the memory boundary is
> + * marked in the corresponding range of the shadow area.
> + */
> +static inline void
> +asan_set_redzone(struct malloc_elem *elem, size_t user_size)
> +{
> + uint64_t ptr;
> + char *shadow;
> + if (elem != NULL) {
> + if (elem->state != ELEM_PAD)
> + elem = RTE_PTR_ADD(elem, elem->pad);
> +
> + elem->user_size = user_size;
> +
> + /* Set mark before the start of the allocated memory */
> + ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN)
> + - ASAN_SHADOW_GRAIN_SIZE;
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> + asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> + - ASAN_SHADOW_GRAIN_SIZE);
> + asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
> +
> + /* Set mark after the end of the allocated memory */
> + ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN
> + + elem->user_size);
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> + uint32_t val = (ptr % ASAN_SHADOW_GRAIN_SIZE);
> + val = (val == 0) ? ASAN_MEM_REDZONE_FLAG : val;
> + asan_set_shadow(shadow, val);
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> + + ASAN_SHADOW_GRAIN_SIZE);
> + asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
> + }
> +}
> +
> +/*
> + * When the memory is released, the mark of the memory boundary
> + * in the corresponding range of the shadow area is cleared.
> + */
> +static inline void
> +asan_clear_redzone(struct malloc_elem *elem)
> +{
> + uint64_t ptr;
> + char *shadow;
> + if (elem != NULL) {
> + elem = RTE_PTR_ADD(elem, elem->pad);
> +
> + /* Clear mark before the start of the allocated memory */
> + ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN)
> + - ASAN_SHADOW_GRAIN_SIZE;
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> + asan_set_shadow(shadow, 0x00);
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> + - ASAN_SHADOW_GRAIN_SIZE);
> + asan_set_shadow(shadow, 0x00);
> +
> + /* Clear mark after the end of the allocated memory */
> + ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN
> + + elem->user_size);
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> + asan_set_shadow(shadow, 0x00);
> + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> + + ASAN_SHADOW_GRAIN_SIZE);
> + asan_set_shadow(shadow, 0x00);
> + }
> +}
> +#endif
> +
> /*
> * Given a pointer to the start of a memory block returned by malloc, get
> * the actual malloc_elem header for that block.
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index ee400f38e..6d39549d3 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -238,6 +238,9 @@ heap_alloc(struct malloc_heap *heap, const char *type __rte_unused, size_t size,
> {
> struct malloc_elem *elem;
>
> +#ifdef RTE_MALLOC_ASAN
> + size_t user_size = size;
> +#endif
> size = RTE_CACHE_LINE_ROUNDUP(size);
> align = RTE_CACHE_LINE_ROUNDUP(align);
>
> @@ -250,6 +253,9 @@ heap_alloc(struct malloc_heap *heap, const char *type __rte_unused, size_t size,
>
> /* increase heap's count of allocated elements */
> heap->alloc_count++;
> +#ifdef RTE_MALLOC_ASAN
> + asan_set_redzone(elem, user_size);
> +#endif
> }
>
> return elem == NULL ? NULL : (void *)(&elem[1]);
> @@ -270,6 +276,9 @@ heap_alloc_biggest(struct malloc_heap *heap, const char *type __rte_unused,
>
> /* increase heap's count of allocated elements */
> heap->alloc_count++;
> +#ifdef RTE_MALLOC_ASAN
> + asan_set_redzone(elem, size);
> +#endif
> }
>
> return elem == NULL ? NULL : (void *)(&elem[1]);
> @@ -841,6 +850,9 @@ malloc_heap_free(struct malloc_elem *elem)
> if (!malloc_elem_cookies_ok(elem) || elem->state != ELEM_BUSY)
> return -1;
>
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_redzone(elem);
> +#endif
> /* elem may be merged with previous element, so keep heap address */
> heap = elem->heap;
> msl = elem->msl;
> @@ -848,6 +860,10 @@ malloc_heap_free(struct malloc_elem *elem)
>
> rte_spinlock_lock(&(heap->lock));
>
> +#ifdef RTE_MALLOC_ASAN
> + void *asan_ptr = RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN + elem->pad);
> + size_t asan_data_len = elem->size - MALLOC_ELEM_OVERHEAD - elem->pad;
> +#endif
> /* mark element as free */
> elem->state = ELEM_FREE;
>
> @@ -1001,6 +1017,9 @@ malloc_heap_free(struct malloc_elem *elem)
>
> rte_mcfg_mem_write_unlock();
> free_unlock:
> +#ifdef RTE_MALLOC_ASAN
> + asan_set_freezone(asan_ptr, asan_data_len);
> +#endif
> rte_spinlock_unlock(&(heap->lock));
> return ret;
> }
> diff --git a/lib/eal/common/rte_malloc.c b/lib/eal/common/rte_malloc.c
> index 9d39e58c0..fe70ee938 100644
> --- a/lib/eal/common/rte_malloc.c
> +++ b/lib/eal/common/rte_malloc.c
> @@ -170,6 +170,9 @@ rte_realloc_socket(void *ptr, size_t size, unsigned int align, int socket)
> RTE_LOG(ERR, EAL, "Error: memory corruption detected\n");
> return NULL;
> }
> +#ifdef RTE_MALLOC_ASAN
> + size_t user_size = size;
> +#endif
>
> size = RTE_CACHE_LINE_ROUNDUP(size), align = RTE_CACHE_LINE_ROUNDUP(align);
>
> @@ -181,6 +184,9 @@ rte_realloc_socket(void *ptr, size_t size, unsigned int align, int socket)
> RTE_PTR_ALIGN(ptr, align) == ptr &&
> malloc_heap_resize(elem, size) == 0) {
> rte_eal_trace_mem_realloc(size, align, socket, ptr);
> +#ifdef RTE_MALLOC_ASAN
> + asan_set_redzone(elem, user_size);
> +#endif
> return ptr;
> }
>
> --
> 2.17.1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
2021-06-10 9:12 ` Ananyev, Konstantin
@ 2021-06-11 4:49 ` Lin, Xueqin
0 siblings, 0 replies; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-11 4:49 UTC (permalink / raw)
To: Ananyev, Konstantin, Peng, ZhihongX, Burakov, Anatoly, stephen
Cc: dev, Peng, ZhihongX
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, June 10, 2021 5:12 PM
> To: Peng, ZhihongX <zhihongx.peng@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; stephen@networkplumber.org
> Cc: dev@dpdk.org; Lin, Xueqin <xueqin.lin@intel.com>; Peng, ZhihongX
> <zhihongx.peng@intel.com>
> Subject: RE: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
>
>
> >
> > From: Zhihong Peng <zhihongx.peng@intel.com>
> >
> > AddressSanitizer (ASan) is a google memory error detect standard tool.
> > It could help to detect use-after-free and {heap,stack,global}-buffer
> > overflow bugs in C/C++ programs, print detailed error information when
> > error happens, large improve debug efficiency.
> >
> > By referring to its implementation algorithm
> > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> > ported heap-buffer-overflow and use-after-freefunctions to dpdk.
> >
> > Here is an example of heap-buffer-overflow bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > p[7] = 'a';
> > ......
> >
> > Here is an example of use-after-free bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > rte_free(p);
> > *p = 'a';
> > ......
> >
> > If you want to use this feature,
> > you need to use the following compilation options:
> > -Dc_args='-DRTE_MALLOC_ASAN'
> > -Db_lundef=false -Db_sanitize=address
> >
> > Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
> > Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
> > ---
> > lib/eal/common/malloc_elem.c | 33 +++++++-
> > lib/eal/common/malloc_elem.h | 141
> ++++++++++++++++++++++++++++++++++-
> > lib/eal/common/malloc_heap.c | 19 +++++
> > lib/eal/common/rte_malloc.c | 6 ++
> > 4 files changed, 197 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/eal/common/malloc_elem.c
> > b/lib/eal/common/malloc_elem.c index c2c9461f1..4a146b1b9 100644
> > --- a/lib/eal/common/malloc_elem.c
> > +++ b/lib/eal/common/malloc_elem.c
> > @@ -446,6 +446,9 @@ malloc_elem_alloc(struct malloc_elem *elem,
> size_t size, unsigned align,
> > struct malloc_elem *new_free_elem =
> > RTE_PTR_ADD(new_elem, size +
> MALLOC_ELEM_OVERHEAD);
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_split_alloczone(new_free_elem);
> > +#endif
>
> Here and everywhere:
> Instead of polluting code with all these ifdefs, I think it would be better to
> move all these asan_*() functions into a separate *.h.
> And have all these ifdefs inside it.
> Something like that:
> asan.h:
>
> #ifdef RTE_MALLOC_ASAN
> static inline void asan_clear_split_alloczone(...) {
> /* actual code */
> }
> ....
> #else
> /* dummy one */
> static inline void asan_clear_split_alloczone(...) { } ...
> #endif
>
Good idea, we can improve it in v2, thanks.
> > split_elem(elem, new_free_elem);
> > malloc_elem_free_list_insert(new_free_elem);
> >
> > @@ -458,6 +461,9 @@ malloc_elem_alloc(struct malloc_elem *elem,
> size_t size, unsigned align,
> > elem->state = ELEM_BUSY;
> > elem->pad = old_elem_size;
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_alloczone(elem);
> > +#endif
> > /* put a dummy header in padding, to point to real element
> header */
> > if (elem->pad > 0) { /* pad will be at least 64-bytes, as
> everything
> > * is cache-line aligned */ @@ -475,7 +481,13
> > @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned
> align,
> > * Re-insert original element, in case its new size makes it
> > * belong on a different list.
> > */
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_split_alloczone(new_elem);
> > +#endif
> > split_elem(elem, new_elem);
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_alloczone(new_elem);
> > +#endif
> > new_elem->state = ELEM_BUSY;
> > malloc_elem_free_list_insert(elem);
> >
> > @@ -601,6 +613,9 @@ malloc_elem_hide_region(struct malloc_elem
> *elem, void *start, size_t len)
> > if (next && next_elem_is_adjacent(elem)) {
> > len_after = RTE_PTR_DIFF(next, hide_end);
> > if (len_after >= MALLOC_ELEM_OVERHEAD + MIN_DATA_SIZE)
> {
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_split_alloczone(hide_end);
> > +#endif
> > /* split after */
> > split_elem(elem, hide_end);
> >
> > @@ -615,6 +630,9 @@ malloc_elem_hide_region(struct malloc_elem
> *elem, void *start, size_t len)
> > if (prev && prev_elem_is_adjacent(elem)) {
> > len_before = RTE_PTR_DIFF(hide_start, elem);
> > if (len_before >= MALLOC_ELEM_OVERHEAD +
> MIN_DATA_SIZE) {
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_split_alloczone(hide_start);
> > +#endif
> > /* split before */
> > split_elem(elem, hide_start);
> >
> > @@ -628,6 +646,9 @@ malloc_elem_hide_region(struct malloc_elem
> *elem, void *start, size_t len)
> > }
> > }
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_alloczone(elem);
> > +#endif
> > remove_elem(elem);
> > }
> >
> > @@ -641,8 +662,12 @@ malloc_elem_resize(struct malloc_elem *elem,
> size_t size)
> > const size_t new_size = size + elem->pad +
> MALLOC_ELEM_OVERHEAD;
> >
> > /* if we request a smaller size, then always return ok */
> > - if (elem->size >= new_size)
> > + if (elem->size >= new_size) {
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_alloczone(elem);
> > +#endif
> > return 0;
> > + }
> >
> > /* check if there is a next element, it's free and adjacent */
> > if (!elem->next || elem->next->state != ELEM_FREE || @@ -661,9
> > +686,15 @@ malloc_elem_resize(struct malloc_elem *elem, size_t size)
> > /* now we have a big block together. Lets cut it down a bit,
> by splitting */
> > struct malloc_elem *split_pt = RTE_PTR_ADD(elem,
> new_size);
> > split_pt = RTE_PTR_ALIGN_CEIL(split_pt,
> RTE_CACHE_LINE_SIZE);
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_split_alloczone(split_pt);
> > +#endif
> > split_elem(elem, split_pt);
> > malloc_elem_free_list_insert(split_pt);
> > }
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_alloczone(elem);
> > +#endif
> > return 0;
> > }
> >
> > diff --git a/lib/eal/common/malloc_elem.h
> > b/lib/eal/common/malloc_elem.h index a1e5f7f02..d0d8bbb48 100644
> > --- a/lib/eal/common/malloc_elem.h
> > +++ b/lib/eal/common/malloc_elem.h
> > @@ -36,10 +36,20 @@ struct malloc_elem {
> > uint64_t header_cookie; /* Cookie marking start of data */
> > /* trailer cookie at start + size */
> > #endif
> > +#ifdef RTE_MALLOC_ASAN
> > + size_t user_size;
> > + uint64_t asan_cookie[2]; /*must be next to header_cookie*/ #endif
> > } __rte_cache_aligned;
> >
> > +static const unsigned MALLOC_ELEM_HEADER_LEN = sizeof(struct
> malloc_elem);
> > +
> > #ifndef RTE_MALLOC_DEBUG
> > +#ifdef RTE_MALLOC_ASAN
> > +static const unsigned MALLOC_ELEM_TRAILER_LEN =
> RTE_CACHE_LINE_SIZE;
> > +#else
> > static const unsigned MALLOC_ELEM_TRAILER_LEN = 0;
> > +#endif
> >
> > /* dummy function - just check if pointer is non-null */
> > static inline int
> > @@ -90,9 +100,138 @@ malloc_elem_cookies_ok(const struct
> malloc_elem *elem)
> >
> > #endif
> >
> > -static const unsigned MALLOC_ELEM_HEADER_LEN = sizeof(struct
> malloc_elem);
> > #define MALLOC_ELEM_OVERHEAD (MALLOC_ELEM_HEADER_LEN +
> MALLOC_ELEM_TRAILER_LEN)
> >
> > +#ifdef RTE_MALLOC_ASAN
> > +
> > +#define ASAN_SHADOW_GRAIN_SIZE 8
> > +#define ASAN_MEM_FREE_FLAG 0xfd
> > +#define ASAN_MEM_REDZONE_FLAG 0xfa
> > +#define ASAN_MEM_TO_SHADOW(mem) (((mem) >> 3) + 0x00007fff8000)
> > +
> > +#if defined(__clang__)
> > +__attribute__((no_sanitize("address", "hwaddress")))
> > +#else
> > +__attribute__((no_sanitize_address))
> > +#endif
> > +static inline void
> > +asan_set_shadow(void *addr, char val)
> > +{
> > + *(char *)addr = val;
> > +}
> > +
> > +static inline void
> > +asan_set_zone(void *ptr, size_t len, uint32_t val)
> > +{
> > + size_t offset;
> > + char *shadow;
> > + size_t zone_len = len / ASAN_SHADOW_GRAIN_SIZE;
> > + if (len % ASAN_SHADOW_GRAIN_SIZE != 0)
> > + zone_len += 1;
> > +
> > + for (size_t i = 0; i < zone_len; i++) {
> > + offset = i * ASAN_SHADOW_GRAIN_SIZE;
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(((int64_t)ptr +
> offset));
> > + asan_set_shadow(shadow, val);
> > + }
> > +}
> > +
> > +/*
> > + * When the memory is released, the release mark is
> > + * set in the corresponding range of the shadow area.
> > + */
> > +static inline void
> > +asan_set_freezone(void *ptr, size_t size)
> > +{
> > + asan_set_zone(ptr, size, ASAN_MEM_FREE_FLAG);
> > +}
> > +
> > +/*
> > + * When the memory is allocated, memory state must set accessible.
> > + */
> > +static inline void
> > +asan_clear_alloczone(struct malloc_elem *elem)
> > +{
> > + asan_set_zone((void *)elem, elem->size, 0x0);
> > +}
> > +
> > +static inline void
> > +asan_clear_split_alloczone(struct malloc_elem *elem)
> > +{
> > + void *ptr = RTE_PTR_SUB(elem, MALLOC_ELEM_TRAILER_LEN);
> > + asan_set_zone(ptr, MALLOC_ELEM_OVERHEAD, 0x0);
> > +}
> > +
> > +/*
> > + * When the memory is allocated, the memory boundary is
> > + * marked in the corresponding range of the shadow area.
> > + */
> > +static inline void
> > +asan_set_redzone(struct malloc_elem *elem, size_t user_size)
> > +{
> > + uint64_t ptr;
> > + char *shadow;
> > + if (elem != NULL) {
> > + if (elem->state != ELEM_PAD)
> > + elem = RTE_PTR_ADD(elem, elem->pad);
> > +
> > + elem->user_size = user_size;
> > +
> > + /* Set mark before the start of the allocated memory */
> > + ptr = (uint64_t)RTE_PTR_ADD(elem,
> MALLOC_ELEM_HEADER_LEN)
> > + - ASAN_SHADOW_GRAIN_SIZE;
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> > + asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> > + - ASAN_SHADOW_GRAIN_SIZE);
> > + asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
> > +
> > + /* Set mark after the end of the allocated memory */
> > + ptr = (uint64_t)RTE_PTR_ADD(elem,
> MALLOC_ELEM_HEADER_LEN
> > + + elem->user_size);
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> > + uint32_t val = (ptr % ASAN_SHADOW_GRAIN_SIZE);
> > + val = (val == 0) ? ASAN_MEM_REDZONE_FLAG : val;
> > + asan_set_shadow(shadow, val);
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> > + + ASAN_SHADOW_GRAIN_SIZE);
> > + asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
> > + }
> > +}
> > +
> > +/*
> > + * When the memory is released, the mark of the memory boundary
> > + * in the corresponding range of the shadow area is cleared.
> > + */
> > +static inline void
> > +asan_clear_redzone(struct malloc_elem *elem)
> > +{
> > + uint64_t ptr;
> > + char *shadow;
> > + if (elem != NULL) {
> > + elem = RTE_PTR_ADD(elem, elem->pad);
> > +
> > + /* Clear mark before the start of the allocated memory */
> > + ptr = (uint64_t)RTE_PTR_ADD(elem,
> MALLOC_ELEM_HEADER_LEN)
> > + - ASAN_SHADOW_GRAIN_SIZE;
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> > + asan_set_shadow(shadow, 0x00);
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> > + - ASAN_SHADOW_GRAIN_SIZE);
> > + asan_set_shadow(shadow, 0x00);
> > +
> > + /* Clear mark after the end of the allocated memory */
> > + ptr = (uint64_t)RTE_PTR_ADD(elem,
> MALLOC_ELEM_HEADER_LEN
> > + + elem->user_size);
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
> > + asan_set_shadow(shadow, 0x00);
> > + shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
> > + + ASAN_SHADOW_GRAIN_SIZE);
> > + asan_set_shadow(shadow, 0x00);
> > + }
> > +}
> > +#endif
> > +
> > /*
> > * Given a pointer to the start of a memory block returned by malloc, get
> > * the actual malloc_elem header for that block.
> > diff --git a/lib/eal/common/malloc_heap.c
> b/lib/eal/common/malloc_heap.c
> > index ee400f38e..6d39549d3 100644
> > --- a/lib/eal/common/malloc_heap.c
> > +++ b/lib/eal/common/malloc_heap.c
> > @@ -238,6 +238,9 @@ heap_alloc(struct malloc_heap *heap, const char
> *type __rte_unused, size_t size,
> > {
> > struct malloc_elem *elem;
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + size_t user_size = size;
> > +#endif
> > size = RTE_CACHE_LINE_ROUNDUP(size);
> > align = RTE_CACHE_LINE_ROUNDUP(align);
> >
> > @@ -250,6 +253,9 @@ heap_alloc(struct malloc_heap *heap, const char
> *type __rte_unused, size_t size,
> >
> > /* increase heap's count of allocated elements */
> > heap->alloc_count++;
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_set_redzone(elem, user_size);
> > +#endif
> > }
> >
> > return elem == NULL ? NULL : (void *)(&elem[1]);
> > @@ -270,6 +276,9 @@ heap_alloc_biggest(struct malloc_heap *heap,
> const char *type __rte_unused,
> >
> > /* increase heap's count of allocated elements */
> > heap->alloc_count++;
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_set_redzone(elem, size);
> > +#endif
> > }
> >
> > return elem == NULL ? NULL : (void *)(&elem[1]);
> > @@ -841,6 +850,9 @@ malloc_heap_free(struct malloc_elem *elem)
> > if (!malloc_elem_cookies_ok(elem) || elem->state != ELEM_BUSY)
> > return -1;
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_redzone(elem);
> > +#endif
> > /* elem may be merged with previous element, so keep heap
> address */
> > heap = elem->heap;
> > msl = elem->msl;
> > @@ -848,6 +860,10 @@ malloc_heap_free(struct malloc_elem *elem)
> >
> > rte_spinlock_lock(&(heap->lock));
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + void *asan_ptr = RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN
> + elem->pad);
> > + size_t asan_data_len = elem->size - MALLOC_ELEM_OVERHEAD -
> elem->pad;
> > +#endif
> > /* mark element as free */
> > elem->state = ELEM_FREE;
> >
> > @@ -1001,6 +1017,9 @@ malloc_heap_free(struct malloc_elem *elem)
> >
> > rte_mcfg_mem_write_unlock();
> > free_unlock:
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_set_freezone(asan_ptr, asan_data_len);
> > +#endif
> > rte_spinlock_unlock(&(heap->lock));
> > return ret;
> > }
> > diff --git a/lib/eal/common/rte_malloc.c b/lib/eal/common/rte_malloc.c
> > index 9d39e58c0..fe70ee938 100644
> > --- a/lib/eal/common/rte_malloc.c
> > +++ b/lib/eal/common/rte_malloc.c
> > @@ -170,6 +170,9 @@ rte_realloc_socket(void *ptr, size_t size, unsigned
> int align, int socket)
> > RTE_LOG(ERR, EAL, "Error: memory corruption detected\n");
> > return NULL;
> > }
> > +#ifdef RTE_MALLOC_ASAN
> > + size_t user_size = size;
> > +#endif
> >
> > size = RTE_CACHE_LINE_ROUNDUP(size), align =
> RTE_CACHE_LINE_ROUNDUP(align);
> >
> > @@ -181,6 +184,9 @@ rte_realloc_socket(void *ptr, size_t size, unsigned
> int align, int socket)
> > RTE_PTR_ALIGN(ptr, align) == ptr &&
> > malloc_heap_resize(elem, size) == 0) {
> > rte_eal_trace_mem_realloc(size, align, socket, ptr);
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_set_redzone(elem, user_size);
> > +#endif
> > return ptr;
> > }
> >
> > --
> > 2.17.1
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
2021-06-10 5:13 [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK zhihongx.peng
2021-06-10 8:32 ` Bruce Richardson
2021-06-10 9:12 ` Ananyev, Konstantin
@ 2021-06-10 20:03 ` Stephen Hemminger
2021-06-11 6:15 ` Lin, Xueqin
2021-06-15 8:12 ` [dpdk-dev] [RFC v2] " zhihongx.peng
3 siblings, 1 reply; 23+ messages in thread
From: Stephen Hemminger @ 2021-06-10 20:03 UTC (permalink / raw)
To: zhihongx.peng; +Cc: anatoly.burakov, dev, xueqin.lin
On Thu, 10 Jun 2021 13:13:52 +0800
zhihongx.peng@intel.com wrote:
> From: Zhihong Peng <zhihongx.peng@intel.com>
>
> AddressSanitizer (ASan) is a google memory error detect
> standard tool. It could help to detect use-after-free and
> {heap,stack,global}-buffer overflow bugs in C/C++ programs,
> print detailed error information when error happens, large
> improve debug efficiency.
>
> By referring to its implementation algorithm
> (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> ported heap-buffer-overflow and use-after-freefunctions to dpdk.
>
> Here is an example of heap-buffer-overflow bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> p[7] = 'a';
> ......
>
> Here is an example of use-after-free bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> rte_free(p);
> *p = 'a';
> ......
>
> If you want to use this feature,
> you need to use the following compilation options:
> -Dc_args='-DRTE_MALLOC_ASAN'
> -Db_lundef=false -Db_sanitize=address
>
> Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
> Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
> ---
> lib/eal/common/malloc_elem.c | 33 +++++++-
> lib/eal/common/malloc_elem.h | 141 ++++++++++++++++++++++++++++++++++-
> lib/eal/common/malloc_heap.c | 19 +++++
> lib/eal/common/rte_malloc.c | 6 ++
> 4 files changed, 197 insertions(+), 2 deletions(-)
>
> diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
> index c2c9461f1..4a146b1b9 100644
> --- a/lib/eal/common/malloc_elem.c
> +++ b/lib/eal/common/malloc_elem.c
> @@ -446,6 +446,9 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
> struct malloc_elem *new_free_elem =
> RTE_PTR_ADD(new_elem, size + MALLOC_ELEM_OVERHEAD);
>
> +#ifdef RTE_MALLOC_ASAN
> + asan_clear_split_alloczone(new_free_elem);
> +#endif
Two things:
ASAN should be detected using standard compiler flags, not a DPDK option.
GCC uses __SANITIZE_ADDRESS__ and Clang uses feature macro.
Rather than littering DPDK code with ifdefs' a better method is to
do define stub inline (or macros if you insist) in the header file.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK
2021-06-10 20:03 ` Stephen Hemminger
@ 2021-06-11 6:15 ` Lin, Xueqin
0 siblings, 0 replies; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-11 6:15 UTC (permalink / raw)
To: Stephen Hemminger, Peng, ZhihongX; +Cc: Burakov, Anatoly, dev
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, June 11, 2021 4:03 AM
> To: Peng, ZhihongX <zhihongx.peng@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; dev@dpdk.org; Lin,
> Xueqin <xueqin.lin@intel.com>
> Subject: Re: [RFC] porting AddressSanitizer feature to DPDK
>
> On Thu, 10 Jun 2021 13:13:52 +0800
> zhihongx.peng@intel.com wrote:
>
> > From: Zhihong Peng <zhihongx.peng@intel.com>
> >
> > AddressSanitizer (ASan) is a google memory error detect standard tool.
> > It could help to detect use-after-free and {heap,stack,global}-buffer
> > overflow bugs in C/C++ programs, print detailed error information when
> > error happens, large improve debug efficiency.
> >
> > By referring to its implementation algorithm
> > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> > ported heap-buffer-overflow and use-after-freefunctions to dpdk.
> >
> > Here is an example of heap-buffer-overflow bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > p[7] = 'a';
> > ......
> >
> > Here is an example of use-after-free bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > rte_free(p);
> > *p = 'a';
> > ......
> >
> > If you want to use this feature,
> > you need to use the following compilation options:
> > -Dc_args='-DRTE_MALLOC_ASAN'
> > -Db_lundef=false -Db_sanitize=address
> >
> > Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
> > Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
> > ---
> > lib/eal/common/malloc_elem.c | 33 +++++++-
> > lib/eal/common/malloc_elem.h | 141
> ++++++++++++++++++++++++++++++++++-
> > lib/eal/common/malloc_heap.c | 19 +++++
> > lib/eal/common/rte_malloc.c | 6 ++
> > 4 files changed, 197 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/eal/common/malloc_elem.c
> > b/lib/eal/common/malloc_elem.c index c2c9461f1..4a146b1b9 100644
> > --- a/lib/eal/common/malloc_elem.c
> > +++ b/lib/eal/common/malloc_elem.c
> > @@ -446,6 +446,9 @@ malloc_elem_alloc(struct malloc_elem *elem,
> size_t size, unsigned align,
> > struct malloc_elem *new_free_elem =
> > RTE_PTR_ADD(new_elem, size +
> MALLOC_ELEM_OVERHEAD);
> >
> > +#ifdef RTE_MALLOC_ASAN
> > + asan_clear_split_alloczone(new_free_elem);
> > +#endif
>
>
> Two things:
> ASAN should be detected using standard compiler flags, not a DPDK option.
> GCC uses __SANITIZE_ADDRESS__ and Clang uses feature macro.
Thanks Stephen for your review and suggestion, we will improve this part.
Only use Asan standard compiler flags, remove DPDK option for the tool detect.
>
> Rather than littering DPDK code with ifdefs' a better method is to do define
> stub inline (or macros if you insist) in the header file.
Good capture, we will improve it in V2.
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-10 5:13 [dpdk-dev] [RFC] porting AddressSanitizer feature to DPDK zhihongx.peng
` (2 preceding siblings ...)
2021-06-10 20:03 ` Stephen Hemminger
@ 2021-06-15 8:12 ` zhihongx.peng
2021-06-15 8:40 ` Jerin Jacob
2021-07-06 20:40 ` David Christensen
3 siblings, 2 replies; 23+ messages in thread
From: zhihongx.peng @ 2021-06-15 8:12 UTC (permalink / raw)
To: anatoly.burakov, konstantin.ananyev, stephen
Cc: dev, xueqin.lin, Zhihong Peng
From: Zhihong Peng <zhihongx.peng@intel.com>
AddressSanitizer (ASan) is a google memory error detect
standard tool. It could help to detect use-after-free and
{heap,stack,global}-buffer overflow bugs in C/C++ programs,
print detailed error information when error happens, large
improve debug efficiency.
By referring to its implementation algorithm
(https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
ported heap-buffer-overflow and use-after-freefunctions to dpdk.
Here is an example of heap-buffer-overflow bug:
......
char *p = rte_zmalloc(NULL, 7, 0);
p[7] = 'a';
......
Here is an example of use-after-free bug:
......
char *p = rte_zmalloc(NULL, 7, 0);
rte_free(p);
*p = 'a';
......
If you want to use this feature,
you need to use the following compilation options:
-Db_lundef=false -Db_sanitize=address
Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
---
lib/eal/common/malloc_elem.c | 26 +++++-
lib/eal/common/malloc_elem.h | 159 ++++++++++++++++++++++++++++++++++-
lib/eal/common/malloc_heap.c | 12 +++
lib/eal/common/meson.build | 4 +
lib/eal/common/rte_malloc.c | 7 ++
5 files changed, 206 insertions(+), 2 deletions(-)
diff --git a/lib/eal/common/malloc_elem.c b/lib/eal/common/malloc_elem.c
index c2c9461f1..bdd20a162 100644
--- a/lib/eal/common/malloc_elem.c
+++ b/lib/eal/common/malloc_elem.c
@@ -446,6 +446,8 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
struct malloc_elem *new_free_elem =
RTE_PTR_ADD(new_elem, size + MALLOC_ELEM_OVERHEAD);
+ asan_clear_split_alloczone(new_free_elem);
+
split_elem(elem, new_free_elem);
malloc_elem_free_list_insert(new_free_elem);
@@ -458,6 +460,8 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
elem->state = ELEM_BUSY;
elem->pad = old_elem_size;
+ asan_clear_alloczone(elem);
+
/* put a dummy header in padding, to point to real element header */
if (elem->pad > 0) { /* pad will be at least 64-bytes, as everything
* is cache-line aligned */
@@ -470,12 +474,18 @@ malloc_elem_alloc(struct malloc_elem *elem, size_t size, unsigned align,
return new_elem;
}
+ asan_clear_split_alloczone(new_elem);
+
/* we are going to split the element in two. The original element
* remains free, and the new element is the one allocated.
* Re-insert original element, in case its new size makes it
* belong on a different list.
*/
+
split_elem(elem, new_elem);
+
+ asan_clear_alloczone(new_elem);
+
new_elem->state = ELEM_BUSY;
malloc_elem_free_list_insert(elem);
@@ -601,6 +611,8 @@ malloc_elem_hide_region(struct malloc_elem *elem, void *start, size_t len)
if (next && next_elem_is_adjacent(elem)) {
len_after = RTE_PTR_DIFF(next, hide_end);
if (len_after >= MALLOC_ELEM_OVERHEAD + MIN_DATA_SIZE) {
+ asan_clear_split_alloczone(hide_end);
+
/* split after */
split_elem(elem, hide_end);
@@ -615,6 +627,8 @@ malloc_elem_hide_region(struct malloc_elem *elem, void *start, size_t len)
if (prev && prev_elem_is_adjacent(elem)) {
len_before = RTE_PTR_DIFF(hide_start, elem);
if (len_before >= MALLOC_ELEM_OVERHEAD + MIN_DATA_SIZE) {
+ asan_clear_split_alloczone(hide_start);
+
/* split before */
split_elem(elem, hide_start);
@@ -628,6 +642,8 @@ malloc_elem_hide_region(struct malloc_elem *elem, void *start, size_t len)
}
}
+ asan_clear_alloczone(elem);
+
remove_elem(elem);
}
@@ -641,8 +657,10 @@ malloc_elem_resize(struct malloc_elem *elem, size_t size)
const size_t new_size = size + elem->pad + MALLOC_ELEM_OVERHEAD;
/* if we request a smaller size, then always return ok */
- if (elem->size >= new_size)
+ if (elem->size >= new_size) {
+ asan_clear_alloczone(elem);
return 0;
+ }
/* check if there is a next element, it's free and adjacent */
if (!elem->next || elem->next->state != ELEM_FREE ||
@@ -661,9 +679,15 @@ malloc_elem_resize(struct malloc_elem *elem, size_t size)
/* now we have a big block together. Lets cut it down a bit, by splitting */
struct malloc_elem *split_pt = RTE_PTR_ADD(elem, new_size);
split_pt = RTE_PTR_ALIGN_CEIL(split_pt, RTE_CACHE_LINE_SIZE);
+
+ asan_clear_split_alloczone(split_pt);
+
split_elem(elem, split_pt);
malloc_elem_free_list_insert(split_pt);
}
+
+ asan_clear_alloczone(elem);
+
return 0;
}
diff --git a/lib/eal/common/malloc_elem.h b/lib/eal/common/malloc_elem.h
index a1e5f7f02..365625c83 100644
--- a/lib/eal/common/malloc_elem.h
+++ b/lib/eal/common/malloc_elem.h
@@ -36,10 +36,20 @@ struct malloc_elem {
uint64_t header_cookie; /* Cookie marking start of data */
/* trailer cookie at start + size */
#endif
+#ifdef RTE_MALLOC_ASAN
+ size_t user_size;
+ uint64_t asan_cookie[2]; /*must be next to header_cookie*/
+#endif
} __rte_cache_aligned;
+static const unsigned MALLOC_ELEM_HEADER_LEN = sizeof(struct malloc_elem);
+
#ifndef RTE_MALLOC_DEBUG
+#ifdef RTE_MALLOC_ASAN
+static const unsigned MALLOC_ELEM_TRAILER_LEN = RTE_CACHE_LINE_SIZE;
+#else
static const unsigned MALLOC_ELEM_TRAILER_LEN = 0;
+#endif
/* dummy function - just check if pointer is non-null */
static inline int
@@ -90,9 +100,156 @@ malloc_elem_cookies_ok(const struct malloc_elem *elem)
#endif
-static const unsigned MALLOC_ELEM_HEADER_LEN = sizeof(struct malloc_elem);
#define MALLOC_ELEM_OVERHEAD (MALLOC_ELEM_HEADER_LEN + MALLOC_ELEM_TRAILER_LEN)
+#ifdef RTE_MALLOC_ASAN
+
+#define ASAN_SHADOW_GRAIN_SIZE 8
+#define ASAN_MEM_FREE_FLAG 0xfd
+#define ASAN_MEM_REDZONE_FLAG 0xfa
+#define ASAN_MEM_TO_SHADOW(mem) (((mem) >> 3) + 0x00007fff8000)
+
+#if defined(__clang__)
+__attribute__((no_sanitize("address", "hwaddress")))
+#else
+__attribute__((no_sanitize_address))
+#endif
+static inline void
+asan_set_shadow(void *addr, char val)
+{
+ *(char *)addr = val;
+}
+
+static inline void
+asan_set_zone(void *ptr, size_t len, uint32_t val)
+{
+ size_t offset;
+ char *shadow;
+ size_t zone_len = len / ASAN_SHADOW_GRAIN_SIZE;
+ if (len % ASAN_SHADOW_GRAIN_SIZE != 0)
+ zone_len += 1;
+
+ for (size_t i = 0; i < zone_len; i++) {
+ offset = i * ASAN_SHADOW_GRAIN_SIZE;
+ shadow = (char *)ASAN_MEM_TO_SHADOW(((int64_t)ptr + offset));
+ asan_set_shadow(shadow, val);
+ }
+}
+
+/*
+ * When the memory is released, the release mark is
+ * set in the corresponding range of the shadow area.
+ */
+static inline void
+asan_set_freezone(void *ptr, size_t size)
+{
+ asan_set_zone(ptr, size, ASAN_MEM_FREE_FLAG);
+}
+
+/*
+ * When the memory is allocated, memory state must set accessible.
+ */
+static inline void
+asan_clear_alloczone(struct malloc_elem *elem)
+{
+ asan_set_zone((void *)elem, elem->size, 0x0);
+}
+
+static inline void
+asan_clear_split_alloczone(struct malloc_elem *elem)
+{
+ void *ptr = RTE_PTR_SUB(elem, MALLOC_ELEM_TRAILER_LEN);
+ asan_set_zone(ptr, MALLOC_ELEM_OVERHEAD, 0x0);
+}
+
+/*
+ * When the memory is allocated, the memory boundary is
+ * marked in the corresponding range of the shadow area.
+ */
+static inline void
+asan_set_redzone(struct malloc_elem *elem, size_t user_size)
+{
+ uint64_t ptr;
+ char *shadow;
+ if (elem != NULL) {
+ if (elem->state != ELEM_PAD)
+ elem = RTE_PTR_ADD(elem, elem->pad);
+
+ elem->user_size = user_size;
+
+ /* Set mark before the start of the allocated memory */
+ ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN)
+ - ASAN_SHADOW_GRAIN_SIZE;
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
+ asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
+ - ASAN_SHADOW_GRAIN_SIZE);
+ asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
+
+ /* Set mark after the end of the allocated memory */
+ ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN
+ + elem->user_size);
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
+ uint32_t val = (ptr % ASAN_SHADOW_GRAIN_SIZE);
+ val = (val == 0) ? ASAN_MEM_REDZONE_FLAG : val;
+ asan_set_shadow(shadow, val);
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
+ + ASAN_SHADOW_GRAIN_SIZE);
+ asan_set_shadow(shadow, ASAN_MEM_REDZONE_FLAG);
+ }
+}
+
+/*
+ * When the memory is released, the mark of the memory boundary
+ * in the corresponding range of the shadow area is cleared.
+ */
+static inline void
+asan_clear_redzone(struct malloc_elem *elem)
+{
+ uint64_t ptr;
+ char *shadow;
+ if (elem != NULL) {
+ elem = RTE_PTR_ADD(elem, elem->pad);
+
+ /* Clear mark before the start of the allocated memory */
+ ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN)
+ - ASAN_SHADOW_GRAIN_SIZE;
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
+ asan_set_shadow(shadow, 0x00);
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
+ - ASAN_SHADOW_GRAIN_SIZE);
+ asan_set_shadow(shadow, 0x00);
+
+ /* Clear mark after the end of the allocated memory */
+ ptr = (uint64_t)RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN
+ + elem->user_size);
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr);
+ asan_set_shadow(shadow, 0x00);
+ shadow = (char *)ASAN_MEM_TO_SHADOW(ptr
+ + ASAN_SHADOW_GRAIN_SIZE);
+ asan_set_shadow(shadow, 0x00);
+ }
+}
+
+#else
+static inline void
+asan_set_freezone(void *ptr __rte_unused, size_t size __rte_unused) { }
+
+static inline void
+asan_clear_alloczone(struct malloc_elem *elem __rte_unused) { }
+
+static inline void
+asan_clear_split_alloczone(struct malloc_elem *elem __rte_unused) { }
+
+static inline void
+asan_set_redzone(struct malloc_elem *elem __rte_unused,
+ size_t user_size __rte_unused) { }
+
+static inline void
+asan_clear_redzone(struct malloc_elem *elem __rte_unused) { }
+
+#endif
+
/*
* Given a pointer to the start of a memory block returned by malloc, get
* the actual malloc_elem header for that block.
diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
index ee400f38e..775d6789d 100644
--- a/lib/eal/common/malloc_heap.c
+++ b/lib/eal/common/malloc_heap.c
@@ -237,6 +237,7 @@ heap_alloc(struct malloc_heap *heap, const char *type __rte_unused, size_t size,
unsigned int flags, size_t align, size_t bound, bool contig)
{
struct malloc_elem *elem;
+ size_t user_size = size;
size = RTE_CACHE_LINE_ROUNDUP(size);
align = RTE_CACHE_LINE_ROUNDUP(align);
@@ -250,6 +251,8 @@ heap_alloc(struct malloc_heap *heap, const char *type __rte_unused, size_t size,
/* increase heap's count of allocated elements */
heap->alloc_count++;
+
+ asan_set_redzone(elem, user_size);
}
return elem == NULL ? NULL : (void *)(&elem[1]);
@@ -270,6 +273,8 @@ heap_alloc_biggest(struct malloc_heap *heap, const char *type __rte_unused,
/* increase heap's count of allocated elements */
heap->alloc_count++;
+
+ asan_set_redzone(elem, size);
}
return elem == NULL ? NULL : (void *)(&elem[1]);
@@ -841,6 +846,8 @@ malloc_heap_free(struct malloc_elem *elem)
if (!malloc_elem_cookies_ok(elem) || elem->state != ELEM_BUSY)
return -1;
+ asan_clear_redzone(elem);
+
/* elem may be merged with previous element, so keep heap address */
heap = elem->heap;
msl = elem->msl;
@@ -848,6 +855,9 @@ malloc_heap_free(struct malloc_elem *elem)
rte_spinlock_lock(&(heap->lock));
+ void *asan_ptr = RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN + elem->pad);
+ size_t asan_data_len = elem->size - MALLOC_ELEM_OVERHEAD - elem->pad;
+
/* mark element as free */
elem->state = ELEM_FREE;
@@ -1001,6 +1011,8 @@ malloc_heap_free(struct malloc_elem *elem)
rte_mcfg_mem_write_unlock();
free_unlock:
+ asan_set_freezone(asan_ptr, asan_data_len);
+
rte_spinlock_unlock(&(heap->lock));
return ret;
}
diff --git a/lib/eal/common/meson.build b/lib/eal/common/meson.build
index edfca7777..2f786841d 100644
--- a/lib/eal/common/meson.build
+++ b/lib/eal/common/meson.build
@@ -5,6 +5,10 @@ includes += include_directories('.')
cflags += [ '-DABI_VERSION="@0@"'.format(abi_version) ]
+if get_option('b_sanitize').startswith('address')
+ cflags += '-DRTE_MALLOC_ASAN'
+endif
+
if is_windows
sources += files(
'eal_common_bus.c',
diff --git a/lib/eal/common/rte_malloc.c b/lib/eal/common/rte_malloc.c
index 9d39e58c0..712bcbfce 100644
--- a/lib/eal/common/rte_malloc.c
+++ b/lib/eal/common/rte_malloc.c
@@ -162,6 +162,8 @@ rte_calloc(const char *type, size_t num, size_t size, unsigned align)
void *
rte_realloc_socket(void *ptr, size_t size, unsigned int align, int socket)
{
+ size_t user_size;
+
if (ptr == NULL)
return rte_malloc_socket(NULL, size, align, socket);
@@ -171,6 +173,8 @@ rte_realloc_socket(void *ptr, size_t size, unsigned int align, int socket)
return NULL;
}
+ user_size = size;
+
size = RTE_CACHE_LINE_ROUNDUP(size), align = RTE_CACHE_LINE_ROUNDUP(align);
/* check requested socket id and alignment matches first, and if ok,
@@ -181,6 +185,9 @@ rte_realloc_socket(void *ptr, size_t size, unsigned int align, int socket)
RTE_PTR_ALIGN(ptr, align) == ptr &&
malloc_heap_resize(elem, size) == 0) {
rte_eal_trace_mem_realloc(size, align, socket, ptr);
+
+ asan_set_redzone(elem, user_size);
+
return ptr;
}
--
2.17.1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-15 8:12 ` [dpdk-dev] [RFC v2] " zhihongx.peng
@ 2021-06-15 8:40 ` Jerin Jacob
2021-06-16 9:13 ` Lin, Xueqin
2021-07-06 20:40 ` David Christensen
1 sibling, 1 reply; 23+ messages in thread
From: Jerin Jacob @ 2021-06-15 8:40 UTC (permalink / raw)
To: zhihongx.peng
Cc: Anatoly Burakov, Ananyev, Konstantin, Stephen Hemminger,
dpdk-dev, xueqin.lin
On Tue, Jun 15, 2021 at 1:46 PM <zhihongx.peng@intel.com> wrote:
>
> From: Zhihong Peng <zhihongx.peng@intel.com>
>
> AddressSanitizer (ASan) is a google memory error detect
> standard tool. It could help to detect use-after-free and
> {heap,stack,global}-buffer overflow bugs in C/C++ programs,
> print detailed error information when error happens, large
> improve debug efficiency.
>
> By referring to its implementation algorithm
> (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> ported heap-buffer-overflow and use-after-freefunctions to dpdk.
>
> Here is an example of heap-buffer-overflow bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> p[7] = 'a';
> ......
>
> Here is an example of use-after-free bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> rte_free(p);
> *p = 'a';
> ......
>
> If you want to use this feature,
> you need to use the following compilation options:
> -Db_lundef=false -Db_sanitize=address
# Thanks for this patch. It is a useful item.
# Subject could be changed
from:
porting AddressSanitizer feature to DPDK
to
eal: support for AddressSanitizer
or so
# Could you add a section in the documentation for Sanitizers to
document the build time option and other points that users need to
know.
We can add other sanitizers such as UBSan etc in the future here
# Add a UT test case to make sure it is working in app/test or so.
# Also, Please update the release note for this feature.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-15 8:40 ` Jerin Jacob
@ 2021-06-16 9:13 ` Lin, Xueqin
2021-06-16 11:34 ` Jerin Jacob
0 siblings, 1 reply; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-16 9:13 UTC (permalink / raw)
To: Jerin Jacob, Peng, ZhihongX
Cc: Burakov, Anatoly, Ananyev, Konstantin, Stephen Hemminger, dpdk-dev
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, June 15, 2021 4:40 PM
> To: Peng, ZhihongX <zhihongx.peng@intel.com>
> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>; Lin, Xueqin
> <xueqin.lin@intel.com>
> Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
>
> On Tue, Jun 15, 2021 at 1:46 PM <zhihongx.peng@intel.com> wrote:
> >
> > From: Zhihong Peng <zhihongx.peng@intel.com>
> >
> > AddressSanitizer (ASan) is a google memory error detect standard tool.
> > It could help to detect use-after-free and {heap,stack,global}-buffer
> > overflow bugs in C/C++ programs, print detailed error information when
> > error happens, large improve debug efficiency.
> >
> > By referring to its implementation algorithm
> > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> > ported heap-buffer-overflow and use-after-freefunctions to dpdk.
> >
> > Here is an example of heap-buffer-overflow bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > p[7] = 'a';
> > ......
> >
> > Here is an example of use-after-free bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > rte_free(p);
> > *p = 'a';
> > ......
> >
> > If you want to use this feature,
> > you need to use the following compilation options:
> > -Db_lundef=false -Db_sanitize=address
>
> # Thanks for this patch. It is a useful item.
>
> # Subject could be changed
> from:
> porting AddressSanitizer feature to DPDK to
> eal: support for AddressSanitizer
> or so
Thanks for your positive feedback and review.
Good point, we will update the title in next version.
>
> # Could you add a section in the documentation for Sanitizers to document the
> build time option and other points that users need to know.
Make sense to add build option and key points to document, will add this part in doc folder.
> We can add other sanitizers such as UBSan etc in the future here
WIP to research other sanitizer tool.
>
> # Add a UT test case to make sure it is working in app/test or so.
This tool could help to detect memory issue, need to change bad code to check if working.
Suggest listing demo code and tool capture information for user to try if tool works, also add this part into doc.
>
> # Also, Please update the release note for this feature.
Sure, we can update the release note if code merge.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-16 9:13 ` Lin, Xueqin
@ 2021-06-16 11:34 ` Jerin Jacob
2021-06-18 7:48 ` Lin, Xueqin
0 siblings, 1 reply; 23+ messages in thread
From: Jerin Jacob @ 2021-06-16 11:34 UTC (permalink / raw)
To: Lin, Xueqin
Cc: Peng, ZhihongX, Burakov, Anatoly, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
On Wed, Jun 16, 2021 at 2:43 PM Lin, Xueqin <xueqin.lin@intel.com> wrote:
>
> > -----Original Message-----
> > From: Jerin Jacob <jerinjacobk@gmail.com>
> > Sent: Tuesday, June 15, 2021 4:40 PM
> > To: Peng, ZhihongX <zhihongx.peng@intel.com>
> > Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; Ananyev, Konstantin
> > <konstantin.ananyev@intel.com>; Stephen Hemminger
> > <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>; Lin, Xueqin
> > <xueqin.lin@intel.com>
> > Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
> >
> > On Tue, Jun 15, 2021 at 1:46 PM <zhihongx.peng@intel.com> wrote:
> > >
> > > From: Zhihong Peng <zhihongx.peng@intel.com>
> > >
> > > AddressSanitizer (ASan) is a google memory error detect standard tool.
> > > It could help to detect use-after-free and {heap,stack,global}-buffer
> > > overflow bugs in C/C++ programs, print detailed error information when
> > > error happens, large improve debug efficiency.
> > >
> > > By referring to its implementation algorithm
> > > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> > > ported heap-buffer-overflow and use-after-freefunctions to dpdk.
> > >
> > > Here is an example of heap-buffer-overflow bug:
> > > ......
> > > char *p = rte_zmalloc(NULL, 7, 0);
> > > p[7] = 'a';
> > > ......
> > >
> > > Here is an example of use-after-free bug:
> > > ......
> > > char *p = rte_zmalloc(NULL, 7, 0);
> > > rte_free(p);
> > > *p = 'a';
> > > ......
> > >
> > > If you want to use this feature,
> > > you need to use the following compilation options:
> > > -Db_lundef=false -Db_sanitize=address
> >
> > # Thanks for this patch. It is a useful item.
> >
> > # Subject could be changed
> > from:
> > porting AddressSanitizer feature to DPDK to
> > eal: support for AddressSanitizer
> > or so
>
> Thanks for your positive feedback and review.
> Good point, we will update the title in next version.
>
> >
> > # Could you add a section in the documentation for Sanitizers to document the
> > build time option and other points that users need to know.
>
> Make sense to add build option and key points to document, will add this part in doc
folder.
>
> > We can add other sanitizers such as UBSan etc in the future here
> WIP to research other sanitizer tool.
UBsan is a good candate.
Some old DPDK patch for the same:
http://patches.dpdk.org/project/dpdk/patch/1573832013-18946-1-git-send-email-hkalra@marvell.com/
> >
> > # Add a UT test case to make sure it is working in app/test or so.
>
> This tool could help to detect memory issue, need to change bad code to check if working.
It is better to have a UT to test things are working. You could add it in
app/test/test_address_sanity.c. Tests can be such that
- Skip if not complied with Sanity enabled
- Pass if the code detects the known bad code. You can have test cases
with pubic rte_ API
that internally exercise the verify your implementation related to
new asan_* APIs.
> Suggest listing demo code and tool capture information for user to try if tool works, also add this part into doc.
>
> >
> > # Also, Please update the release note for this feature.
> Sure, we can update the release note if code merge.
Probably you can send v1 version next i.e change the RFC status to get merged.
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-16 11:34 ` Jerin Jacob
@ 2021-06-18 7:48 ` Lin, Xueqin
2021-06-18 9:04 ` David Marchand
0 siblings, 1 reply; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-18 7:48 UTC (permalink / raw)
To: Jerin Jacob
Cc: Peng, ZhihongX, Burakov, Anatoly, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Wednesday, June 16, 2021 7:35 PM
> To: Lin, Xueqin <xueqin.lin@intel.com>
> Cc: Peng, ZhihongX <zhihongx.peng@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
>
> On Wed, Jun 16, 2021 at 2:43 PM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Tuesday, June 15, 2021 4:40 PM
> > > To: Peng, ZhihongX <zhihongx.peng@intel.com>
> > > Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; Ananyev,
> > > Konstantin <konstantin.ananyev@intel.com>; Stephen Hemminger
> > > <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>; Lin,
> Xueqin
> > > <xueqin.lin@intel.com>
> > > Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to
> > > DPDK
> > >
> > > On Tue, Jun 15, 2021 at 1:46 PM <zhihongx.peng@intel.com> wrote:
> > > >
> > > > From: Zhihong Peng <zhihongx.peng@intel.com>
> > > >
> > > > AddressSanitizer (ASan) is a google memory error detect standard tool.
> > > > It could help to detect use-after-free and
> > > > {heap,stack,global}-buffer overflow bugs in C/C++ programs, print
> > > > detailed error information when error happens, large improve debug
> efficiency.
> > > >
> > > > By referring to its implementation algorithm
> > > > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorit
> > > > hm), ported heap-buffer-overflow and use-after-freefunctions to
> > > > dpdk.
> > > >
> > > > Here is an example of heap-buffer-overflow bug:
> > > > ......
> > > > char *p = rte_zmalloc(NULL, 7, 0);
> > > > p[7] = 'a';
> > > > ......
> > > >
> > > > Here is an example of use-after-free bug:
> > > > ......
> > > > char *p = rte_zmalloc(NULL, 7, 0);
> > > > rte_free(p);
> > > > *p = 'a';
> > > > ......
> > > >
> > > > If you want to use this feature,
> > > > you need to use the following compilation options:
> > > > -Db_lundef=false -Db_sanitize=address
> > >
> > > # Thanks for this patch. It is a useful item.
> > >
> > > # Subject could be changed
> > > from:
> > > porting AddressSanitizer feature to DPDK to
> > > eal: support for AddressSanitizer
> > > or so
> >
> > Thanks for your positive feedback and review.
> > Good point, we will update the title in next version.
> >
> > >
> > > # Could you add a section in the documentation for Sanitizers to
> > > document the build time option and other points that users need to know.
> >
> > Make sense to add build option and key points to document, will add
> > this part in doc
> folder.
> >
> > > We can add other sanitizers such as UBSan etc in the future here
> > WIP to research other sanitizer tool.
>
> UBsan is a good candate.
> Some old DPDK patch for the same:
> http://patches.dpdk.org/project/dpdk/patch/1573832013-18946-1-git-send-
> email-hkalra@marvell.com/
>
Thanks for share, we will research the tool next.
> > >
> > > # Add a UT test case to make sure it is working in app/test or so.
> >
> > This tool could help to detect memory issue, need to change bad code to
> check if working.
>
> It is better to have a UT to test things are working. You could add it in
> app/test/test_address_sanity.c. Tests can be such that
> - Skip if not complied with Sanity enabled
> - Pass if the code detects the known bad code. You can have test cases with
> pubic rte_ API that internally exercise the verify your implementation related
> to new asan_* APIs.
Thanks Jacob for your suggestion, we will add unit test for Asan tool later.
>
>
> > Suggest listing demo code and tool capture information for user to try if
> tool works, also add this part into doc.
> >
> > >
> > > # Also, Please update the release note for this feature.
> > Sure, we can update the release note if code merge.
>
> Probably you can send v1 version next i.e change the RFC status to get
> merged.
Sure, we will send v1 patch if no obvious objection for that, hope patch could receive some ACKs and could success to merge, thanks.
>
>
> >
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-18 7:48 ` Lin, Xueqin
@ 2021-06-18 9:04 ` David Marchand
2021-06-22 3:26 ` Lin, Xueqin
2021-06-28 14:22 ` Burakov, Anatoly
0 siblings, 2 replies; 23+ messages in thread
From: David Marchand @ 2021-06-18 9:04 UTC (permalink / raw)
To: Lin, Xueqin
Cc: Jerin Jacob, Peng, ZhihongX, Burakov, Anatoly, Ananyev,
Konstantin, Stephen Hemminger, dpdk-dev
On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> > > Suggest listing demo code and tool capture information for user to try if
> > tool works, also add this part into doc.
> > >
> > > >
> > > > # Also, Please update the release note for this feature.
> > > Sure, we can update the release note if code merge.
> >
> > Probably you can send v1 version next i.e change the RFC status to get
> > merged.
>
> Sure, we will send v1 patch if no obvious objection for that, hope patch could receive some ACKs and could success to merge, thanks.
How did you test this work?
UNH recently started testing with ASAN and it reveals leaks just in
the unit test.
Merging these annotations will help catch more issues.
But users will hit the current issues that we must fix first.
--
David Marchand
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-18 9:04 ` David Marchand
@ 2021-06-22 3:26 ` Lin, Xueqin
2021-06-28 14:22 ` Burakov, Anatoly
1 sibling, 0 replies; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-22 3:26 UTC (permalink / raw)
To: David Marchand
Cc: Jerin Jacob, Peng, ZhihongX, Burakov, Anatoly, Ananyev,
Konstantin, Stephen Hemminger, dpdk-dev, Chen, Zhaoyan
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, June 18, 2021 5:04 PM
> To: Lin, Xueqin <xueqin.lin@intel.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Peng, ZhihongX
> <zhihongx.peng@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
>
> On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> > > > Suggest listing demo code and tool capture information for user to
> > > > try if
> > > tool works, also add this part into doc.
> > > >
> > > > >
> > > > > # Also, Please update the release note for this feature.
> > > > Sure, we can update the release note if code merge.
> > >
> > > Probably you can send v1 version next i.e change the RFC status to
> > > get merged.
> >
> > Sure, we will send v1 patch if no obvious objection for that, hope patch
> could receive some ACKs and could success to merge, thanks.
>
> How did you test this work?
>
> UNH recently started testing with ASAN and it reveals leaks just in the unit
> test.
Our lab WIP to run regression test with enabling Asan tool build, already reported dozens of issues by Asan tool, continue for unit test and issue resolve.
>
> Merging these annotations will help catch more issues.
> But users will hit the current issues that we must fix first.
>
>
Yeah, it is a powerful tool that could help to catch more issues and error log print, happy that more guys found this tool value and working on this.
So it is important and meaningful if this tool code could merge.
These issues only captured when enable Asan tool build, without the tool, not impact current test, also this tool is not supported before.
We are trying our best and WIP to do regression test based on our all of current regression test cases, capture issues and check.
Some resolved issue patches for Asan detected have been merged, and found that some of issues we captured are same to UNH captured.
But we can't cover all, once the code merge, more users could guarantee if their code or module have memory issue, and supply solution for issue fix.
> --
> David Marchand
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-18 9:04 ` David Marchand
2021-06-22 3:26 ` Lin, Xueqin
@ 2021-06-28 14:22 ` Burakov, Anatoly
2021-06-28 14:23 ` Jerin Jacob
` (2 more replies)
1 sibling, 3 replies; 23+ messages in thread
From: Burakov, Anatoly @ 2021-06-28 14:22 UTC (permalink / raw)
To: David Marchand, Lin, Xueqin
Cc: Jerin Jacob, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
On 18-Jun-21 10:04 AM, David Marchand wrote:
> On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
>>>> Suggest listing demo code and tool capture information for user to try if
>>> tool works, also add this part into doc.
>>>>
>>>>>
>>>>> # Also, Please update the release note for this feature.
>>>> Sure, we can update the release note if code merge.
>>>
>>> Probably you can send v1 version next i.e change the RFC status to get
>>> merged.
>>
>> Sure, we will send v1 patch if no obvious objection for that, hope patch could receive some ACKs and could success to merge, thanks.
>
> How did you test this work?
>
> UNH recently started testing with ASAN and it reveals leaks just in
> the unit test.
>
> Merging these annotations will help catch more issues.
> But users will hit the current issues that we must fix first.
>
As far as i can tell, the regular build is not affected by this patch,
so no issues will be hit until someone actually runs the test. IMO it's
better to merge it early to catch more issues than to gate the feature
on the condition that we fix all bugs unrelated to this feature first.
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-28 14:22 ` Burakov, Anatoly
@ 2021-06-28 14:23 ` Jerin Jacob
2021-06-30 8:15 ` Lin, Xueqin
2021-06-30 8:34 ` David Marchand
2 siblings, 0 replies; 23+ messages in thread
From: Jerin Jacob @ 2021-06-28 14:23 UTC (permalink / raw)
To: Burakov, Anatoly
Cc: David Marchand, Lin, Xueqin, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
On Mon, Jun 28, 2021 at 7:52 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 18-Jun-21 10:04 AM, David Marchand wrote:
> > On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> >>>> Suggest listing demo code and tool capture information for user to try if
> >>> tool works, also add this part into doc.
> >>>>
> >>>>>
> >>>>> # Also, Please update the release note for this feature.
> >>>> Sure, we can update the release note if code merge.
> >>>
> >>> Probably you can send v1 version next i.e change the RFC status to get
> >>> merged.
> >>
> >> Sure, we will send v1 patch if no obvious objection for that, hope patch could receive some ACKs and could success to merge, thanks.
> >
> > How did you test this work?
> >
> > UNH recently started testing with ASAN and it reveals leaks just in
> > the unit test.
> >
> > Merging these annotations will help catch more issues.
> > But users will hit the current issues that we must fix first.
> >
>
> As far as i can tell, the regular build is not affected by this patch,
> so no issues will be hit until someone actually runs the test. IMO it's
> better to merge it early to catch more issues than to gate the feature
> on the condition that we fix all bugs unrelated to this feature first.
+1
>
> --
> Thanks,
> Anatoly
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-28 14:22 ` Burakov, Anatoly
2021-06-28 14:23 ` Jerin Jacob
@ 2021-06-30 8:15 ` Lin, Xueqin
2021-06-30 8:34 ` David Marchand
2 siblings, 0 replies; 23+ messages in thread
From: Lin, Xueqin @ 2021-06-30 8:15 UTC (permalink / raw)
To: Burakov, Anatoly, David Marchand
Cc: Jerin Jacob, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, June 28, 2021 10:22 PM
> To: David Marchand <david.marchand@redhat.com>; Lin, Xueqin
> <xueqin.lin@intel.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Peng, ZhihongX
> <zhihongx.peng@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
>
> On 18-Jun-21 10:04 AM, David Marchand wrote:
> > On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> >>>> Suggest listing demo code and tool capture information for user to
> >>>> try if
> >>> tool works, also add this part into doc.
> >>>>
> >>>>>
> >>>>> # Also, Please update the release note for this feature.
> >>>> Sure, we can update the release note if code merge.
> >>>
> >>> Probably you can send v1 version next i.e change the RFC status to
> >>> get merged.
> >>
> >> Sure, we will send v1 patch if no obvious objection for that, hope patch
> could receive some ACKs and could success to merge, thanks.
> >
> > How did you test this work?
> >
> > UNH recently started testing with ASAN and it reveals leaks just in
> > the unit test.
> >
> > Merging these annotations will help catch more issues.
> > But users will hit the current issues that we must fix first.
> >
>
> As far as i can tell, the regular build is not affected by this patch, so no issues
> will be hit until someone actually runs the test. IMO it's better to merge it
> early to catch more issues than to gate the feature on the condition that we
> fix all bugs unrelated to this feature first.
Thanks for review and good feedback.
>
> --
> Thanks,
> Anatoly
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-28 14:22 ` Burakov, Anatoly
2021-06-28 14:23 ` Jerin Jacob
2021-06-30 8:15 ` Lin, Xueqin
@ 2021-06-30 8:34 ` David Marchand
2021-07-01 6:48 ` Lin, Xueqin
2 siblings, 1 reply; 23+ messages in thread
From: David Marchand @ 2021-06-30 8:34 UTC (permalink / raw)
To: Burakov, Anatoly
Cc: Lin, Xueqin, Jerin Jacob, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
On Mon, Jun 28, 2021 at 4:22 PM Burakov, Anatoly
<anatoly.burakov@intel.com> wrote:
>
> On 18-Jun-21 10:04 AM, David Marchand wrote:
> > On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> >>>> Suggest listing demo code and tool capture information for user to try if
> >>> tool works, also add this part into doc.
> >>>>
> >>>>>
> >>>>> # Also, Please update the release note for this feature.
> >>>> Sure, we can update the release note if code merge.
> >>>
> >>> Probably you can send v1 version next i.e change the RFC status to get
> >>> merged.
> >>
> >> Sure, we will send v1 patch if no obvious objection for that, hope patch could receive some ACKs and could success to merge, thanks.
> >
> > How did you test this work?
> >
> > UNH recently started testing with ASAN and it reveals leaks just in
> > the unit test.
> >
> > Merging these annotations will help catch more issues.
> > But users will hit the current issues that we must fix first.
> >
>
> As far as i can tell, the regular build is not affected by this patch,
> so no issues will be hit until someone actually runs the test. IMO it's
> better to merge it early to catch more issues than to gate the feature
> on the condition that we fix all bugs unrelated to this feature first.
- This is affecting more than unit tests.
$ meson setup build-asan -Db_lundef=false -Db_sanitize=address
...
$ ninja-build -C build-asan
ninja: Entering directory `build-asan'
[2801/2801] Linking target app/test/dpdk-test
$ ./devtools/test-null.sh build-asan
EAL: Detected 28 lcore(s)
EAL: Detected 1 NUMA nodes
EAL: Detected static linkage of DPDK
EAL: WARNING! Base virtual address hint (0x100005000 !=
0x7fb31c632000) not respected!
EAL: This may cause issues with mapping memory into secondary processes
EAL: Multi-process socket /run/user/1001/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: VFIO support initialized
EAL: WARNING! Base virtual address hint (0x10000b000 !=
0x7fb31c3b2000) not respected!
EAL: This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x100011000 !=
0x7fb31c375000) not respected!
EAL: This may cause issues with mapping memory into secondary processes
EAL: WARNING! Base virtual address hint (0x100017000 !=
0x7fb319bfe000) not respected!
EAL: This may cause issues with mapping memory into secondary processes
Interactive-mode selected
Auto-start selected
[...]
Bye...
EAL: recvmsg failed, Bad file descriptor
EAL: recvmsg failed, Bad file descriptor
EAL: recvmsg failed, Bad file descriptor
EAL: recvmsg failed, Bad file descriptor
EAL: recvmsg failed, Bad file descriptor
EAL: recvmsg failed, Bad file descriptor
Infinite loop of those messages.
In the thread with Owen, we also noticed what looks like a deadlock
with multiprocess when ASAN is enabled.
- Adding a new feature on top of something that does not work yet
seems at best premature to me.
This patch does not seem that much tested, since those issues above
are fairly easy to catch.
Anyway, the memory allocator is your stuff, so your call.
Prefix for the title of such a patch should be mem:.
--
David Marchand
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-30 8:34 ` David Marchand
@ 2021-07-01 6:48 ` Lin, Xueqin
2021-07-01 7:40 ` David Marchand
0 siblings, 1 reply; 23+ messages in thread
From: Lin, Xueqin @ 2021-07-01 6:48 UTC (permalink / raw)
To: David Marchand, Burakov, Anatoly
Cc: Jerin Jacob, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, June 30, 2021 4:34 PM
> To: Burakov, Anatoly <anatoly.burakov@intel.com>
> Cc: Lin, Xueqin <xueqin.lin@intel.com>; Jerin Jacob <jerinjacobk@gmail.com>;
> Peng, ZhihongX <zhihongx.peng@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
>
> On Mon, Jun 28, 2021 at 4:22 PM Burakov, Anatoly
> <anatoly.burakov@intel.com> wrote:
> >
> > On 18-Jun-21 10:04 AM, David Marchand wrote:
> > > On Fri, Jun 18, 2021 at 9:49 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> > >>>> Suggest listing demo code and tool capture information for user
> > >>>> to try if
> > >>> tool works, also add this part into doc.
> > >>>>
> > >>>>>
> > >>>>> # Also, Please update the release note for this feature.
> > >>>> Sure, we can update the release note if code merge.
> > >>>
> > >>> Probably you can send v1 version next i.e change the RFC status to
> > >>> get merged.
> > >>
> > >> Sure, we will send v1 patch if no obvious objection for that, hope patch
> could receive some ACKs and could success to merge, thanks.
> > >
> > > How did you test this work?
> > >
> > > UNH recently started testing with ASAN and it reveals leaks just in
> > > the unit test.
> > >
> > > Merging these annotations will help catch more issues.
> > > But users will hit the current issues that we must fix first.
> > >
> >
> > As far as i can tell, the regular build is not affected by this patch,
> > so no issues will be hit until someone actually runs the test. IMO
> > it's better to merge it early to catch more issues than to gate the
> > feature on the condition that we fix all bugs unrelated to this feature first.
>
> - This is affecting more than unit tests.
>
> $ meson setup build-asan -Db_lundef=false -Db_sanitize=address ...
>
> $ ninja-build -C build-asan
> ninja: Entering directory `build-asan'
> [2801/2801] Linking target app/test/dpdk-test
>
> $ ./devtools/test-null.sh build-asan
> EAL: Detected 28 lcore(s)
> EAL: Detected 1 NUMA nodes
> EAL: Detected static linkage of DPDK
> EAL: WARNING! Base virtual address hint (0x100005000 !=
> 0x7fb31c632000) not respected!
> EAL: This may cause issues with mapping memory into secondary processes
> EAL: Multi-process socket /run/user/1001/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'VA'
> EAL: VFIO support initialized
> EAL: WARNING! Base virtual address hint (0x10000b000 !=
> 0x7fb31c3b2000) not respected!
> EAL: This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x100011000 !=
> 0x7fb31c375000) not respected!
> EAL: This may cause issues with mapping memory into secondary processes
> EAL: WARNING! Base virtual address hint (0x100017000 !=
> 0x7fb319bfe000) not respected!
> EAL: This may cause issues with mapping memory into secondary processes
> Interactive-mode selected
> Auto-start selected
> [...]
> Bye...
> EAL: recvmsg failed, Bad file descriptor
> EAL: recvmsg failed, Bad file descriptor
> EAL: recvmsg failed, Bad file descriptor
> EAL: recvmsg failed, Bad file descriptor
> EAL: recvmsg failed, Bad file descriptor
> EAL: recvmsg failed, Bad file descriptor
>
> Infinite loop of those messages.
> In the thread with Owen, we also noticed what looks like a deadlock with
> multiprocess when ASAN is enabled.
>
>
> - Adding a new feature on top of something that does not work yet seems at
> best premature to me.
> This patch does not seem that much tested, since those issues above are
> fairly easy to catch.
Great that you have tried the patch and capture the issue. It is DPDK issue not this patch issue.
We also captured this type issues and selected one, reported in internal system JARA and bugzilla in May.
Pls check https://bugs.dpdk.org/show_bug.cgi?id=722 for detail, some developers are analysis this issue in JIRA but not resolved so far, Anatoly should also know this.
It is DPDK issue, not this patch issue, it doesn't mean we not do much test.
We put all of thousands of regression cases as case list for this patch test, some issues still are tracking.
>
> Anyway, the memory allocator is your stuff, so your call.
>
> Prefix for the title of such a patch should be mem:
>
>
> --
> David Marchand
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-07-01 6:48 ` Lin, Xueqin
@ 2021-07-01 7:40 ` David Marchand
2021-07-02 11:05 ` Lin, Xueqin
0 siblings, 1 reply; 23+ messages in thread
From: David Marchand @ 2021-07-01 7:40 UTC (permalink / raw)
To: Lin, Xueqin, Burakov, Anatoly
Cc: Jerin Jacob, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
On Thu, Jul 1, 2021 at 8:48 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> We also captured this type issues and selected one, reported in internal system JARA and bugzilla in May.
> Pls check https://bugs.dpdk.org/show_bug.cgi?id=722 for detail, some developers are analysis this issue in JIRA but not resolved so far, Anatoly should also know this.
Thanks for sharing this.
> It is DPDK issue, not this patch issue, it doesn't mean we not do much test.
> We put all of thousands of regression cases as case list for this patch test, some issues still are tracking.
Sorry, can you rephrase?
I sure hope there are not thousands of issues with ASAN.
Are all identified issues reported in dpdk.org bugzilla?
It would help to have a hint in the bzs title, like "caught with ASAN".
I can't find a report about the infinite loop on invalid fds, making a
dpdk app unable to exit gracefully with ASAN enabled.
But at least, I sent a fix for this, waiting for Anatoly to review.
https://patchwork.dpdk.org/project/dpdk/patch/20210614091213.3953-1-david.marchand@redhat.com/
> >
> > Anyway, the memory allocator is your stuff, so your call.
> >
> > Prefix for the title of such a patch should be mem:
Anatoly and Jerin like your patch, but it is still a RFC and there
were comments on a missing release note update and unit test.
Please address those comments and submit a regular patch.
Thanks.
--
David Marchand
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-07-01 7:40 ` David Marchand
@ 2021-07-02 11:05 ` Lin, Xueqin
0 siblings, 0 replies; 23+ messages in thread
From: Lin, Xueqin @ 2021-07-02 11:05 UTC (permalink / raw)
To: David Marchand, Burakov, Anatoly
Cc: Jerin Jacob, Peng, ZhihongX, Ananyev, Konstantin,
Stephen Hemminger, dpdk-dev
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, July 1, 2021 3:41 PM
> To: Lin, Xueqin <xueqin.lin@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>
> Cc: Jerin Jacob <jerinjacobk@gmail.com>; Peng, ZhihongX
> <zhihongx.peng@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Stephen Hemminger
> <stephen@networkplumber.org>; dpdk-dev <dev@dpdk.org>
> Subject: Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
>
> On Thu, Jul 1, 2021 at 8:48 AM Lin, Xueqin <xueqin.lin@intel.com> wrote:
> > We also captured this type issues and selected one, reported in internal
> system JARA and bugzilla in May.
> > Pls check https://bugs.dpdk.org/show_bug.cgi?id=722 for detail, some
> developers are analysis this issue in JIRA but not resolved so far, Anatoly
> should also know this.
>
> Thanks for sharing this.
>
>
> > It is DPDK issue, not this patch issue, it doesn't mean we not do much test.
> > We put all of thousands of regression cases as case list for this patch test,
> some issues still are tracking.
>
> Sorry, can you rephrase?
> I sure hope there are not thousands of issues with ASAN.
Relax, don't have so many issues.
I mean we have thousands of automation dpdk cases, run the cases for DPDK version build with Asan tool, check dpdk code if have memory issue.
Some issues are still WIP to check.
>
>
> Are all identified issues reported in dpdk.org bugzilla?
> It would help to have a hint in the bzs title, like "caught with ASAN".
No, we reported common issue to Bugzilla.
Good idea to add title.
>
> I can't find a report about the infinite loop on invalid fds, making a dpdk app
> unable to exit gracefully with ASAN enabled.
> But at least, I sent a fix for this, waiting for Anatoly to review.
> https://patchwork.dpdk.org/project/dpdk/patch/20210614091213.3953-1-
> david.marchand@redhat.com/
>
>
> > >
> > > Anyway, the memory allocator is your stuff, so your call.
> > >
> > > Prefix for the title of such a patch should be mem:
>
> Anatoly and Jerin like your patch, but it is still a RFC and there were
> comments on a missing release note update and unit test.
> Please address those comments and submit a regular patch.
Sure, we are preparing the v1 patch according to comment.
>
>
> Thanks.
>
> --
> David Marchand
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-06-15 8:12 ` [dpdk-dev] [RFC v2] " zhihongx.peng
2021-06-15 8:40 ` Jerin Jacob
@ 2021-07-06 20:40 ` David Christensen
2021-07-06 23:12 ` Stephen Hemminger
1 sibling, 1 reply; 23+ messages in thread
From: David Christensen @ 2021-07-06 20:40 UTC (permalink / raw)
To: zhihongx.peng, anatoly.burakov, konstantin.ananyev, stephen
Cc: dev, xueqin.lin
On 6/15/21 1:12 AM, zhihongx.peng@intel.com wrote:
> From: Zhihong Peng <zhihongx.peng@intel.com>
>
> AddressSanitizer (ASan) is a google memory error detect
> standard tool. It could help to detect use-after-free and
> {heap,stack,global}-buffer overflow bugs in C/C++ programs,
> print detailed error information when error happens, large
> improve debug efficiency.
>
> By referring to its implementation algorithm
> (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> ported heap-buffer-overflow and use-after-freefunctions to dpdk.
>
> Here is an example of heap-buffer-overflow bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> p[7] = 'a';
> ......
>
> Here is an example of use-after-free bug:
> ......
> char *p = rte_zmalloc(NULL, 7, 0);
> rte_free(p);
> *p = 'a';
> ......
>
> If you want to use this feature,
> you need to use the following compilation options:
> -Db_lundef=false -Db_sanitize=address
Any library dependencies here that might be architecture specific? I
applied the patch to a POWER9 system with RHEL 8.3 and observed a SEGV:
sudo /home/drc/src/dpdk/build/app/dpdk-testpmd -l 64-71
--vdev=net_memif0,role=server,id=0 --vdev=net_memif1,role=client,id=0
--no-pci -- -i --numa --port-numa-config=0,8,1,8
--ring-numa-config=0,3,8,1,3,8 --socket-num=8
EAL: Detected 128 lcore(s)
EAL: Detected 2 NUMA nodes
EAL: Detected static linkage of DPDK
EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
EAL: Selected IOVA mode 'VA'
EAL: No available 1048576 kB hugepages reported
EAL: VFIO support initialized
AddressSanitizer:DEADLYSIGNAL
=================================================================
==3011526==ERROR: AddressSanitizer: SEGV on unknown address
0x0002a0177bd0 (pc 0x000011411ce0 bp 0x7fffccd738b0 sp 0x7fffccd738b0 T0)
==3011526==The signal is caused by a UNKNOWN memory access.
#0 0x11411cdc in asan_set_shadow.constprop.4
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11411cdc)
#1 0x114131ec in malloc_elem_alloc
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x114131ec)
#2 0x11416adc in heap_alloc.isra.1
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11416adc)
#3 0x11419570 in malloc_heap_alloc_on_heap_id.isra.5
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11419570)
#4 0x1141977c in malloc_heap_alloc
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x1141977c)
#5 0x11421794 in rte_malloc_socket
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11421794)
#6 0x11421e14 in rte_zmalloc_socket
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11421e14)
#7 0x11422250 in rte_zmalloc
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11422250)
#8 0x114222f4 in rte_calloc
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x114222f4)
#9 0x11428fa4 in rte_service_init
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11428fa4)
#10 0x11433680 in rte_eal_init
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11433680)
#11 0x1039a734 in main
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x1039a734)
#12 0x7fffa3664074 in generic_start_main ../csu/libc-start.c:308
#13 0x7fffa3664260 in __libc_start_main
../sysdeps/unix/sysv/linux/powerpc/libc-start.c:102
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV
(/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11411cdc) in
asan_set_shadow.constprop.4
==3011526==ABORTING
Dave
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [dpdk-dev] [RFC v2] porting AddressSanitizer feature to DPDK
2021-07-06 20:40 ` David Christensen
@ 2021-07-06 23:12 ` Stephen Hemminger
0 siblings, 0 replies; 23+ messages in thread
From: Stephen Hemminger @ 2021-07-06 23:12 UTC (permalink / raw)
To: David Christensen
Cc: zhihongx.peng, anatoly.burakov, konstantin.ananyev, dev, xueqin.lin
On Tue, 6 Jul 2021 13:40:56 -0700
David Christensen <drc@linux.vnet.ibm.com> wrote:
> On 6/15/21 1:12 AM, zhihongx.peng@intel.com wrote:
> > From: Zhihong Peng <zhihongx.peng@intel.com>
> >
> > AddressSanitizer (ASan) is a google memory error detect
> > standard tool. It could help to detect use-after-free and
> > {heap,stack,global}-buffer overflow bugs in C/C++ programs,
> > print detailed error information when error happens, large
> > improve debug efficiency.
> >
> > By referring to its implementation algorithm
> > (https://github.com/google/sanitizers/wiki/AddressSanitizerAlgorithm),
> > ported heap-buffer-overflow and use-after-freefunctions to dpdk.
> >
> > Here is an example of heap-buffer-overflow bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > p[7] = 'a';
> > ......
> >
> > Here is an example of use-after-free bug:
> > ......
> > char *p = rte_zmalloc(NULL, 7, 0);
> > rte_free(p);
> > *p = 'a';
> > ......
> >
> > If you want to use this feature,
> > you need to use the following compilation options:
> > -Db_lundef=false -Db_sanitize=address
>
> Any library dependencies here that might be architecture specific? I
> applied the patch to a POWER9 system with RHEL 8.3 and observed a SEGV:
>
> sudo /home/drc/src/dpdk/build/app/dpdk-testpmd -l 64-71
> --vdev=net_memif0,role=server,id=0 --vdev=net_memif1,role=client,id=0
> --no-pci -- -i --numa --port-numa-config=0,8,1,8
> --ring-numa-config=0,3,8,1,3,8 --socket-num=8
> EAL: Detected 128 lcore(s)
> EAL: Detected 2 NUMA nodes
> EAL: Detected static linkage of DPDK
> EAL: Multi-process socket /var/run/dpdk/rte/mp_socket
> EAL: Selected IOVA mode 'VA'
> EAL: No available 1048576 kB hugepages reported
> EAL: VFIO support initialized
> AddressSanitizer:DEADLYSIGNAL
> =================================================================
> ==3011526==ERROR: AddressSanitizer: SEGV on unknown address
> 0x0002a0177bd0 (pc 0x000011411ce0 bp 0x7fffccd738b0 sp 0x7fffccd738b0 T0)
> ==3011526==The signal is caused by a UNKNOWN memory access.
> #0 0x11411cdc in asan_set_shadow.constprop.4
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11411cdc)
> #1 0x114131ec in malloc_elem_alloc
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x114131ec)
> #2 0x11416adc in heap_alloc.isra.1
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11416adc)
> #3 0x11419570 in malloc_heap_alloc_on_heap_id.isra.5
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11419570)
> #4 0x1141977c in malloc_heap_alloc
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x1141977c)
> #5 0x11421794 in rte_malloc_socket
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11421794)
> #6 0x11421e14 in rte_zmalloc_socket
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11421e14)
> #7 0x11422250 in rte_zmalloc
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11422250)
> #8 0x114222f4 in rte_calloc
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x114222f4)
> #9 0x11428fa4 in rte_service_init
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11428fa4)
> #10 0x11433680 in rte_eal_init
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11433680)
> #11 0x1039a734 in main
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x1039a734)
> #12 0x7fffa3664074 in generic_start_main ../csu/libc-start.c:308
> #13 0x7fffa3664260 in __libc_start_main
> ../sysdeps/unix/sysv/linux/powerpc/libc-start.c:102
>
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: SEGV
> (/home/drc/src/dpdk/build/app/dpdk-testpmd+0x11411cdc) in
> asan_set_shadow.constprop.4
> ==3011526==ABORTING
>
> Dave
ASAN says you should use -fno-omit-frame-pointer to get reasonable backtrace.
^ permalink raw reply [flat|nested] 23+ messages in thread