Re: [dpdk-dev] [PATCH v3] Enable AddressSanitizer feature on DPDK

["Peng, ZhihongX" <zhihongx.peng@intel.com>]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Monday, September 27, 2021 8:03 PM
> To: Peng, ZhihongX <zhihongx.peng@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; stephen@networkplumber.org
> Cc: dev@dpdk.org; Lin, Xueqin <xueqin.lin@intel.com>
> Subject: Re: [PATCH v3] Enable AddressSanitizer feature on DPDK
> 
> On 18-Sep-21 8:41 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),
> > enable heap-buffer-overflow and use-after-free functions on dpdk.
> > DPDK ASAN function currently only supports on Linux x86_64.
> >
> > 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 add below compilation options when compiling code:
> > -Dbuildtype=debug -Db_lundef=false -Db_sanitize=address
> > "-Dbuildtype=debug": Display code information when coredump occurs in
> > the program.
> > "-Db_lundef=false": It is enabled by default, and needs to be disabled
> > when using asan.
> >
> > Signed-off-by: Xueqin Lin <xueqin.lin@intel.com>
> > Signed-off-by: Zhihong Peng <zhihongx.peng@intel.com>
> > ---
> >   doc/guides/prog_guide/asan.rst  | 130 ++++++++++++++++++++++
> >   doc/guides/prog_guide/index.rst |   1 +
> >   lib/eal/common/malloc_elem.c    |  26 ++++-
> >   lib/eal/common/malloc_elem.h    | 184
> +++++++++++++++++++++++++++++++-
> >   lib/eal/common/malloc_heap.c    |  12 +++
> >   lib/eal/common/rte_malloc.c     |   9 +-
> >   lib/pipeline/rte_swx_pipeline.c |   4 +-
> >   7 files changed, 359 insertions(+), 7 deletions(-)
> >   create mode 100644 doc/guides/prog_guide/asan.rst
> >
> > diff --git a/doc/guides/prog_guide/asan.rst
> > b/doc/guides/prog_guide/asan.rst new file mode 100644 index
> > 0000000000..a0589d9b8a
> > --- /dev/null
> > +++ b/doc/guides/prog_guide/asan.rst
> > @@ -0,0 +1,130 @@
> > +.. Copyright (c) <2021>, Intel Corporation
> > +   All rights reserved.
> > +
> > +Memory error detect standard tool - AddressSanitizer(Asan)
> >
> +=========================================================
> =
> > +
> > +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)
> > +, enabled heap-buffer-overflow and use-after-free functions on dpdk.
> > +DPDK ASAN function currently only supports on Linux x86_64.
> 
> I think the description is a little to wordy and unnecessarily redundant (pun
> intended!) in places. Suggested rewording:
> 
> `AddressSanitizer
> <https://github.com/google/sanitizers/wiki/AddressSanitizer>` (ASan) is a
> widely-used debugging tool to detect memory access errors. It helps detect
> issues like use-after-free, various kinds of buffer overruns in C/C++ programs,
> and other similar errors, as well as printing out detailed debug information
> whenever an error is detected.
> 
> DPDK ASan functionality is currently only supported Linux x86_64.
> 

The v6 version will be modified.

> > +
> > +AddressSanitizer is a part of LLVM(3.1+)and GCC(4.8+).
> > +
> > +Example heap-buffer-overflow error
> > +----------------------------------
> > +
> > +Following error was reported when Asan was enabled::
> > +
> > +    Applied 9 bytes of memory, but accessed the 10th byte of memory,
> > +    so heap-buffer-overflow appeared.
> > +
> > +Below code results in this error::
> > +
> > +    char *p = rte_zmalloc(NULL, 9, 0);
> > +    if (!p) {
> > +        printf("rte_zmalloc error.");
> > +        return -1;
> > +    }
> > +    p[9] = 'a';
> > +
> > +The error log::
> > +
> > +    ==49433==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x7f773fafa249 at pc 0x5556b13bdae4 bp 0x7ffeb4965e40 sp 0x7ffeb4965e30
> WRITE of size 1 at 0x7f773fafa249 thread T0
> > +    #0 0x5556b13bdae3 in
> asan_heap_buffer_overflow ../app/test/test_asan_heap_buffer_overflow.
> c:25
> > +    #1 0x5556b043e9d4 in
> cmd_autotest_parsed ../app/test/commands.c:71
> > +    #2 0x5556b1cdd4b0 in
> cmdline_parse ../lib/cmdline/cmdline_parse.c:290
> > +    #3 0x5556b1cd8987 in cmdline_valid_buffer ../lib/cmdline/cmdline.c:26
> > +    #4 0x5556b1ce477a in rdline_char_in ../lib/cmdline/cmdline_rdline.c:421
> > +    #5 0x5556b1cd923e in cmdline_in ../lib/cmdline/cmdline.c:149
> > +    #6 0x5556b1cd9769 in cmdline_interact ../lib/cmdline/cmdline.c:223
> > +    #7 0x5556b045f53b in main ../app/test/test.c:234
> > +    #8 0x7f7f1eba90b2 in __libc_start_main (/lib/x86_64-linux-
> gnu/libc.so.6+0x270b2)
> > +    #9 0x5556b043e70d in _start
> > + (/home/pzh/yyy/x86_64-native-linuxapp-gcc/app/test/dpdk-
> test+0x7ce70
> > + d)
> > +
> > +    Address 0x7f773fafa249 is a wild pointer.
> > +    SUMMARY: AddressSanitizer: heap-buffer-
> overflow ../app/test/test_asan_heap_buffer_overflow.c:25 in
> asan_heap_buffer_overflow
> > +    Shadow bytes around the buggy address:
> > +    0x0fef67f573f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0fef67f57400: fa fa 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0fef67f57410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0fef67f57420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0fef67f57430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    =>0x0fef67f57440: 00 00 00 00 00 00 fa fa 00[01]fa 00 00 00 00 00
> > +    0x0fef67f57450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0fef67f57460: 00 00 00 00 00 00 fa fa 00 00 00 00 00 00 00 00
> > +    0x0fef67f57470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0fef67f57480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +
> > +Example use-after-free error
> > +----------------------------
> > +
> > +Following error was reported when Asan was enabled::
> > +
> > +    Applied for 9 bytes of memory, and accessed the first byte after
> > +    released, so heap-use-after-free appeared.
> > +
> > +Below code results in this error::
> > +
> > +    char *p = rte_zmalloc(NULL, 9, 0);
> > +    if (!p) {
> > +        printf("rte_zmalloc error.");
> > +        return -1;
> > +    }
> > +    rte_free(p);
> > +    *p = 'a';
> > +
> > +The error log::
> > +
> > +    ==49478==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x7fe2ffafa240 at pc 0x56409b084bc8 bp 0x7ffef62c57d0 sp 0x7ffef62c57c0
> WRITE of size 1 at 0x7fe2ffafa240 thread T0
> > +    #0 0x56409b084bc7 in
> asan_use_after_free ../app/test/test_asan_use_after_free.c:26
> > +    #1 0x56409a1059d4 in
> cmd_autotest_parsed ../app/test/commands.c:71
> > +    #2 0x56409b9a44b0 in
> cmdline_parse ../lib/cmdline/cmdline_parse.c:290
> > +    #3 0x56409b99f987 in cmdline_valid_buffer ../lib/cmdline/cmdline.c:26
> > +    #4 0x56409b9ab77a in
> rdline_char_in ../lib/cmdline/cmdline_rdline.c:421
> > +    #5 0x56409b9a023e in cmdline_in ../lib/cmdline/cmdline.c:149
> > +    #6 0x56409b9a0769 in cmdline_interact ../lib/cmdline/cmdline.c:223
> > +    #7 0x56409a12653b in main ../app/test/test.c:234
> > +    #8 0x7feafafc20b2 in __libc_start_main (/lib/x86_64-linux-
> gnu/libc.so.6+0x270b2)
> > +    #9 0x56409a10570d in _start
> > + (/home/pzh/yyy/x86_64-native-linuxapp-gcc/app/test/dpdk-
> test+0x7ce70
> > + d)
> > +
> > +    Address 0x7fe2ffafa240 is a wild pointer.
> > +    SUMMARY: AddressSanitizer: heap-use-after-
> free ../app/test/test_asan_use_after_free.c:26 in asan_use_after_free
> > +    Shadow bytes around the buggy address:
> > +    0x0ffcdff573f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57400: fa fa 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    =>0x0ffcdff57440: 00 00 00 00 00 00 00 00[fd]fd fd fd fd fd fd fd
> > +    0x0ffcdff57450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57460: 00 00 00 00 00 00 fa fa 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +    0x0ffcdff57490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > +
> > +Usage
> > +-----
> > +
> > +meson build
> > +^^^^^^^^^^^
> > +
> > +To enable Asan in meson build system, use following meson build
> command:
> > +
> > +Example usage::
> > +
> > + meson build -Dbuildtype=debug -Db_lundef=false -Db_sanitize=address
> > + ninja -C build
> > +
> > +.. Note::
> > +
> > +  a) The issue of asan wild pointer is that dpdk asan tool is not fully
> adapted to google asan.
> > +     For example: Address 0x7fe2ffafa240 is a wild pointer.
> > +  b) Centos8 needs to install libasan separately.
> > +  c) If the program uses cmdline, when a memory bug occurs, need to
> execute the "stty echo" command.
> 
> I think all of the terms used here should be properly capitalized, e.g.
> "ASan" instead of "asan", "DPDK" instead of "dpdk", etc.
> 
> Also, i don't quite follow the 'a)' section - what exactly is a wild pointer? What
> makes a pointer 'wild', and why do we care? If the point is that DPDK's ASan
> implementation does not support all ASan features, then maybe this could
> be worded better, e.g. like so:
> 
> a) Some of the features of ASan (for example, 'wild pointers') are not
> currently supported by DPDK's implementation

The v6 version will be modified.

> > diff --git a/doc/guides/prog_guide/index.rst
> > b/doc/guides/prog_guide/index.rst index 2dce507f46..df8a4b93e1 100644
> > --- a/doc/guides/prog_guide/index.rst
> > +++ b/doc/guides/prog_guide/index.rst
> > @@ -71,3 +71,4 @@ Programmer's Guide
> >       lto
> >       profile_app
> >       glossary
> > +    asan
> > diff --git a/lib/eal/common/malloc_elem.c
> > b/lib/eal/common/malloc_elem.c index c2c9461f1d..bdd20a162e 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 a1e5f7f02c..01a739f2ea 100644
> > --- a/lib/eal/common/malloc_elem.h
> > +++ b/lib/eal/common/malloc_elem.h
> > @@ -7,6 +7,14 @@
> >
> >   #include <stdbool.h>
> >
> > +#ifdef __SANITIZE_ADDRESS__
> > +#define RTE_MALLOC_ASAN
> > +#elif defined(__has_feature)
> > +# if __has_feature(address_sanitizer) #define RTE_MALLOC_ASAN #
> endif
> > +#endif
> > +
> >   #define MIN_DATA_SIZE (RTE_CACHE_LINE_SIZE)
> >
> >   /* dummy definition of struct so we can use pointers to it in
> > malloc_elem struct */ @@ -36,10 +44,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 int MALLOC_ELEM_HEADER_LEN = sizeof(struct
> > +malloc_elem);
> > +
> >   #ifndef RTE_MALLOC_DEBUG
> > -static const unsigned MALLOC_ELEM_TRAILER_LEN = 0;
> > +#ifdef RTE_MALLOC_ASAN
> > +static const unsigned int MALLOC_ELEM_TRAILER_LEN =
> > +RTE_CACHE_LINE_SIZE; #else static const unsigned int
> > +MALLOC_ELEM_TRAILER_LEN; #endif
> >
> >   /* dummy function - just check if pointer is non-null */
> >   static inline int
> > @@ -55,7 +73,7 @@ set_trailer(struct malloc_elem *elem __rte_unused){
> > }
> >
> >
> >   #else
> > -static const unsigned MALLOC_ELEM_TRAILER_LEN =
> RTE_CACHE_LINE_SIZE;
> > +static const unsigned int MALLOC_ELEM_TRAILER_LEN =
> > +RTE_CACHE_LINE_SIZE;
> 
> This doesn't look like it's changing anything - maybe drop this particular hun
> from the patch?
>

Solve the issue of static code inspection.

> >
> >   #define MALLOC_HEADER_COOKIE   0xbadbadbadadd2e55ULL /**<
> Header cookie. */
> >   #define MALLOC_TRAILER_COOKIE  0xadd2e55badbadbadULL /**< Trailer
> > cookie.*/ @@ -90,9 +108,169 @@ 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)
> 
> Maybe make it more pointer-safe, like so?
> 
> #define ASAN_MEM_SHIFT(mem) ((void*)((uintptr_t)mem >> 3)) #define
> ASAN_BASE_ADDR 0x00007fff8000 #define ASAN_MEM_TO_SHADOW(mem)
> RTE_PTR_ADD(ASAN_MEM_SHIFT(mem),
> ASAN_BASE_ADDR)
> 

The v6 version will be modified.

> We could even add a new rte_common.h macros for
> RTE_PTR_LSHIFT/RSHIFT.
> The advantage of this suggestion is that you won't need to make so many
> typecasts any more.
> 
> > +
> > +#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));
> 
> How about
> 
> 	shadow = ASAN_MEM_TO_SHADOW(RTE_PTR_ADD(ptr, offset));
> 
> ? The 'shadow' variable can be void*, so you can drop the cast here too.
> 

The v6 version will be modified.

> > +		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 as 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);
> 
> I think DPDK code style is to declare all variables at the beginning of
> the block.

The v6 version will be modified.

> 
> > +		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);
> 
> It seems to me that the code can be rewritten to be more understandable.
> For example, you could opt for more meaningful variable names and not
> reuse 'ptr' for everything. Ex.:
> 
> 	elem_start = RTE_PTR_ADD(elem, MALLOC_ELEM_HEADER_LEN);
> 	start_ptr = RTE_PTR_SUB(elem_start, ASAN_SHADOW_GRAIN_SIZE);
> 	start_shadow = ASAN_MEM_TO_SHADOW(start_ptr);
> 	// just following your code here, but why are we doing it twice?
> 	start_redzone = RTE_PTR_SUB(start_shadow,
> ASAN_SHADOW_GRAIN_SIZE);
> 
> 	asan_set_shadow(start_shadow, ASAN_MEM_REDZONE_FLAG);
> 	asan_set_shadow(start_redzone, ASAN_MEM_REDZONE_FLAG);
> 
> 	...
> 
> And similar for the rest of the code: more meaningful variable names,
> more intermediate variables, less raw bit/ptr manipulation please :)

The v6 version will be modified.

> > +	}
> > +}
> > +
> > +/*
> > + * 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);
> 
> It seems to me that you're repeating a lot of code here that you already
> have in `asan_set_redzone()` - maybe factor common bits out into a
> function? For example,
> 

> static inlind void ptr_set_shadow(void *ptr, int shadow_flags) {
> 	startptr = RTE_PTR_SUB(ptr, ASAN_SHADOW_GRAIN_SIZE);
> 	shadow = ASAN_MEM_TO_SHADOW(startptr);
> 	asan_set_shadow(shadow, shadow_flags);
> 	shadow = RTE_PTR_SUB(shadow, ASAN_SHADOW_GRAIN_SIZE);
> 	asan_set_shadow(shadow, shadow_flags);
> }
> 
> ...
> 
> and then call this from wherever you like? Because this pattern seems to
> be repeating everywhere, the only difference being the ptr value and the
> set_shadow flags.

The variable names have been modified to make the code clearer.
Extracting public functions is not particularly useful, and
this is a logical implementation, not general code.

static inline void
asan_clear_redzone(struct malloc_elem *elem)
{
	uintptr_t head_redzone;
	uintptr_t tail_redzone;
	void *head_shadow;
	void *tail_shadow;

	if (elem != NULL) {
		elem = RTE_PTR_ADD(elem, elem->pad);

		/* Clear mark before the start of the allocated memory */
		head_redzone = (uintptr_t)RTE_PTR_ADD(elem,
			MALLOC_ELEM_HEADER_LEN - ASAN_SHADOW_GRAIN_SIZE);
		head_shadow = ASAN_MEM_TO_SHADOW(head_redzone);
		asan_set_shadow(head_shadow, 0x00);
		head_shadow = ASAN_MEM_TO_SHADOW(head_redzone
				- ASAN_SHADOW_GRAIN_SIZE);
		asan_set_shadow(head_shadow, 0x00);

		/* Clear mark after the end of the allocated memory */
		tail_redzone = (uintptr_t)RTE_PTR_ADD(elem,
			MALLOC_ELEM_HEADER_LEN + elem->user_size);
		tail_shadow = ASAN_MEM_TO_SHADOW(tail_redzone);
		asan_set_shadow(tail_shadow, 0x00);
		tail_shadow = ASAN_MEM_TO_SHADOW(tail_redzone
				+ ASAN_SHADOW_GRAIN_SIZE);
		asan_set_shadow(tail_shadow, 0x00);
	}
}

> > +	}
> > +}
> > +
> > +static inline size_t
> > +old_malloc_size(struct malloc_elem *elem)
> > +{
> > +	if (elem->state != ELEM_PAD)
> > +		elem = RTE_PTR_ADD(elem, elem->pad);
> > +
> > +	return elem->user_size;
> > +}
> > +#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) { }
> > +
> > +static inline size_t
> > +old_malloc_size(struct malloc_elem *elem)
> > +{
> > +	return elem->size - elem->pad - MALLOC_ELEM_OVERHEAD;
> > +}
> > +#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 ee400f38ec..775d6789df 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/rte_malloc.c b/lib/eal/common/rte_malloc.c
> > index 9d39e58c08..d0bec26920 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;
> >   	}
> >
> > @@ -192,7 +199,7 @@ rte_realloc_socket(void *ptr, size_t size, unsigned
> int align, int socket)
> >   	if (new_ptr == NULL)
> >   		return NULL;
> >   	/* elem: |pad|data_elem|data|trailer| */
> > -	const size_t old_size = elem->size - elem->pad -
> MALLOC_ELEM_OVERHEAD;
> > +	const size_t old_size = old_malloc_size(elem);
> >   	rte_memcpy(new_ptr, ptr, old_size < size ? old_size : size);
> >   	rte_free(ptr);
> >
> > diff --git a/lib/pipeline/rte_swx_pipeline.c
> b/lib/pipeline/rte_swx_pipeline.c
> > index 8eb978a30c..aaa0107d02 100644
> > --- a/lib/pipeline/rte_swx_pipeline.c
> > +++ b/lib/pipeline/rte_swx_pipeline.c
> > @@ -6340,7 +6340,7 @@ instr_meter_translate(struct rte_swx_pipeline *p,
> >   		return 0;
> >   	}
> >
> > -	CHECK(0, EINVAL);
> > +	return -EINVAL;
> 
> Why is this here? Accidental change?
> 

After adding ASan, the gcc compilation check will be stricter.
"Control reaches end of non-void function" error occurs here. Will be split into two patches.

> >   }
> >
> >   static inline struct meter *
> > @@ -8025,7 +8025,7 @@ instr_translate(struct rte_swx_pipeline *p,
> >   					      instr,
> >   					      data);
> >
> > -	CHECK(0, EINVAL);
> > +	return -EINVAL;
> 
> Same as above - accidental change?
> 
> >   }
> >
> >   static struct instruction_data *
> >
> 
> --
> Thanks,
> Anatoly